Seeking suggestion

Seeking suggestion

[Jamie Prescott]

Suppose you're writing the backend for a VM supporting two architectures, in which
one of them clobbers the CC registers for certain instructions, while the other does not.
The instructions themselves are exactly the same.
What is the best/shortest/more-elegant way to write this, possibly w/out duplicating the
instructions?
I know I can write a define_expand and redirect, based on the TARGET, to two different
instructions (one with "clobber", the other w/out), but that's basically three declarations
for each insns. Is there a shorter way?


 - Jamie



      

Re: Seeking suggestion

[Jamie Prescott]

> From: Jamie Prescott <jpresss@yahoo.com>
> To: gcc@gcc.gnu.org
> Sent: Friday, May 22, 2009 10:36:47 AM
> Subject: Seeking suggestion
> 
> 
> Suppose you're writing the backend for a VM supporting two architectures, in 
> which
> one of them clobbers the CC registers for certain instructions, while the other 
> does not.
> The instructions themselves are exactly the same.
> What is the best/shortest/more-elegant way to write this, possibly w/out 
> duplicating the
> instructions?
> I know I can write a define_expand and redirect, based on the TARGET, to two 
> different
> instructions (one with "clobber", the other w/out), but that's basically three 
> declarations
> for each insns. Is there a shorter way?

I ended up doing something like this (long way, but the only one I know of).
Example, for addsi3:

(define_insn "addsi3_xxx2"
  [(set (match_operand:SI 0 "fullreg_operand" "=r,r")
        (plus:SI (match_operand:SI 1 "fullreg_operand" "0,r")
                 (match_operand:SI 2 "fullreg_or_imm_operand" "rn,rn")))]
  ""
  "@
   add\t%0,%2,%0
   add\t%1,%2,%0"
)

(define_insn "addsi3_xxx"
  [(set (match_operand:SI 0 "fullreg_operand" "=r,r")
        (plus:SI (match_operand:SI 1 "fullreg_operand" "0,r")
                 (match_operand:SI 2 "fullreg_or_imm_operand" "rn,rn")))
   (clobber (reg:CC CC_REG))]
  ""
  "@
   add\t%0,%2,%0
   add\t%1,%2,%0"
)

(define_expand "addsi3"
  [(set (match_operand:SI 0 "fullreg_operand" "=r,r")
        (plus:SI (match_operand:SI 1 "fullreg_operand" "0,r")
                 (match_operand:SI 2 "fullreg_or_imm_operand" "rn,rn")))]
  ""
  {
    if (!TARGET_XXX2)
      emit_insn(gen_addsi3_xxx(operands[0], operands[1], operands[2]));
    else
      emit_insn(gen_addsi3_xxx2(operands[0], operands[1], operands[2]));
    DONE;
  }
)


But now I get and invalid rtx sharing from the push/pop parallels:


xxxx.c: In function 'test_dashr':
xxxx.c:32: error: invalid rtl sharing found in the insn
(insn 26 3 28 2 xxxx.c:26 (parallel [
            (insn/f 25 0 0 (set (reg/f:SI 51 SP)
                    (minus:SI (reg/f:SI 51 SP)
                        (const_int 4 [0x4]))) -1 (nil))
            (set/f (mem:SI (reg/f:SI 51 SP) [0 S4 A8])
                (reg:SI 8 r8))
        ]) -1 (nil))
xxxx.c:32: error: shared rtx
(insn/f 25 0 0 (set (reg/f:SI 51 SP)
        (minus:SI (reg/f:SI 51 SP)
            (const_int 4 [0x4]))) -1 (nil))
xxxx.c:32: internal compiler error: internal consistency failure


Any insights?
Thanks,


- Jamie


      

Re: Seeking suggestion

[Ian Lance Taylor]

Jamie Prescott <jpresss@yahoo.com> writes:

> But now I get and invalid rtx sharing from the push/pop parallels:

This normally means that you need a copy_rtx somewhere.  Different insns
may not share data structure.

Ian

Re: Seeking suggestion

[Jamie Prescott]

> From: Ian Lance Taylor <iant@google.com>
> To: Jamie Prescott <jpresss@yahoo.com>
> Cc: gcc@gcc.gnu.org
> Sent: Friday, May 22, 2009 5:45:21 PM
> Subject: Re: Seeking suggestion
> 
> Jamie Prescott writes:
> 
> > But now I get and invalid rtx sharing from the push/pop parallels:
> 
> This normally means that you need a copy_rtx somewhere.  Different insns
> may not share data structure.

