On Thu, Feb 25, 2016 at 8:47 AM, Bin.Cheng <amker.ch...@gmail.com> wrote: > 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. Hi Jeff, I tend to apply the new patch with FOR_EACH_SUBRTX because it's clearer. Does it work for you? Of course if you do think it's not that safe I will fall back to the old one.
Thanks, bin
