Re: PATCH: RFA add case to purge addressof

[Eric Christopher <echristo at redhat dot com>]

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

Heh. I know how that feels...

> 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 :).
> 

Right. The target is frv-elf. Unfortunately I don't know if I can
provide the testcase. It looks suspiciously like a commercial testsuite
testcase and, of course, I can't put that in. :(

> 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;".
> 

No reason at all.

> 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.
> 

OK. I wasn't particularly attached to the abort. However, I can't
believe that the addressof, if left would not cause anything but an
abort later on. Especially considering the comment above ("... we must
replace here...").

> 
> 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...

Yup. I suck. Sorry.

Bootstrapped on x86-linux, regtested on mipsisa64-elf and frv-elf. All
success, no new failures.

-eric

-- 
Eric Christopher <echristo@redhat.com>