Use of FLAGS_REGNUM clashes with generates insn

Use of FLAGS_REGNUM clashes with generates insn

[Paulo J. Matos]

Hi,

After the discussion about the use of CCmode in:
http://gcc.gnu.org/ml/gcc/2011-07/msg00303.html

I am trying to ditch support for the only cc0 attr and add support for 
CC_REG.

There are two issues that are making the situation more complicated, 
both of similar nature.

My addition instruction sets all the flags. So I have:
(define_insn "addqi3"
   [(set (match_operand:QI 0 "nonimmediate_operand" "=c")
         (plus:QI (match_operand:QI 1 "nonmemory_operand" "%0")
                  (match_operand:QI 2 "general_operand" "cwmi")))
    (clobber (reg:CC RCC))]
   ""
   "add\\t%0,%f2")

(define_insn "*addqi3_flags"
   [(set (match_operand:QI 0 "nonimmediate_operand" "=c")
         (plus:QI (match_operand:QI 1 "nonmemory_operand" "%0")
                  (match_operand:QI 2 "general_operand" "cwmi")))
    (set (reg RCC)
         (compare (plus:QI (match_dup 1) (match_dup 2))
                  (const_int 0)))]
   "reload_completed && xap_match_ccmode(insn, CCmode)"
   "add\\t%0,%f2")

There's however a problem with this. GCC during reload, after register 
elimination (eliminating argument pointer for an offset from the stack 
pointer tries to output the instruction):
(set (reg ...) (plus:QI (reg ...) (const_int ...)))