Ok, fixed. I was generating the parallel for push/pop by calling directly gen_addsi3.
This eventually was generating that problem with the code I posted in the previous
message.
Once I open coded the addsi3 with gen_rtx_SET(gen_rtx_PLUS()), the error went
away.
Is the implementation I posted the only one, or there are shorter/better ones?


- Jamie


      

Re: Seeking suggestion

[Georg-Johann Lay]

Jamie Prescott schrieb:

> Is the implementation I posted the only one, or there are shorter/better ones?

You could use insn attribute to express insns' effects on cc_status.
Have a look at the avr backend.

Georg-Johann

Re: Seeking suggestion

[Jamie Prescott]

> From: Georg-Johann Lay <avr@gjlay.de>
> To: Jamie Prescott <jpresss@yahoo.com>
> Cc: Ian Lance Taylor <iant@google.com>; gcc@gcc.gnu.org
> Sent: Saturday, May 23, 2009 12:05:09 AM
> Subject: Re: Seeking suggestion
> 
> Jamie Prescott schrieb:
> 
> > Is the implementation I posted the only one, or there are shorter/better ones?
> 
> You could use insn attribute to express insns' effects on cc_status.
> Have a look at the avr backend.

AVR uses CC0, while I just switched to CCmode.
Is there a reason why something like this would not work?

(define_insn "addsi3_nc"
  [(set (match_operand:SI 0 "fullreg_operand" "=r")
        (plus:SI (match_operand:SI 1 "fullreg_operand" "r")
                 (match_operand:SI 2 "fullreg_or_imm_operand" "rn")))]
  ""
  "..."
)

(define_expand "addsi3"
  [(set (match_operand:SI 0 "fullreg_operand" "=r")
        (plus:SI (match_operand:SI 1 "fullreg_operand" "r")
                 (match_operand:SI 2 "fullreg_or_imm_operand" "rn")))]
  ""
  {
    if (!TARGET_XXX2)
      emit_clobber(gen_rtx_REG(CCmode, CC_REGNUM));
    emit_insn(gen_addsi3_nc(operands[0], operands[1], operands[2]));
    DONE;
  }
)

That would limit to two instructions per basic insns, instead of the current three.
Thanks,

- Jamie


      

Re: Seeking suggestion

[Georg-Johann Lay]

Jamie Prescott schrieb:

> Is there a reason why something like this would not work?
> 
> (define_insn "addsi3_nc"
>   [(set (match_operand:SI 0 "fullreg_operand" "=r")
>         (plus:SI (match_operand:SI 1 "fullreg_operand" "r")
>                  (match_operand:SI 2 "fullreg_or_imm_operand" "rn")))]
>   ""
>   "..."
> )
> 
> (define_expand "addsi3"
>   [(set (match_operand:SI 0 "fullreg_operand" "=r")
>         (plus:SI (match_operand:SI 1 "fullreg_operand" "r")
>                  (match_operand:SI 2 "fullreg_or_imm_operand" "rn")))]
>   ""
>   {
>     if (!TARGET_XXX2)
>       emit_clobber(gen_rtx_REG(CCmode, CC_REGNUM));
>     emit_insn(gen_addsi3_nc(operands[0], operands[1], operands[2]));
>     DONE;
>   }
> )
> 
> That would limit to two instructions per basic insns, instead of the current three.
> Thanks,
> 
> - Jamie

Maybe that works. But I would not include such stuff in one of my 
machine descriptions because addsi3_nc lies: Its effect on the machine 
differ from what it states algebraically in RTL. Therefore, you may get 
trouble in corner cases or when instructions are 
reordered/scheduled/combined/...

Georg-Johann

Re: Seeking suggestion

[Michael Meissner]

