Re: PATCH: RFA add case to purge addressof

[Roger Sayle <roger at eyesopen dot com>]

Re: http://gcc.gnu.org/ml/gcc-patches/2003-10/msg02446.html

On Tue, 28 Oct 2003, Eric Christopher wrote:
> Just wanted to let you know that I believe the patch to be correct even
> though I thinko'd the insn in the mail just in case you thought I'd
> dropped it.

Hi Eric,

Personally, I'm probably over cautious reviewing patches during stage3,
and I was hoping to leave this to someone more knowledgable than me...

However, if you could provide a new testcase for the testsuite and perhaps
mention the target triple you claim this is failing on, it should be
possible to confirm your patch is indeed a bug-fix and therefore suitable
for stage3.  Unlike some reviewers, I won't demand that you file a PR,
but you should atleast provide the equivlent information (accurately :).

The patch itself looks reasonable, but is there any reason why the new
"GET_MODE_SIZE (GET_MODE (x)) < GET_MODE_SIZE (GET_MODE (sub))" couldn't
be added to the disjuntion in the previous if statement.  It looks like
you're just duplicating "*loc = gen_rtx_SUBREG (GET_MODE (x), sub, 0);
return true;".

Finally, in stage3, I'd feel happier if we didn't add the new call to
"abort()".  I know this is catching potential problems, but hypothetically
there may exist source codes where the addressof is erroneously left in
the REG_EQUAL note, but the note is never used by later passes.  These
programs would compile fine with mainline, but ICE once your patch is
applied.  Perhaps we can add this new abort to 3.5 once we unfreeze.


And you know the routine; list the target(s) you bootstrapped this patch
on, and whether there were any new testsuite failures during regression
tests.  I shouldn't need to point a GCC maintainer to contribute.html...

I hope this helps.

Roger
--