Patchwork [mips] Fix for PR 54619, GCC aborting with -O -mips16

login
register
mail settings
Submitter Steve Ellcey
Date Sept. 19, 2012, 4:13 p.m.
Message ID <6db5176b-8592-4c9e-b43b-4fe183d7995b@EXCHHUB01.MIPS.com>
Download mbox | patch
Permalink /patch/185103/
State New
Headers show

Comments

Steve Ellcey - Sept. 19, 2012, 4:13 p.m.
While building newlib with -O2 -mips16 I ran into a problem with GCC
aborting due to the compiler trying to execute '0 % 0' in
mips16_unextended_reference_p.  The problem was that the code checked
for BLKmode and skipped the modulo operation in that case because
GET_MODE_SIZE (BLKmode) is zero but it didn't check for VOIDmode whose
size is also zero.  Rather then add a check for VOIDmode I changed
the check to 'GET_MODE_SIZE (mode) != 0'.

While looking at the code I also noticed that if offset was zero then
we should return true even if mode is BLKmode or VOIDmode.  Returning
false would not generate bad code or cause problems but returning true
would result in better costing model in mips_address_insns so I made
that change too.

Tested on a mips elf target, OK to checkin?

Steve Ellcey
sellcey@mips.com



2012-09-19  Steve Ellcey  <sellcey@mips.com>

	PR target/54619
	* config/mips/mips.c (mips16_unextended_reference_p): Check for
	zero offset and zero size mode.
Richard Sandiford - Sept. 19, 2012, 5:42 p.m.
"Steve Ellcey " <sellcey@mips.com> writes:
> While building newlib with -O2 -mips16 I ran into a problem with GCC
> aborting due to the compiler trying to execute '0 % 0' in
> mips16_unextended_reference_p.  The problem was that the code checked
> for BLKmode and skipped the modulo operation in that case because
> GET_MODE_SIZE (BLKmode) is zero but it didn't check for VOIDmode whose
> size is also zero.  Rather then add a check for VOIDmode I changed
> the check to 'GET_MODE_SIZE (mode) != 0'.
>
> While looking at the code I also noticed that if offset was zero then
> we should return true even if mode is BLKmode or VOIDmode.  Returning
> false would not generate bad code or cause problems but returning true
> would result in better costing model in mips_address_insns so I made
> that change too.

This came in with the recent change to pass the mode to address_cost.
I hadn't realised when asking for the associated mips_address_cost change
that address_cost sometimes gets passed VOIDmode.

mips_address_insns shouldn't have to deal with VOIDmode.  The quick
hack I'd been using locally is to add:

  if (mode == VOIDmode)
    mode = SImode;

to mips_address_cost, which restores the previous behaviour for this case.
But the documentation says:

  This hook is never called with an invalid address.

Since VOIDmode MEMs aren't valid, I think that should mean it's invalid
to call this hook (and rtlanal.c:address_cost) with VOIDmode.  I never
got time to look at that though.  (The culprit in the case I saw was
tree-ssa-loop-ivopts.c.  Sandra had some improvements in this area,
so maybe they would fix this too.)

Loading or storing BLKmode doesn't map to any instruction, so I don't
think returning true for zero is any better than what we do now.

Richard
Steve Ellcey - Sept. 19, 2012, 6:39 p.m.
On Wed, 2012-09-19 at 18:42 +0100, Richard Sandiford wrote:

>   This hook is never called with an invalid address.
> 
> Since VOIDmode MEMs aren't valid, I think that should mean it's invalid
> to call this hook (and rtlanal.c:address_cost) with VOIDmode.  I never
> got time to look at that though.  (The culprit in the case I saw was
> tree-ssa-loop-ivopts.c.  Sandra had some improvements in this area,
> so maybe they would fix this too.)

Yes, the test case I put in PR 54619 also shows us getting there from
tree-ssa-loop-ivopts.c with VOIDmode.  I will look a bit more at this
when I get a chance.

