On 03/01/2016 10:08 AM, Bin.Cheng wrote:
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.
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