On Fri, May 22, 2009 at 05:04:22PM -0700, Jamie Prescott wrote:
> 
> > From: Jamie Prescott <jpresss@yahoo.com>
> > To: gcc@gcc.gnu.org
> > Sent: Friday, May 22, 2009 10:36:47 AM
> > Subject: Seeking suggestion
> > 
> > 
> > Suppose you're writing the backend for a VM supporting two architectures, in 
> > which
> > one of them clobbers the CC registers for certain instructions, while the other 
> > does not.
> > The instructions themselves are exactly the same.
> > What is the best/shortest/more-elegant way to write this, possibly w/out 
> > duplicating the
> > instructions?
> > I know I can write a define_expand and redirect, based on the TARGET, to two 
> > different
> > instructions (one with "clobber", the other w/out), but that's basically three 
> > declarations
> > for each insns. Is there a shorter way?
> 
> I ended up doing something like this (long way, but the only one I know of).
> Example, for addsi3:
> 
> (define_insn "addsi3_xxx2"
>   [(set (match_operand:SI 0 "fullreg_operand" "=r,r")
>         (plus:SI (match_operand:SI 1 "fullreg_operand" "0,r")
>                  (match_operand:SI 2 "fullreg_or_imm_operand" "rn,rn")))]
>   ""
>   "@
>    add\t%0,%2,%0
>    add\t%1,%2,%0"
> )
> 
> (define_insn "addsi3_xxx"
>   [(set (match_operand:SI 0 "fullreg_operand" "=r,r")
>         (plus:SI (match_operand:SI 1 "fullreg_operand" "0,r")
>                  (match_operand:SI 2 "fullreg_or_imm_operand" "rn,rn")))
>    (clobber (reg:CC CC_REG))]
>   ""
>   "@
>    add\t%0,%2,%0
>    add\t%1,%2,%0"
> )
> 
> (define_expand "addsi3"
>   [(set (match_operand:SI 0 "fullreg_operand" "=r,r")
>         (plus:SI (match_operand:SI 1 "fullreg_operand" "0,r")
>                  (match_operand:SI 2 "fullreg_or_imm_operand" "rn,rn")))]
>   ""
>   {
>     if (!TARGET_XXX2)
>       emit_insn(gen_addsi3_xxx(operands[0], operands[1], operands[2]));
>     else
>       emit_insn(gen_addsi3_xxx2(operands[0], operands[1], operands[2]));
>     DONE;
>   }
> )

One way is to use match_scratch, and different register classes for the two
cases.

(define_insn "add<mode>3"
  [(set (match_operand:SI 0 "register_operand" "=x,y")
	(plus:SI (match_operand:SI 1 "register_operand" "%x,y")
		 (match_operand:SI 2 "register_operand" "x,y")))
   (clobber (match_scratch:CC 3 "=X,z"))]
  ""
  "add %0,%1,%2")


(define_register_constraint "x" "TARGET_MACHINE ? GENERAL_REGS : NO_REGS"
  "@internal")

(define_register_constraint "y" "!TARGET_MACHINE ? GENERAL_REGS : NO_REGS"
  "@internal")

(define_register_constraint "z" CR_REGS "@interal")

This assumes you have a register class for the condition code register.  Most
machines however, use the normal define_expand with two different insns.

In theory, you could define a second condition code register that doesn't
actually exist in the machine, and change the clobber from the main CC to the
fake one.

 
> But now I get and invalid rtx sharing from the push/pop parallels:
> 
> 
> xxxx.c: In function 'test_dashr':
> xxxx.c:32: error: invalid rtl sharing found in the insn
> (insn 26 3 28 2 xxxx.c:26 (parallel [
>             (insn/f 25 0 0 (set (reg/f:SI 51 SP)
>                     (minus:SI (reg/f:SI 51 SP)
>                         (const_int 4 [0x4]))) -1 (nil))
>             (set/f (mem:SI (reg/f:SI 51 SP) [0 S4 A8])
>                 (reg:SI 8 r8))
>         ]) -1 (nil))
> xxxx.c:32: error: shared rtx
> (insn/f 25 0 0 (set (reg/f:SI 51 SP)
>         (minus:SI (reg/f:SI 51 SP)
>             (const_int 4 [0x4]))) -1 (nil))
> xxxx.c:32: internal compiler error: internal consistency failure

I suspect you don't have the proper guards on the push/pop insns, and the
combiner is eliminating the clobber.  You probably need to have parallel insns
for the push and pop.

-- 
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com

Re: Seeking suggestion

[Jamie Prescott]

