Re: [PATCH PR69052]Check if loop inv can be propagated into mem ref with additional addr expr canonicalization

[Jeff Law <law@redhat.com>]

On 03/01/2016 10:08 AM, Bin.Cheng wrote:
> On Thu, Feb 25, 2016 at 8:47 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> On Thu, Feb 25, 2016 at 6:39 AM, Jeff Law <law@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.
Sorry, things are just busy here.

I'm reasonably comfortable with both as long as we're not diving inside 
the subexpressions in the ASHIFT/MULT case.  If you'd prefer the 
FOR_EACH_SUBRTX variant, that's OK with me.

jeff