* RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
@ 2014-10-06 8:52 Joern Rennecke
2014-10-06 18:58 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Joern Rennecke @ 2014-10-06 8:52 UTC (permalink / raw)
To: GCC Patches, Jeff Law
[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]
Investigating an ICE while trying to compile libgcc2.c:__udivmoddi4 for
a new avr variant with different register set/allocation order, I found
replace_reg_with_saved_mem falling over its own nonsense. The instruction:
(debug_insn 97 96 98 2 (var_location:SI __x2 (mult:SI (lshiftrt:SI
(reg/v:SI 18 r18 [orig:58 q0 ] [58])
(const_int 16 [0x10]))
(reg/v:SI 61 [ __vl ])))
../../gcc/gcc/testsuite/gcc.target/avr/tiny-caller-save.c:67 -1
(nil))
would be transformed into:
(debug_insn 97 96 98 2 (var_location:SI __x2 (mult:SI (lshiftrt:SI (concatn:SI [
(reg:SI 18 r18)
(reg:SI 19 r19)
(mem/c:QI (plus:HI (reg/f:HI 28 r28)
(const_int 43 [0x2b])) [6 S1 A8])
(mem/c:QI (plus:HI (reg/f:HI 28 r28)
(const_int 44 [0x2c])) [6 S1 A8])
])
(const_int 16 [0x10]))
(reg/v:SI 61 [ __vl ])))
../../gcc/gcc/testsuite/gcc.target/avr/tiny-caller-save.c:67 -1
Note that r18 and r19 inside the concatn are supposed to be single hard
registers, but as the word size of this processor is 8 bit, SImode
extends actually
over four hard registers. save_mode is SImode because four registers can be
saved/restored at once.
The attached patch fixes the failure by using word_mode if smode would cover
multiple hard registers.
bootstrapped & regtested on i686-pc-linux-gnu.
OK to apply?
[-- Attachment #2: caller-save-patch --]
[-- Type: application/octet-stream, Size: 749 bytes --]
2014-08-28 Joern Rennecke <joern.rennecke@embecosm.com>
* caller-save.c (replace_reg_with_saved_mem): If saved_mode covers
multiple hard registers, use word_mode.
diff --git a/gcc/caller-save.c b/gcc/caller-save.c
index 03d22a0..ce900c0 100644
--- a/gcc/caller-save.c
+++ b/gcc/caller-save.c
@@ -1152,9 +1152,11 @@ replace_reg_with_saved_mem (rtx *loc,
}
else
{
- gcc_assert (save_mode[regno] != VOIDmode);
- XVECEXP (mem, 0, i) = gen_rtx_REG (save_mode [regno],
- regno + i);
+ enum machine_mode smode = save_mode[regno];
+ gcc_assert (smode != VOIDmode);
+ if (hard_regno_nregs [regno][smode] > 1)
+ smode = word_mode;
+ XVECEXP (mem, 0, i) = gen_rtx_REG (smode, regno + i);
}
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-06 8:52 RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem Joern Rennecke
@ 2014-10-06 18:58 ` Jeff Law
2014-10-07 2:57 ` Joern Rennecke
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2014-10-06 18:58 UTC (permalink / raw)
To: Joern Rennecke, GCC Patches
On 10/06/14 02:51, Joern Rennecke wrote:
> Investigating an ICE while trying to compile libgcc2.c:__udivmoddi4 for
> a new avr variant with different register set/allocation order, I found
> replace_reg_with_saved_mem falling over its own nonsense. The instruction:
>
> (debug_insn 97 96 98 2 (var_location:SI __x2 (mult:SI (lshiftrt:SI
> (reg/v:SI 18 r18 [orig:58 q0 ] [58])
> (const_int 16 [0x10]))
> (reg/v:SI 61 [ __vl ])))
> ../../gcc/gcc/testsuite/gcc.target/avr/tiny-caller-save.c:67 -1
> (nil))
>
> would be transformed into:
>
> (debug_insn 97 96 98 2 (var_location:SI __x2 (mult:SI (lshiftrt:SI (concatn:SI [
> (reg:SI 18 r18)
> (reg:SI 19 r19)
> (mem/c:QI (plus:HI (reg/f:HI 28 r28)
> (const_int 43 [0x2b])) [6 S1 A8])
> (mem/c:QI (plus:HI (reg/f:HI 28 r28)
> (const_int 44 [0x2c])) [6 S1 A8])
> ])
> (const_int 16 [0x10]))
> (reg/v:SI 61 [ __vl ])))
> ../../gcc/gcc/testsuite/gcc.target/avr/tiny-caller-save.c:67 -1
>
> Note that r18 and r19 inside the concatn are supposed to be single hard
> registers, but as the word size of this processor is 8 bit, SImode
> extends actually
> over four hard registers. save_mode is SImode because four registers can be
> saved/restored at once.
>
> The attached patch fixes the failure by using word_mode if smode would cover
> multiple hard registers.
> bootstrapped & regtested on i686-pc-linux-gnu.
>
> OK to apply?
What makes word_mode special here? ie, why is special casing for
word_mode the right thing to do?
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-06 18:58 ` Jeff Law
@ 2014-10-07 2:57 ` Joern Rennecke
2014-10-07 17:38 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Joern Rennecke @ 2014-10-07 2:57 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On 6 October 2014 19:58, Jeff Law <law@redhat.com> wrote:
> What makes word_mode special here? ie, why is special casing for word_mode
> the right thing to do?
The patch does not special-case word mode. The if condition tests if
smode would
cover multiple hard registers.
If that would be the case, smode is replaced with word_mode.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-07 2:57 ` Joern Rennecke
@ 2014-10-07 17:38 ` Jeff Law
2014-10-07 21:56 ` Joern Rennecke
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2014-10-07 17:38 UTC (permalink / raw)
To: Joern Rennecke; +Cc: GCC Patches
On 10/06/14 20:57, Joern Rennecke wrote:
> On 6 October 2014 19:58, Jeff Law <law@redhat.com> wrote:
>> What makes word_mode special here? ie, why is special casing for word_mode
>> the right thing to do?
>
> The patch does not special-case word mode. The if condition tests if
> smode would
> cover multiple hard registers.
> If that would be the case, smode is replaced with word_mode.
SO I'll ask another way. Why do you want to change smode to word_mode?
Jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-07 17:38 ` Jeff Law
@ 2014-10-07 21:56 ` Joern Rennecke
2014-10-10 20:23 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Joern Rennecke @ 2014-10-07 21:56 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On 7 October 2014 18:38, Jeff Law <law@redhat.com> wrote:
> On 10/06/14 20:57, Joern Rennecke wrote:
>>
>> On 6 October 2014 19:58, Jeff Law <law@redhat.com> wrote:
>>>
>>> What makes word_mode special here? ie, why is special casing for
>>> word_mode
>>> the right thing to do?
>>
>>
>> The patch does not special-case word mode. The if condition tests if
>> smode would
>> cover multiple hard registers.
>> If that would be the case, smode is replaced with word_mode.
>
> SO I'll ask another way. Why do you want to change smode to word_mode?
Because SImode covers four hard registers, wheras the intention is to
have a single
one.
(concatn:SI [
(reg:SI 18 r18)
(reg:SI 19 r19)
(mem/c:QI (plus:HI (reg/f:HI 28 r28)
(const_int 43 [0x2b])) [6 S1 A8])
(mem/c:QI (plus:HI (reg/f:HI 28 r28)
(const_int 44 [0x2c])) [6 S1 A8])
])
(see original post) is invalid RTL, and thuis the cause of the later ICE.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-07 21:56 ` Joern Rennecke
@ 2014-10-10 20:23 ` Jeff Law
2014-10-11 10:27 ` Joern Rennecke
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2014-10-10 20:23 UTC (permalink / raw)
To: Joern Rennecke; +Cc: GCC Patches
On 10/07/14 15:56, Joern Rennecke wrote:
> On 7 October 2014 18:38, Jeff Law <law@redhat.com> wrote:
>> On 10/06/14 20:57, Joern Rennecke wrote:
>>>
>>> On 6 October 2014 19:58, Jeff Law <law@redhat.com> wrote:
>>>>
>>>> What makes word_mode special here? ie, why is special casing for
>>>> word_mode
>>>> the right thing to do?
>>>
>>>
>>> The patch does not special-case word mode. The if condition tests if
>>> smode would
>>> cover multiple hard registers.
>>> If that would be the case, smode is replaced with word_mode.
>>
>> SO I'll ask another way. Why do you want to change smode to word_mode?
>
> Because SImode covers four hard registers, wheras the intention is to
> have a single
> one.
>
> (concatn:SI [
> (reg:SI 18 r18)
> (reg:SI 19 r19)
> (mem/c:QI (plus:HI (reg/f:HI 28 r28)
> (const_int 43 [0x2b])) [6 S1 A8])
> (mem/c:QI (plus:HI (reg/f:HI 28 r28)
> (const_int 44 [0x2c])) [6 S1 A8])
> ])
>
> (see original post) is invalid RTL, and thuis the cause of the later ICE.
Thanks. I see the problem now.
Dont we have an ever deeper problem here, namely that registers don't
necessarily have to be word_mode sized objects. More likely than not in
that case the register is going to be larger than word_mode, but I could
also envision a target where there's registers smaller than word_mode.
This stuff seems rather broken/fragile in that world and using word_mode
isn't necessarily any better than what we're doing today.
ISTM it would be better to find the mode of the same class that
corresponds to GET_MODE_SIZE (mode) / nregs. In your case that's
obviously QImode :-)
I think that still breaks in various ways, but probably significantly
less so than what we're doing now or with your current patch.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-10 20:23 ` Jeff Law
@ 2014-10-11 10:27 ` Joern Rennecke
2014-10-13 19:45 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Joern Rennecke @ 2014-10-11 10:27 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
On 10 October 2014 21:13, Jeff Law <law@redhat.com> wrote:
...
> ISTM it would be better to find the mode of the same class that corresponds
> to GET_MODE_SIZE (mode) / nregs. In your case that's obviously QImode :-)
Like this?
Or did you mean to remove the save_mode[regno] use altogether? I can
think of arguments for or against, but got no
concrete examples for either.
[-- Attachment #2: caller-save.diff --]
[-- Type: text/plain, Size: 852 bytes --]
2014-10-11 Joern Rennecke <joern.rennecke@embecosm.com>
Jeff Law <law@redhat.com>
* caller-save.c (replace_reg_with_saved_mem): If saved_mode covers
multiple hard registers, use word_mode.
diff --git a/gcc/caller-save.c b/gcc/caller-save.c
index e28facb..31b1a36 100644
--- a/gcc/caller-save.c
+++ b/gcc/caller-save.c
@@ -1158,9 +1158,12 @@ replace_reg_with_saved_mem (rtx *loc,
}
else
{
- gcc_assert (save_mode[regno] != VOIDmode);
- XVECEXP (mem, 0, i) = gen_rtx_REG (save_mode [regno],
- regno + i);
+ enum machine_mode smode = save_mode[regno];
+ gcc_assert (smode != VOIDmode);
+ if (hard_regno_nregs [regno][smode] > 1)
+ smode = mode_for_size (GET_MODE_SIZE (mode) / nregs,
+ GET_MODE_CLASS (mode), 0);
+ XVECEXP (mem, 0, i) = gen_rtx_REG (smode, regno + i);
}
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-11 10:27 ` Joern Rennecke
@ 2014-10-13 19:45 ` Jeff Law
2014-10-14 0:44 ` Joern Rennecke
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2014-10-13 19:45 UTC (permalink / raw)
To: Joern Rennecke; +Cc: GCC Patches
On 10/11/14 03:32, Joern Rennecke wrote:
> On 10 October 2014 21:13, Jeff Law <law@redhat.com> wrote:
> ...
>> ISTM it would be better to find the mode of the same class that corresponds
>> to GET_MODE_SIZE (mode) / nregs. In your case that's obviously QImode :-)
>
> Like this?
> Or did you mean to remove the save_mode[regno] use altogether? I can
> think of arguments for or against, but got no
> concrete examples for either.
Yea, that's basically what I was thinking. I probably wouldn't have
bothered with the if (hard_regno ...) check, but I can see why you might
want that added measure of safety before slamming in a new mode.
I think you want "smode" in the mode_for_size call rather than "mode",
right (both instances)?
If that fixes your your problem and passes the usual bootstrap and
regression test, then it's OK with me.
I can see Richard S. getting in here one day and saying, umm, this all
needs further refinement, but at least this hunk of code is in better shape.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-13 19:45 ` Jeff Law
@ 2014-10-14 0:44 ` Joern Rennecke
2014-10-14 19:45 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: Joern Rennecke @ 2014-10-14 0:44 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On 13 October 2014 20:43, Jeff Law <law@redhat.com> wrote:
...
> I think you want "smode" in the mode_for_size call rather than "mode", right
> (both instances)?
No, nregs is the number of hard registers of regno in "mode". Hence
we must use the
size of "mode".
How to choose the mode class is not so clear-cut. For the code that
went wrong with the
old code, mode and smode are both of MODE_INT.
To get some case where there's a difference, I was thinking of an
architecture that
has partial integer mode registers that can be grouped together as
integral integer mode
registers (e.g. one reg is HImode or PSImode, save_mode would be PSImode,
two regs form SImode). In that case, you'd want something so that you can piece
together "mode", i.e. either GET_MODE_CLASS (mode) or MODE_INT
(which happen to be again the same), but not GET_MODE_CLASS(smode), which would
be MODE_PARTIAL_INT .
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem
2014-10-14 0:44 ` Joern Rennecke
@ 2014-10-14 19:45 ` Jeff Law
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Law @ 2014-10-14 19:45 UTC (permalink / raw)
To: Joern Rennecke; +Cc: GCC Patches
On 10/13/14 18:16, Joern Rennecke wrote:
> On 13 October 2014 20:43, Jeff Law <law@redhat.com> wrote: ...
>> I think you want "smode" in the mode_for_size call rather than
>> "mode", right (both instances)?
>
> No, nregs is the number of hard registers of regno in "mode". Hence
> we must use the size of "mode".
OK. My bad.
> To get some case where there's a difference, I was thinking of an
> architecture that has partial integer mode registers that can be
> grouped together as integral integer mode registers (e.g. one reg is
> HImode or PSImode, save_mode would be PSImode, two regs form SImode).
> In that case, you'd want something so that you can piece together
> "mode", i.e. either GET_MODE_CLASS (mode) or MODE_INT (which happen
> to be again the same), but not GET_MODE_CLASS(smode), which would be
> MODE_PARTIAL_INT
You're right. We definitely don't want MODE_PARTIAL_INT here.
So if your patch resolves your issue, passes the usual
bootstrap/regression test, then let's go with it.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-10-14 19:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-06 8:52 RFA: fix mode confusion in caller-save.c:replace_reg_with_saved_mem Joern Rennecke
2014-10-06 18:58 ` Jeff Law
2014-10-07 2:57 ` Joern Rennecke
2014-10-07 17:38 ` Jeff Law
2014-10-07 21:56 ` Joern Rennecke
2014-10-10 20:23 ` Jeff Law
2014-10-11 10:27 ` Joern Rennecke
2014-10-13 19:45 ` Jeff Law
2014-10-14 0:44 ` Joern Rennecke
2014-10-14 19:45 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).