> From: Michael Meissner <meissner@linux.vnet.ibm.com>
> To: Jamie Prescott <jpresss@yahoo.com>
> Cc: gcc@gcc.gnu.org
> Sent: Sunday, May 24, 2009 1:57:19 PM
> Subject: Re: Seeking suggestion
> 
> One way is to use match_scratch, and different register classes for the two
> cases.
> 
> (define_insn "add3"
>   [(set (match_operand:SI 0 "register_operand" "=x,y")
>     (plus:SI (match_operand:SI 1 "register_operand" "%x,y")
>          (match_operand:SI 2 "register_operand" "x,y")))
>    (clobber (match_scratch:CC 3 "=X,z"))]
>   ""
>   "add %0,%1,%2")
> 
> 
> (define_register_constraint "x" "TARGET_MACHINE ? GENERAL_REGS : NO_REGS"
>   "@internal")
> 
> (define_register_constraint "y" "!TARGET_MACHINE ? GENERAL_REGS : NO_REGS"
>   "@internal")
> 
> (define_register_constraint "z" CR_REGS "@interal")
> 
> This assumes you have a register class for the condition code register.  Most
> machines however, use the normal define_expand with two different insns.
> 
> In theory, you could define a second condition code register that doesn't
> actually exist in the machine, and change the clobber from the main CC to the
> fake one.

Hmm, interesting. Thanks Michael.
Though, as you were saying, I'll probably leave them as separate insns. These should
have been two separate targets probably, but I'm too lazy to split them up ATM.



> > But now I get and invalid rtx sharing from the push/pop parallels:
> > 
> > 
> > xxxx.c: In function 'test_dashr':
> > xxxx.c:32: error: invalid rtl sharing found in the insn
> > (insn 26 3 28 2 xxxx.c:26 (parallel [
> >             (insn/f 25 0 0 (set (reg/f:SI 51 SP)
> >                     (minus:SI (reg/f:SI 51 SP)
> >                         (const_int 4 [0x4]))) -1 (nil))
> >             (set/f (mem:SI (reg/f:SI 51 SP) [0 S4 A8])
> >                 (reg:SI 8 r8))
> >         ]) -1 (nil))
> > xxxx.c:32: error: shared rtx
> > (insn/f 25 0 0 (set (reg/f:SI 51 SP)
> >         (minus:SI (reg/f:SI 51 SP)
> >             (const_int 4 [0x4]))) -1 (nil))
> > xxxx.c:32: internal compiler error: internal consistency failure
> 
> I suspect you don't have the proper guards on the push/pop insns, and the
> combiner is eliminating the clobber.  You probably need to have parallel insns
> for the push and pop.

Dunno exactly what was happening. The push/pop were generated with a parallel,
but I was issuing a gen_addsi3() directly, and this somehow was creating the problem.
Once I open coded that with SET(SP, PLUS(SP, SIZE)), the issue disappeared.


- Jamie


      

Re: Seeking suggestion

[Jim Wilson]

Jamie Prescott wrote:
> Is there a reason why something like this would not work?
>     if (!TARGET_XXX2)
>       emit_clobber(gen_rtx_REG(CCmode, CC_REGNUM));
>     emit_insn(gen_addsi3_nc(operands[0], operands[1], operands[2]));

Yes.  The optimizer will not know that addsi3_nc uses CC_REGNUM, as it 
is not mentioned, so the optimizer will not know that these two RTL 
instructions always need to remain next to each other.  Any optimization 
pass that moves insns around may separate the add from the clobber 
resulting in broken code.  This is what parallels are for, to make sure 
that the clobber and add stay together.

Jim

Re: Seeking suggestion

[Jamie Prescott]

> From: Jim Wilson <wilson@codesourcery.com>
> To: Jamie Prescott <jpresss@yahoo.com>
> Cc: Georg-Johann Lay <avr@gjlay.de>; Ian Lance Taylor <iant@google.com>; gcc@gcc.gnu.org
> Sent: Tuesday, May 26, 2009 7:47:45 PM
> Subject: Re: Seeking suggestion
> 
> Jamie Prescott wrote:
> > Is there a reason why something like this would not work?
> >     if (!TARGET_XXX2)
> >       emit_clobber(gen_rtx_REG(CCmode, CC_REGNUM));
> >     emit_insn(gen_addsi3_nc(operands[0], operands[1], operands[2]));
> 
> Yes.  The optimizer will not know that addsi3_nc uses CC_REGNUM, as it is not 
> mentioned, so the optimizer will not know that these two RTL instructions always 
> need to remain next to each other.  Any optimization pass that moves insns 
> around may separate the add from the clobber resulting in broken code.  This is 
> what parallels are for, to make sure that the clobber and add stay together.