Steve Ellcey
sellcey@mips.com
Steve Ellcey - Sept. 20, 2012, 4:41 p.m.
On Wed, 2012-09-19 at 18:42 +0100, Richard Sandiford wrote:

> But the documentation says:
> 
>   This hook is never called with an invalid address.
> 
> Since VOIDmode MEMs aren't valid, I think that should mean it's invalid
> to call this hook (and rtlanal.c:address_cost) with VOIDmode.  I never
> got time to look at that though.  (The culprit in the case I saw was
> tree-ssa-loop-ivopts.c.  Sandra had some improvements in this area,
> so maybe they would fix this too.)
> 
> Loading or storing BLKmode doesn't map to any instruction, so I don't
> think returning true for zero is any better than what we do now.
> 
> Richard

It looks like address_cost does get called with invalid addresses though
and I think that happens on other platforms then MIPS, it is just that
MIPS is the only one where the invalid address causes a compile time
failure.  For example, if you compile my test program without -mips16
the compiler does not abort but address_cost is still called with a
VOIDmode MEM.  What do you think about my patch minus the initial check
for offset == 0?  That would avoid the compile time abort.  If we didn't
want to do that it seems like address_cost should include a check for
valid addresses before it calls the hook.  I am sure that would cause
failures on many platforms and get some attention.

Steve Ellcey
sellcey@mips.com
Richard Sandiford - Sept. 20, 2012, 6:18 p.m.
Steve Ellcey <sellcey@mips.com> writes:
> On Wed, 2012-09-19 at 18:42 +0100, Richard Sandiford wrote:
>> But the documentation says:
>> 
>>   This hook is never called with an invalid address.
>> 
>> Since VOIDmode MEMs aren't valid, I think that should mean it's invalid
>> to call this hook (and rtlanal.c:address_cost) with VOIDmode.  I never
>> got time to look at that though.  (The culprit in the case I saw was
>> tree-ssa-loop-ivopts.c.  Sandra had some improvements in this area,
>> so maybe they would fix this too.)
>> 
>> Loading or storing BLKmode doesn't map to any instruction, so I don't
>> think returning true for zero is any better than what we do now.
>> 
>> Richard
>
> It looks like address_cost does get called with invalid addresses though

Right, that's what I meant.  The hook (and address_cost) are being passed
VOIDmode, even though VOIDmode isn't a legitimate mode for a MEM.
And that's a bug IMO.

> and I think that happens on other platforms then MIPS, it is just that
> MIPS is the only one where the invalid address causes a compile time
> failure.  For example, if you compile my test program without -mips16
> the compiler does not abort but address_cost is still called with a
> VOIDmode MEM.  What do you think about my patch minus the initial check
> for offset == 0?  That would avoid the compile time abort.  If we didn't
> want to do that it seems like address_cost should include a check for
> valid addresses before it calls the hook.  I am sure that would cause
> failures on many platforms and get some attention.

address_cost already checks memory_address_addr_space_p, and returns
a large value if it's false.

We could avoid the assert by making memory_address_addr_space_p return
false for VOIDmode, but an assert seems better (which I think is
what you're suggesting).  Returning 1000 for VOIDmode addresses --
as per the former option -- would certainly avoid the assert,
but wouldn't be very helpful.  I think tree-ssa-loop-ivopts is simply
asking for the wrong thing, and needs to be changed.  As I say,
Sandra had some fixes in this area.

Richard

Patch

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 7f9df4c..ecdb811 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -2230,7 +2230,10 @@  static bool
 mips16_unextended_reference_p (enum machine_mode mode, rtx base,
 			       unsigned HOST_WIDE_INT offset)
 {
-  if (mode != BLKmode && offset % GET_MODE_SIZE (mode) == 0)
+  if (offset == 0)
+    return true;
+
+  if (GET_MODE_SIZE (mode) != 0 && offset % GET_MODE_SIZE (mode) == 0)
     {
       if (GET_MODE_SIZE (mode) == 4 && base == stack_pointer_rtx)
 	return offset < 256U * GET_MODE_SIZE (mode);