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