Thanks for the explanation. I somehow thought that every insn spit out by a define_insn
was automatically turned into a parallel.
I guess I was wrong.


- Jamie


      

Re: Seeking suggestion

[Jamie Prescott]

> From: Jamie Prescott <jpresss@yahoo.com>
> To: Jim Wilson <wilson@codesourcery.com>
> Cc: Georg-Johann Lay <avr@gjlay.de>; Ian Lance Taylor <iant@google.com>; gcc@gcc.gnu.org
> Sent: Wednesday, May 27, 2009 10:12:42 AM
> Subject: Re: Seeking suggestion
>
> Thanks for the explanation. I somehow thought that every insn spit out by a 
> define_insn
> was automatically turned into a parallel.

Packed into a parallel, was more what I meant to imply.


- Jamie


      

Re: Seeking suggestion

[Eric Botcazou]

> Thanks for the explanation. I somehow thought that every insn spit out by a
> define_insn was automatically turned into a parallel.

That's true, the template of a define_insn is automatically wrapped up in a 
PARALLEL.  But your addsi3 is a define_expand and this works differently.

-- 
Eric Botcazou

Re: Seeking suggestion

[Jamie Prescott]

> From: Eric Botcazou <ebotcazou@adacore.com>
> To: Jamie Prescott <jpresss@yahoo.com>
> Cc: gcc@gcc.gnu.org; Jim Wilson <wilson@codesourcery.com>; Georg-Johann Lay <avr@gjlay.de>; Ian Lance Taylor <iant@google.com>
> Sent: Wednesday, May 27, 2009 10:37:24 AM
> Subject: Re: Seeking suggestion
> 
> > Thanks for the explanation. I somehow thought that every insn spit out by a
> > define_insn was automatically turned into a parallel.
> 
> That's true, the template of a define_insn is automatically wrapped up in a 
> PARALLEL.  But your addsi3 is a define_expand and this works differently.

Oh, OK. Thanks. So in case of a define_expand I have to manually pack the
multiple insns into a parallel, if I want them to stick together, right?


- Jamie


      

Re: Seeking suggestion

[Georg-Johann Lay]

Jamie Prescott schrieb:

>>>Thanks for the explanation. I somehow thought that every insn spit out by a
>>>define_insn was automatically turned into a parallel.
>>
>>That's true, the template of a define_insn is automatically wrapped up in a 
>>PARALLEL.  But your addsi3 is a define_expand and this works differently.
> 
> 
> Oh, OK. Thanks. So in case of a define_expand I have to manually pack the
> multiple insns into a parallel, if I want them to stick together, right?

You are running in circles... you original solution with two distinct 
insns already did that (implicitely), as one insn expanded to a parallel 
add+clobber and the other to a plain addsi.

Georg-Johann

Re: Seeking suggestion

[Jamie Prescott]

> From: Georg-Johann Lay <avr@gjlay.de>
> To: Jamie Prescott <jpresss@yahoo.com>
> Cc: Eric Botcazou <ebotcazou@adacore.com>; gcc@gcc.gnu.org; Jim Wilson <wilson@codesourcery.com>; Ian Lance Taylor <iant@google.com>
> Sent: Wednesday, May 27, 2009 12:11:08 PM
> Subject: Re: Seeking suggestion
> 
> Jamie Prescott schrieb:
> 
> >>> Thanks for the explanation. I somehow thought that every insn spit out by a
> >>> define_insn was automatically turned into a parallel.
> >> 
> >> That's true, the template of a define_insn is automatically wrapped up in a 
> PARALLEL.  But your addsi3 is a define_expand and this works differently.
> > 
> > 
> > Oh, OK. Thanks. So in case of a define_expand I have to manually pack the
> > multiple insns into a parallel, if I want them to stick together, right?
> 
> You are running in circles... you original solution with two distinct insns 
> already did that (implicitely), as one insn expanded to a parallel add+clobber 
> and the other to a plain addsi.

Yes, I know. The original solution works.
This sub-thread started from Michael response to another solution I proposed in
order to avoid doubling the instructions.


- Jamie