| Message ID | 20170804200915.56738-5-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Aug 4, 2017 at 4:09 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > FOR_EACH_PTR(target->users, pu) { > + if (!pu) > + continue; Your previous patch looks fine. Here in my previous email feed back I mean to include this check into FOR_EACH_PTR() so you don't need touch every pseudo_user list looping every where. Just do some thing like if (list->rm && list->list[__nr] == NULL) continue; inside the FOR_EACH_PTR(). and REVERSE as well. It will not have impact on other list because other list will not have list->rm != 0. With this change, the caller side of the change not needed. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 4:17 PM, Christopher Li <sparse@chrisli.org> wrote: > > Your previous patch looks fine. I mean your patch 1-3 looks fine. This one can be simplified. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 10:17 PM, Christopher Li <sparse@chrisli.org> wrote: > On Fri, Aug 4, 2017 at 4:09 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> FOR_EACH_PTR(target->users, pu) { >> + if (!pu) >> + continue; > > Your previous patch looks fine. > > Here in my previous email feed back I mean to include this check into > FOR_EACH_PTR() > so you don't need touch every pseudo_user list looping every where. > Just do some thing like > if (list->rm && list->list[__nr] == NULL) > continue; > > inside the FOR_EACH_PTR(). and REVERSE as well. > > It will not have impact on other list because other list will not > have list->rm != 0. It will not have a functional impact but you will add this test (and code for both tests) for every other use of the list macros for which it is totally unneeded. Are you sure to really want that? -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 4:34 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > It will not have a functional impact but you will add this test > (and code for both tests) for every other use of the list macros > for which it is totally unneeded. > Are you sure to really want that? I am not sure we are on the same page. Only 2 macro need to be updated: DO_FOR_EACH_PTR, DO_FOR_EACH_REVERSE. Maybe DO_REPAIR(), DO_NEXT() and DO_REVERSE() if we are ambition. It think it is fine without touch the last three also. That is not too bad, is it? Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 10:48 PM, Christopher Li <sparse@chrisli.org> wrote: > On Fri, Aug 4, 2017 at 4:34 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> It will not have a functional impact but you will add this test >> (and code for both tests) for every other use of the list macros >> for which it is totally unneeded. >> Are you sure to really want that? > > I am not sure we are on the same page. Only 2 macro need to be updated: > DO_FOR_EACH_PTR, DO_FOR_EACH_REVERSE. It's not for the number of macros defined. It's about the code size & the run-time effect on the iteration of the other lists. It seems simple enough but it would add two load-test-branch to every such macro invocation and these macros are already quite heavy. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 5:03 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> I am not sure we are on the same page. Only 2 macro need to be updated: >> DO_FOR_EACH_PTR, DO_FOR_EACH_REVERSE. > > It's not for the number of macros defined. > It's about the code size & the run-time effect on the iteration of > the other lists. It seems simple enough but it would add two load-test-branch Because the rm is near the nr, which get visit every loop iteration. From cache line point of view, the ptrlist->rm will not cause extra cache load. The second test and load will not matter for most of the ptrlist any way. I am willing to bet even my test-ptrlist benchmark will not see much a difference. > to every such macro invocation and these macros are already quite heavy. The flip side is if some one forget to check that, it will be very bad. How do you make sure you add the check to every single one of the loop iterator? I can take patch as it is. It is only a suggestion. No deal breaker. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 11:14 PM, Christopher Li <sparse@chrisli.org> wrote: > On Fri, Aug 4, 2017 at 5:03 PM, Luc Van Oostenryck wrote: >>> I am not sure we are on the same page. Only 2 macro need to be updated: >>> DO_FOR_EACH_PTR, DO_FOR_EACH_REVERSE. >> >> It's not for the number of macros defined. >> It's about the code size & the run-time effect on the iteration of >> the other lists. It seems simple enough but it would add two load-test-branch > > Because the rm is near the nr, which get visit every loop iteration. > From cache line point of view, the ptrlist->rm will not cause extra cache load. > The second test and load will not matter for most of the ptrlist any way. Yes, we agree. > I am willing to bet even my test-ptrlist benchmark will not see much a > difference. It will depend on how micro the benchmark is but indeed on real code, most probably it won't make any measurable difference. >> to every such macro invocation and these macros are already quite heavy. > > The flip side is if some one forget to check that, it will be very > bad. How do you > make sure you add the check to every single one of the loop iterator? Oh, I agree on this one but the same argument when pushed further can lead to absurd situations. We're not there though. > I can take patch as it is. It is only a suggestion. No deal breaker. Frankly, I don't know. I never liked to have to add these checks a bit everywhere, that's why I called it 'not very pretty'. My inclination would be to create a variant FOR_EACH_PTR_NULL(), this can be done without duplicating the real macro DO_... but then you can as easily forgot to call this variant that you could forgot to add the test locally (adding some typing could help here though). On the other hand, adding these tests to all list iteration code when not needed looks bad. OK, since the run-time impact will be low enough let's choose the solution with the smallest (source) code impact. New version on the way. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/flow.c b/flow.c index 6cac21b24..1dbfd431e 100644 --- a/flow.c +++ b/flow.c @@ -283,6 +283,8 @@ void convert_instruction_target(struct instruction *insn, pseudo_t src) if (target == src) return; FOR_EACH_PTR(target->users, pu) { + if (!pu) + continue; if (*pu->userp != VOID) { assert(*pu->userp == target); *pu->userp = src; @@ -675,8 +677,10 @@ static void simplify_one_symbol(struct entrypoint *ep, struct symbol *sym) complex = 0; FOR_EACH_PTR(pseudo->users, pu) { /* We know that the symbol-pseudo use is the "src" in the instruction */ - struct instruction *insn = pu->insn; - + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; switch (insn->opcode) { case OP_STORE: stores++; @@ -715,7 +719,10 @@ static void simplify_one_symbol(struct entrypoint *ep, struct symbol *sym) src = def->target; FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_LOAD) { check_access(insn); convert_load_instruction(insn, src); @@ -731,7 +738,10 @@ complex_def: external_visibility: all = 1; FOR_EACH_PTR_REVERSE(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_LOAD) all &= find_dominating_stores(pseudo, insn, ++bb_generation, !mod); } END_FOR_EACH_PTR_REVERSE(pu); @@ -739,7 +749,10 @@ external_visibility: /* If we converted all the loads, remove the stores. They are dead */ if (all && !mod) { FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_STORE) kill_store(insn); } END_FOR_EACH_PTR(pu); @@ -749,7 +762,10 @@ external_visibility: * of them.. */ FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_STORE) kill_dominated_stores(pseudo, insn, ++bb_generation, insn->bb, !mod, 0); } END_FOR_EACH_PTR(pu); diff --git a/linearize.c b/linearize.c index ba76397ea..0933e935f 100644 --- a/linearize.c +++ b/linearize.c @@ -542,6 +542,8 @@ static void show_symbol_usage(pseudo_t pseudo) if (pseudo) { FOR_EACH_PTR(pseudo->users, pu) { + if (!pu) + continue; printf("\t%s\n", show_instruction(pu->insn)); } END_FOR_EACH_PTR(pu); } diff --git a/memops.c b/memops.c index aeacdf566..ce5aecbe8 100644 --- a/memops.c +++ b/memops.c @@ -66,7 +66,10 @@ static int address_taken(pseudo_t pseudo) { struct pseudo_user *pu; FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->bb && (insn->opcode != OP_LOAD && insn->opcode != OP_STORE)) return 1; } END_FOR_EACH_PTR(pu); diff --git a/simplify.c b/simplify.c index 03ff9c942..0569007a1 100644 --- a/simplify.c +++ b/simplify.c @@ -166,20 +166,24 @@ static int clean_up_phi(struct instruction *insn) return if_convert_phi(insn); } + static int delete_pseudo_user_list_entry(struct pseudo_user_list **list, pseudo_t *entry, int count) { struct pseudo_user *pu; FOR_EACH_PTR(*list, pu) { + if (!pu) + continue; if (pu->userp == entry) { - DELETE_CURRENT_PTR(pu); + MARK_CURRENT_DELETED(pu); if (!--count) goto out; } } END_FOR_EACH_PTR(pu); assert(count <= 0); out: - pack_ptr_list((struct ptr_list **)list); + if (ptr_list_size((struct ptr_list *) *list) == 0) + *list = NULL; return count; } @@ -308,6 +312,8 @@ static int dead_insn(struct instruction *insn, pseudo_t *src1, pseudo_t *src2, p { struct pseudo_user *pu; FOR_EACH_PTR(insn->target->users, pu) { + if (!pu) + continue; if (*pu->userp != VOID) return 0; } END_FOR_EACH_PTR(pu); diff --git a/unssa.c b/unssa.c index e7c9154d5..87d0c2c7f 100644 --- a/unssa.c +++ b/unssa.c @@ -58,6 +58,8 @@ static int simplify_phi_node(struct instruction *phi, pseudo_t tmp) // no need to make a copy of this one // -> replace the target pseudo by the tmp FOR_EACH_PTR(target->users, pu) { + if (!pu) + continue; use_pseudo(pu->insn, tmp, pu->userp); } END_FOR_EACH_PTR(pu);
This will fix the (rare) problems with deletion while doing nested ptrlist walking that occurs when doing recursive kill_instruction() - remove_usage() - kill_instruction() Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- flow.c | 28 ++++++++++++++++++++++------ linearize.c | 2 ++ memops.c | 5 ++++- simplify.c | 10 ++++++++-- unssa.c | 2 ++ 5 files changed, 38 insertions(+), 9 deletions(-)