However, it spills and fails because no rule matches this expression 
(it's missing the clobber). I fixed this with:

(define_insn_and_split "addqi3_noclobber"
   [(set (match_operand:QI 0 "register_operand" "=c")
         (plus:QI (match_operand:QI 1 "register_operand")
                  (match_operand:QI 2 "immediate_operand")))]
   "reload_in_progress"
   "#"
   "reload_completed"
   [(set (match_dup 0) (match_dup 1))
    (parallel [(set (match_dup 0) (plus:QI (match_dup 0) (match_dup 2)))
               (clobber (reg:CC RCC))])])

And it works. However, the more complex issue comes with register moves. 
A register move which ends up as a load or store, sets flags N, Z.

I have an expand movqi which expand to a move with the clobber like so:
(define_expand "movqi"
   [(parallel [(set (match_operand 0 "nonimmediate_operand")
                    (match_operand 1 "general_operand"))
               (clobber (reg:CC RCC))])]
   ""
{
    /* One of the ops has to be in a register.  */
    if (!register_operand(operands[0], QImode)
        && ! (register_operand(operands[1], QImode) || const0_rtx == 
operands[1]))
        operands[1] = copy_to_mode_reg(QImode, operands[1]);
})

And all my (define_insn "*mov..." are tagged with a (clobber (reg:CC 
RCC)). This generates all kinds of trouble since GCC generates moves 
internally without the clobber that fail to match.

I tried the same trick as above:

(define_insn_and_split "*movqi_noclobber"
   [(set (match_operand:QI 0 "nonimmediate_operand")
         (match_operand:QI 1 "general_operand"))]
   "!reload_completed"
   "#"
   ""
   [(parallel [(set (match_dup 0) (match_dup 1))
               (clobber (reg:CC RCC))])])

This doesn't fix the problem. It actually brings an internal compiler 
error. I am definitely not doing this the right way.

Any suggestions on how to correctly handle these?

Cheers,
-- 
PMatos

Re: Use of FLAGS_REGNUM clashes with generates insn

[Joern Rennecke]

Quoting "Paulo J. Matos" <paulo@matos-sorge.com>:

> My addition instruction sets all the flags. So I have:

This is annoying, but can be handled.  Been there, done that.
dse.c needs a small patch, which I intend to submit sometime in the future.

> And all my (define_insn "*mov..." are tagged with a (clobber (reg:CC
> RCC)). This generates all kinds of trouble since GCC generates moves
> internally without the clobber that fail to match.

I don't think that can be overcome without cc0.  Unless you want to
hide your flags register altogether.

Re: Use of FLAGS_REGNUM clashes with generates insn

[Paulo J. Matos]

On 23/09/11 08:21, Joern Rennecke wrote:
> Quoting "Paulo J. Matos" <paulo@matos-sorge.com>:
>
>> My addition instruction sets all the flags. So I have:
>
> This is annoying, but can be handled. Been there, done that.
> dse.c needs a small patch, which I intend to submit sometime in the future.
>

Ok. Actually I was quite happy with my solution too which avoided having 
to change the core. However, it was not heavily tested.

>> And all my (define_insn "*mov..." are tagged with a (clobber (reg:CC
>> RCC)). This generates all kinds of trouble since GCC generates moves
>> internally without the clobber that fail to match.
>
> I don't think that can be overcome without cc0. Unless you want to
> hide your flags register altogether.
>

That's seriously annoying. The idea was to ditch cc0 and explicitly 
represent CC in a register to perform optimizations like splitting add 
and addc for a double word addition. If by hiding my register flags 
means going back to cc0, then it seems that the only way to go unless I 
get it to work somehow. If you having anything else in mind to get it to 
work let me know.

What I currently have in mind is to have a backend macro listing all the 
move for which a move clobber CC_REG, then whenever GCC generates a 
move, it queries the macro to know if the move requires clobbering and 
emits the clobber if required. However, I am unsure how deep the rabbit 
hole goes.

-- 
PMatos

Re: Use of FLAGS_REGNUM clashes with generates insn

[amylaar]

Quoting "Paulo J. Matos" <paulo@matos-sorge.com>:

> That's seriously annoying. The idea was to ditch cc0 and explicitly
> represent CC in a register to perform optimizations like splitting add
> and addc for a double word addition. If by hiding my register flags
> means going back to cc0, then it seems that the only way to go unless I
> get it to work somehow. If you having anything else in mind to get it
> to work let me know.

Hiding the flags register would mean it is not represented in the rtl at
all.  You can have combined compare-branch instructions.
Of course, going that route would mean that the model you present to
GCC is even further from the hardware than one that uses cc0.

> What I currently have in mind is to have a backend macro listing all
> the move for which a move clobber CC_REG, then whenever GCC generates a
> move, it queries the macro to know if the move requires clobbering and
> emits the clobber if required. However, I am unsure how deep the rabbit
> hole goes.

Oh, so you do have variants that can do without the clobber.
If you can make all the reloads without introducing explicit flag
clobbers, that it should work.
But you can't just pull a flag clobber out of thin air.  You should
have some way to generate valid code when the flags register is
unavailable / must be saved.  Then you can use peephole2 to add
flag clobbers where the flags register is available.

Or you can use machine_dependent_reorg or another machine-specific pass
inserted with the pass manager to rewrite clobber-free instructions into
ones that have a hardware equivalent; but you must make sure that your
data flow remains sound in the process.

Re: Use of FLAGS_REGNUM clashes with generates insn

[Paulo J. Matos]

On Fri, 23 Sep 2011 09:30:48 -0400, amylaar wrote:

> Hiding the flags register would mean it is not represented in the rtl at
> all.  You can have combined compare-branch instructions. Of course,
> going that route would mean that the model you present to GCC is even
> further from the hardware than one that uses cc0.
>

Got it! That seems that it would go against the whole point of replacing 
cc0 for CC_REGNUM in my specific case. Oh well...
 
>> What I currently have in mind is to have a backend macro listing all
>> the move for which a move clobber CC_REG, then whenever GCC generates a
>> move, it queries the macro to know if the move requires clobbering and
>> emits the clobber if required. However, I am unsure how deep the rabbit
>> hole goes.
> 
> Oh, so you do have variants that can do without the clobber. 

Actually I don't... My explanation was supposed to be referring to a 
general solution. In my case, the macro would list all moves since all 
moves clobber CC.

> If you can
> make all the reloads without introducing explicit flag clobbers, that it
> should work.

Unfortunately I can't.

> But you can't just pull a flag clobber out of thin air.  

Understood.

> You should have
> some way to generate valid code when the flags register is unavailable /
> must be saved.  Then you can use peephole2 to add flag clobbers where
> the flags register is available.
> 
> Or you can use machine_dependent_reorg or another machine-specific pass
> inserted with the pass manager to rewrite clobber-free instructions into
> ones that have a hardware equivalent; but you must make sure that your
> data flow remains sound in the process.

I think your last suggestion of having a pass to rewrite the clobber free 
instructions into one with a hardware equivalent seems the one to go for 
me. 

Thanks for the suggestions,

-- 
PMatos

Re: Use of FLAGS_REGNUM clashes with generates insn

[Hans-Peter Nilsson]

On Fri, 23 Sep 2011, Joern Rennecke wrote:

> Quoting "Paulo J. Matos" <paulo@matos-sorge.com>:
>
> > My addition instruction sets all the flags. So I have:
>
> This is annoying, but can be handled.  Been there, done that.
> dse.c needs a small patch, which I intend to submit sometime in the future.

Could you be persuaded to send it to the list as-is, right now?

Be sure to mark it work-in-progress, or someone might feel
compelled to review it. :)

brgds, H-P

Re: Use of FLAGS_REGNUM clashes with generates insn

[amylaar]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

Quoting Hans-Peter Nilsson <hp@bitrange.com>:

> On Fri, 23 Sep 2011, Joern Rennecke wrote:
>
>> Quoting "Paulo J. Matos" <paulo@matos-sorge.com>:
>>
>> > My addition instruction sets all the flags. So I have:
>>
>> This is annoying, but can be handled.  Been there, done that.
>> dse.c needs a small patch, which I intend to submit sometime in the future.
>
> Could you be persuaded to send it to the list as-is, right now?
>
> Be sure to mark it work-in-progress, or someone might feel
> compelled to review it. :)

Attached.

The issue with this patch is that we'll have to check if gen_add3_insn might
fail on any target, and also we have to identify on which targets it will
create an insn that clobbers potentially live hard registers
(like a flags register), and make the substitution fail in this case.
I.e. if in doubt, keep the dead store with the auto-increment.
But not fail for a target that knows what it clobbers in a
reload_in_progress / reload_completed add.

For Epiphany, the add will be expanded with a clobber of a fake hard register,
and this pattern is subject to various peephole2 patterns which usually
find a low-cost substitute.
The point of clobbering a fake hard register is to avoid having passes like
combine creating the pattern with the unresolved flag clobber problem.
The unoptimized add expands into a sequentially issued five-instruction
in order to save/restore the flags to a temp register, which in turn is
saved on the stack.
There are peephole2 patterns to use a constant directly rather than loading
it into a temp register, to clobber the flags register if it is free,
to move the add before an immediately preceding flag-setting instruction,
to find a possibility for a dummy post-modify load from / store to the stack
(for calculating a stack/frame based address), and to scavenge a temp
register to save the flags into without needing to save the temp register.

[-- Attachment #2: tmp --]
[-- Type: text/plain, Size: 899 bytes --]

2011-09-18  Joern Rennecke <joern.rennecke@embecosm.com>

	* dse.c (emit_inc_dec_insn_before): Use gen_add3_insn / gen_move_insn.

Index: dse.c
===================================================================
--- dse.c	(revision 2071)
+++ dse.c	(revision 2072)
@@ -835,15 +835,15 @@ emit_inc_dec_insn_before (rtx mem ATTRIB
 			  rtx op ATTRIBUTE_UNUSED,
 			  rtx dest, rtx src, rtx srcoff, void *arg)
 {
-  rtx insn = (rtx)arg;
-
-  if (srcoff)
-    src = gen_rtx_PLUS (GET_MODE (src), src, srcoff);
+  rtx insn = (rtx) arg, new_insn;
 
   /* We can reuse all operands without copying, because we are about
      to delete the insn that contained it.  */
-
-  emit_insn_before (gen_rtx_SET (VOIDmode, dest, src), insn);
+  if (srcoff)
+    new_insn = gen_add3_insn (dest, src, srcoff);
+  else
+    new_insn = gen_move_insn (dest, src);
+  emit_insn_before (new_insn, insn);
 
   return -1;
 }

Impact assessment of dse.c:emit_inc_dec_insn_before (Was: Re: Use of FLAGS_REGNUM clashes with generates insn)

[Joern Rennecke]

Quoting amylaar@spamcop.net:
> The issue with this patch is that we'll have to check if gen_add3_insn might
> fail on any target, and also we have to identify on which targets it will
> create an insn that clobbers potentially live hard registers
> (like a flags register), and make the substitution fail in this case.
> I.e. if in doubt, keep the dead store with the auto-increment.
> But not fail for a target that knows what it clobbers in a
> reload_in_progress / reload_completed add.

I've gone through the machine description that define one of the
HAVE_P\(RE\|OST\)_.* macros, and checked if the auto-increment addresses
they accept can be replaced with an add, both when using gen_add3_insn,
which goes via optabs and named patterns, and by recognizing the
rtl that dse.c:emit_inc_dec_insn_before generates without any proper checks.

I've added the maintainers of ports that appear to get auto-increments
turned into flag-clobbering adds (excluding cc0) to the Cc: list.

Note that the auto-increment offset range in this summary only states what
the GCC machine description says; for various reasons, the actual hardware
capabilities might be more or less.
"insn pattern" means that the add pattern for Pmode (one of addhi3 / addsi3 /
adddi3 / addpsi3) is a named instruction patterns and is thus used both when
expanding using gen_add3_insn, and to recognize the add.
I have looked at the offset range for any auto-increment use, even though
at times only a smaller range is available for stores; it turned out that
the addition range is not actually a problem for the existing targets.

architecture    Pmode   offset     addP3 src,src,srcoff
with PRE/POST
INC/DEC/MODIFY
arm            SImode  -255..255       expand, recog OK.
arm            SImode (-255..255)*4    expand, recog OK.
avr            HImode -4..4            expand, recog OK (costly & need cc0)
bfin           SImode -8..8            insn pattern, breaks on IREGS, PDImode.
c6x            SImode -8..8            insn pattern, breaks on REG_ILC
cris           SImode  1..8            expand, recog OK.
frv            SImode  reg only        insn pattern, OK.
h8300   HImode/SImode -4..+4           expand, recog OK.
ia64           DImode -256..255        insn pattern, OK.
m32c   HImode/PSImode -8..8/NaN        insn pattern, OK if offset is operand.
m32r           SImode -4/4             insn pattern, OK.
m68k           SImode -8..8            expand, recog OK.
mn10300        SImode any integer      insn pattern, has CC_REG clobber.
                                        recog of naked (set ...  
(plus)) will fail
pa             SImode -8..8            insn pattern, OK.
pa             DImode -8..8            expand, recog OK.
pdp11          HImode -8..8            insn pattern, OK.
rs6000  SImode/DImode -0x8000..0x7fff  expand, recog OK.
rx             SImode -4..4            expand, recog with CC_REG clobber.
                                        recog of naked (set ...  
(plus)) will fail
score          SImode -4..4            expand, recog OK.
sh             SImode -8..8            expand, recog OK.
sh64           DImode N/A
stormy16       HImode any integer      insn pattern, has CARRY_REG clobber.
                 ALL REGISTERS ARE ALLOWED AS ADDRESS REGISTERS,
                 BUT ONLY TWO CAN BE USED IN AN ADD INSN.  Calling gen_addhi3
                 with a non-general register operand after reload will generate
                 an insn that fails to match its constraints.
vax            SImode -8..8            insn pattern, OK.

m32c.c:m32c_legitimate_address_p allows any rubbish inside a PRE_MODIFY,
as long as the address register is the stack pointer.
OTOH, it does not define any HAVE_*_MODIFY_* macro.

score defines HAVE_PRE_MODIFY_DISP and HAVE_POST_MODIFY_DISP, but rejects
PRE_MODIFY / POST_MODIFY in TARGET_LEGITIMATE_ADDRESS_P .

It appears the dse problem can currently cause ICE-on-valid for  
mn10300 and rx,
which could be changed by my original patch into sporadic
wrong-code generation, when CC_REG happens to be live.  OTOH when CC_REG
is free, we get working code instead of the ICE.
The situation is similar for stormy16 unless a register is used as an
address register that is not suitable for an add - which would continue to
get ICEs.

Re: Impact assessment of dse.c:emit_inc_dec_insn_before (Was: Re: Use of FLAGS_REGNUM clashes with generates insn)

[DJ Delorie]

> m32c.c:m32c_legitimate_address_p allows any rubbish inside a
> PRE_MODIFY, as long as the address register is the stack pointer.
> OTOH, it does not define any HAVE_*_MODIFY_* macro.

m32c has push/pop and no other pre/post modify

Re: Impact assessment of dse.c:emit_inc_dec_insn_before (Was: Re: Use of FLAGS_REGNUM clashes with generates insn)

[Joern Rennecke]

Quoting DJ Delorie <dj@redhat.com>:

>
>> m32c.c:m32c_legitimate_address_p allows any rubbish inside a
>> PRE_MODIFY, as long as the address register is the stack pointer.
>> OTOH, it does not define any HAVE_*_MODIFY_* macro.
>
> m32c has push/pop and no other pre/post modify
I was talking about what addresses are allowed by
the TARGET_LEGITIMATE_ADDRESS_P, not what the hardware implements or
what the register allocator is guided to produce; some optimizers can
get quite 'creative' in mashing rtl together.

Considering the following stanza from m32c_legitimate_address_p,
I don't see how it would reject
(pre_modify:HI
   (reg:CC SP_REGNO)
   (cond_exec:V2HI (pc:SC)
                   (if_then_else:PSI (scratch:BLK) (reg 4004) (return:QI))))


   /* allowing PLUS yields mem:HI(plus:SI(mem:SI(plus:SI in m32c_split_move */
   if (GET_CODE (x) == PRE_DEC
       || GET_CODE (x) == POST_INC || GET_CODE (x) == PRE_MODIFY)
     {
       return (GET_CODE (XEXP (x, 0)) == REG
               && REGNO (XEXP (x, 0)) == SP_REGNO);
     }

Re: Impact assessment of dse.c:emit_inc_dec_insn_before (Was: Re: Use of FLAGS_REGNUM clashes with generates insn)

[DJ Delorie]

> I was talking about what addresses are allowed by the
> TARGET_LEGITIMATE_ADDRESS_P, not what the hardware implements or
> what the register allocator is guided to produce; some optimizers
> can get quite 'creative' in mashing rtl together.

I've never seen it actually do that and get it past the backend, but
if they're doing it now, I suppose that logic is no longer sufficient.

Re: Impact assessment of dse.c:emit_inc_dec_insn_before (Was: Re: Use of FLAGS_REGNUM clashes with generates insn)

[Joern Rennecke]

Quoting DJ Delorie <dj@redhat.com>:

>
>> I was talking about what addresses are allowed by the
>> TARGET_LEGITIMATE_ADDRESS_P, not what the hardware implements or
>> what the register allocator is guided to produce; some optimizers
>> can get quite 'creative' in mashing rtl together.
>
> I've never seen it actually do that and get it past the backend, but
> if they're doing it now, I suppose that logic is no longer sufficient.

Well, this pattern was freely invented, but I've seen the combiner
folding arithmetic and other memory references in place of a register
inside a memory address when you give it the opportunity (for a different
port).  Operand predicates matter, and what memory_operand /
address_operand accept is largely determined by TARGET_LEGITIMATE_ADDRESS_P.

But my original point wasn't so much about this potential/latent problem
in the m32c port as such, as that this is a pathway by which  
dse.c:emit_inc_dec_insn_before could find itself with some mighty  
strange
autoincrement addresses which it would attempt to convert to additions.
These would generally not be recognized by the addition - or any other
existing - patterns.  However, if we discount these weird patterns -
be that because 'that never happens' or because they are blocked by a few
more checks in the target hook, so that the offset in a recognized PRE_MODIFY
is always a valid operand, the m32c addhi3 insn pattern will work, no
matter if you go the optabs route, or use the current raw rtl generation
in dse.c:emit_inc_dec_insn_before .

OTOH, if the PRE_MODIFY is malformed, you are likely to end up with an ICE,
no matter which way you try to make it into an add, and even if you leave it
alone, it is not likely to end in the successful generation of a working
program.  So, although the analysis shows the m32c is not entirely in the
clear with regards to the dead store with auto-increment -> add  
transformation,
and problem there is not really a dse problem.

mn10300, rx and stormy16 show a problem with a flags register clobber.
If the avr wanted to sched cc0, it would join these ranks.

The Epiphany has the further interesting aspect that some additions that can
be done with a post_modify have a wider offset range than an add.