On Thu, Feb 25, 2016 at 6:39 AM, Jeff Law <l...@redhat.com> wrote: > On 02/22/2016 02:22 AM, Bin.Cheng wrote: > >>> My only question is why didn't you use FOR_EACH_SUBRTX_VRA from >>> rtl-iter.h >>> to walk the RTX expressions in collect_address_parts and >>> canonicalize_address_mult? >> >> Hi Jeff, >> Nothing special, just I haven't used this before, also >> canonicalize_address_mult is alphabetically copied from fwprop.c. One >> question is when rewriting SHIFT to MULT, we need to modify rtl >> expression in place, does FOR_EACH_SUBRTX iterator support this? If >> yes, what is the behavior for modified sub-expression? > > Hmm. The question of semantics when we change the underlying > sub-expressions is an interesting one. > > While I think we're OK in practice using the iterators, I think that's more > of an accident than by design -- if MULT/ASHIFT had a different underlying > RTL structure then I'm much less sure using the iterators would be safe. Hi Jeff, Yes, I thought about this too and finally decided to skip sub rtxes in modified MULT/ASHIFT expressions. I think this will make the patch with FOR_EACH_SUBRTX iterator safe. Although it doesn't dive into MULT/ASHIFT while the original one does, I don't think there is such case in practice, after all we can't handle such complicated address expression either. > > Let's go with your original patch that didn't use the iterators. Sorry for > making you do the additional work/testing to build the iterator version. Not even a problem. > But after pondering the issue you raised, I think your original patch is > safer. So in conclusion, I think both versions should be safe, the iterator one is definitely cleaner. It is your call which version I should apply.
Thanks, bin > > > jeff
