Problem with modyfying "addsi3" in machine description

Problem with modyfying "addsi3" in machine description

[Tomasz Jankowski]

Hello,

I'm working on machine description for modified OpenRISC and at the
moment I'm trying to modify standard "addsi3" insn. I have custom add
in instruction called "radd", which I'm going to use for regular
arithmetic (for computing addresses, offsets for stack pointer etc. I
use "standard" OpenRISC instructions). The problem with "radd" is only
operates on register, so when I have to add immediate to register I
have to first manually copy immediate to temporary register and then
call my "radd" instruction. I implemented code for that:



(define_expand "addsi3"
  [(set (match_operand:SI 0 "general_operand" "")
	(plus:SI (match_operand:SI 1 "general_operand" "")
		 (match_operand:SI 2 "general_operand" "")))]
  ""
  {
   if (CONST_INT_P (operands [2]))
   {
      operands [2] = force_reg (SImode, operands [2]);
      emit_insn (gen_rto (operands [2], operands [2]));
    }

    //emit_insn (gen_rtx_SET (VOIDmode, operands [0], gen_rtx_PLUS
(SImode, operands [1], operands [2])));
    DONE;
  })

(define_insn "radd"
  [(set (match_operand:SI 0 "register_operand" "=r")
	(plus:SI (match_operand:SI 1 "register_operand" "%r")
		 (match_operand:SI 2 "register_operand" "r")))]
  ""
  "l.radd   \t%0,%1,%2"
  [(set_attr "type" "add")
   (set_attr "length" "1")])



During GCC compilation, when self test are run GCC fails on assertion:

    gcc_assert (can_create_pseudo_p ());

in function:

    rtx gen_reg_rtx (enum machine_mode mode)

Why I cannot create temporary registers in define_expand? What's
possible workaround for this issue?

Bye

Re: Problem with modyfying "addsi3" in machine description

[Ian Lance Taylor]

On Fri, Oct 12, 2012 at 8:04 AM, Tomasz Jankowski <tomcioj@gmail.com> wrote:
>
> I'm working on machine description for modified OpenRISC and at the
> moment I'm trying to modify standard "addsi3" insn. I have custom add
> in instruction called "radd", which I'm going to use for regular
> arithmetic (for computing addresses, offsets for stack pointer etc. I
> use "standard" OpenRISC instructions). The problem with "radd" is only
> operates on register, so when I have to add immediate to register I
> have to first manually copy immediate to temporary register and then
> call my "radd" instruction. I implemented code for that:
>
>
>
> (define_expand "addsi3"
>   [(set (match_operand:SI 0 "general_operand" "")
>         (plus:SI (match_operand:SI 1 "general_operand" "")
>                  (match_operand:SI 2 "general_operand" "")))]
>   ""
>   {
>    if (CONST_INT_P (operands [2]))
>    {
>       operands [2] = force_reg (SImode, operands [2]);
>       emit_insn (gen_rto (operands [2], operands [2]));
>     }
>
>     //emit_insn (gen_rtx_SET (VOIDmode, operands [0], gen_rtx_PLUS
> (SImode, operands [1], operands [2])));
>     DONE;
>   })
>
> (define_insn "radd"
>   [(set (match_operand:SI 0 "register_operand" "=r")
>         (plus:SI (match_operand:SI 1 "register_operand" "%r")
>                  (match_operand:SI 2 "register_operand" "r")))]
>   ""
>   "l.radd   \t%0,%1,%2"
>   [(set_attr "type" "add")
>    (set_attr "length" "1")])
>
>
>
> During GCC compilation, when self test are run GCC fails on assertion:
>
>     gcc_assert (can_create_pseudo_p ());
>
> in function:
>
>     rtx gen_reg_rtx (enum machine_mode mode)
>
> Why I cannot create temporary registers in define_expand? What's
> possible workaround for this issue?

Most likely the reload pass has decided that it needs to add a
constant to a register, most likely because it needs to refer to a
stack slot.  In such a case, can_create_pseudo_p will return false.
You could look higher on the stack to see precisely why this is
happening.  But most likely you are going to have to support addsi3
with a constant integer when you can't allocate a new pseudo register.
 The usual way to handle this is to emit two instructions in this
case: one to load the constant into the destination register, one to
add the other source register to the destination register.

Ian

Re: Problem with modyfying "addsi3" in machine description

[Tomasz Jankowski]

Thank you for answering your email.

I actually cannot use you suggestion, because source register and
destination register are almost always the same register. Now I'm
found something weird. Original template "addsi3" for OpenRISC is:

   (define_insn "addsi3"
     [(set (match_operand:SI 0 "register_operand" "=r,r")
       (plus:SI (match_operand:SI 1 "register_operand" "%r,r")
   	 (match_operand:SI 2 "nonmemory_operand" "r,I")))]
    ""
    "@
     l.add   \t%0,%1,%2
     l.addi  \t%0,%1,%2"
    [(set_attr "type" "add,add")
     (set_attr "length" "1,1")])

and "mulsi3" is:

   (define_insn "mulsi3"
     [(set (match_operand:SI 0 "register_operand" "=r")
           (mult:SI (match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")))]
     "TARGET_HARD_MUL"
     "l.mul   \t%0,%1,%2"
     [(set_attr "type" "mul")
      (set_attr "length" "1")])

Now, when I write in C such code:

   int b = 100;
   b *= 101;

GCC properly moves immediate to temporary register and then call
"l.mul" instruction. I tried to do the same trick and modified
"addsi3" template to:

   (define_insn "addsi3"
     [(set (match_operand:SI 0 "register_operand" "=r")
       (plus:SI (match_operand:SI 1 "register_operand" "%r")
   	 (match_operand:SI 2 "register_operand" "r")))]
    ""
    "l.add  \t%0,%1,%2"
    [(set_attr "type" "add")
     (set_attr "length" "1")])

The problem is that, GCC fails to compile with this modified template
with error:
   ../../gcc-4.5.1/gcc/crtstuff.c: In function ‘__do_global_dtors_aux’:
   ../../gcc-4.5.1/gcc/crtstuff.c:332:1: internal compiler error: in
gen_add2_insn, at optabs.c:4764

As I remember documentation for RTL says, that it's able to make
operands fit instruction template, so it's able to move immediate to
register as it's done for "multsi3". So why it doesn't work for
"addsi3"?

Re: Problem with modyfying "addsi3" in machine description

[Tomasz Jankowski]

Ok, I fixed it using using pair of define_insn and define_expand below:

   (define_expand "addsi3"
     [(set (match_operand:SI 0 "register_operand" "=r")
           (plus:SI (match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "nonmemory_operand" "")))]
     ""
     {
       if (CONST_INT_P (operands [2]))
       {
         if (can_create_pseudo_p ())
         {
           operands[2] = force_reg (SImode, operands[2]);
           emit_insn (gen_rtx_SET (SImode, operands[0], gen_rtx_PLUS
(SImode, operands [1], operands [2])));
         }
       }
       else
       {
         emit_insn (gen_rtx_SET (SImode, operands[0], gen_rtx_PLUS
(SImode, operands [1], operands [2])));
       }
       DONE;
     })

   (define_insn "*addsi3_internal"
     [(set (match_operand:SI 0 "register_operand" "=r")
           (plus:SI (match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")))]
     ""
     "l.radd   \t%0,%1,%2"
     [(set_attr "type" "add")
      (set_attr "length" "1")])

I have to admit, that I don't understand why it works... I figured out
possible explanation. When define_expand doesn't emit any insns, but
it finishes with DONE, GCC tries other strategy to fir RTL instruction
to machine description, right?

I have also question about output assembly (compiled with -dP option).
Sample C code:

    int b = 123;
    int a = b + 998;

output:

    #(insn 8 7 19 simple_test.c:4 (set (reg:SI 3 r3 [44])
    #        (const_int 998 [0x3e6])) 8 {*movsi_insn} (nil))
    	l.addi  	r3,r0,998	 # move immediate I	# 8	*movsi_insn/1	[length = 1]
    #(insn 19 8 9 (set (reg:SI 3 r3 [44])
    #        (rto:SI (reg:SI 3 r3 [44]))) 72 {rto} (nil))
    	l.rto    	r3,r3	# 19	rto	[length = 1]
    #(insn 9 19 10 simple_test.c:4 (set:SI (reg:SI 3 r3 [43])
    #        (plus:SI (reg:SI 4 r4 [42])
    #            (reg:SI 3 r3 [44]))) 49 {*addsi3_internal}
(expr_list:REG_EQUAL (plus:SI (reg:SI 4 r4 [42])
    #            (const_int 998 [0x3e6]))
    #        (nil)))
    	l.radd   	r3,r4,r3	# 9	*addsi3_internal	[length = 1]

What "(expr_list:REG_EQUAL (..." stays for?

Re: Problem with modyfying "addsi3" in machine description

[Georg-Johann Lay]

Tomasz Jankowski schrieb:
> Ok, I fixed it using using pair of define_insn and define_expand below:
> 
>    (define_expand "addsi3"
>      [(set (match_operand:SI 0 "register_operand" "=r")
>            (plus:SI (match_operand:SI 1 "register_operand" "r")

Expanders don't get constraints.

>                     (match_operand:SI 2 "nonmemory_operand" "")))]
>      ""
>      {
>        if (CONST_INT_P (operands [2]))

If SI also acts as pointer mode, you may want to handle symbols, too.
For example in address computations.

>        {
>          if (can_create_pseudo_p ())
>          {
>            operands[2] = force_reg (SImode, operands[2]);
>            emit_insn (gen_rtx_SET (SImode, operands[0], gen_rtx_PLUS

SET should be VOIDmode, not SImode.

> (SImode, operands [1], operands [2])));
>          }
>        }
>        else
>        {
>          emit_insn (gen_rtx_SET (SImode, operands[0], gen_rtx_PLUS
> (SImode, operands [1], operands [2])));
>        }
>        DONE;
>      })
> 
>    (define_insn "*addsi3_internal"
>      [(set (match_operand:SI 0 "register_operand" "=r")
>            (plus:SI (match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")))]
>      ""
>      "l.radd   \t%0,%1,%2"
>      [(set_attr "type" "add")
>       (set_attr "length" "1")])
> 
> I have to admit, that I don't understand why it works... I figured out

I don't think it works.  You still have the problem with offsets during
reload.  addsi3 is special, I think you need const_int offsets.
Otherwise spill code generation cannot work.

> possible explanation. When define_expand doesn't emit any insns, but
> it finishes with DONE, GCC tries other strategy to fir RTL instruction
> to machine description, right?
> 
> I have also question about output assembly (compiled with -dP option).
> Sample C code:
> 
>     int b = 123;
>     int a = b + 998;
> 
> output:
> 
>     #(insn 8 7 19 simple_test.c:4 (set (reg:SI 3 r3 [44])
>     #        (const_int 998 [0x3e6])) 8 {*movsi_insn} (nil))
>     	l.addi  	r3,r0,998	 # move immediate I	# 8	*movsi_insn/1	[length = 1]
>     #(insn 19 8 9 (set (reg:SI 3 r3 [44])
>     #        (rto:SI (reg:SI 3 r3 [44]))) 72 {rto} (nil))
>     	l.rto    	r3,r3	# 19	rto	[length = 1]
>     #(insn 9 19 10 simple_test.c:4 (set:SI (reg:SI 3 r3 [43])
>     #        (plus:SI (reg:SI 4 r4 [42])
>     #            (reg:SI 3 r3 [44]))) 49 {*addsi3_internal}
> (expr_list:REG_EQUAL (plus:SI (reg:SI 4 r4 [42])
>     #            (const_int 998 [0x3e6]))
>     #        (nil)))
>     	l.radd   	r3,r4,r3	# 9	*addsi3_internal	[length = 1]
> 
> What "(expr_list:REG_EQUAL (..." stays for?

It says that content of r3 equals r4 + 998.

Re: Problem with modyfying "addsi3" in machine description

[Tomasz Jankowski]

Thank you for all hints! I'll use them in my code.

2012/10/14 Georg-Johann Lay <gjl@gcc.gnu.org>:
> Expanders don't get constraints.

This sounds reasonable, but it was not stated in documentation.

>
> SET should be VOIDmode, not SImode.
>
Ok, but why? I saw in few places (I'm almost sure event in official
GCC sources), that SET was used with SImode. Why it's so important?

>
> I don't think it works.  You still have the problem with offsets during
> reload.  addsi3 is special, I think you need const_int offsets.
> Otherwise spill code generation cannot work.
>
But what's so special about addsi3? For mulsi3 GCC is able to easily
change strategy and move immediate to register itself. Why doesn't it
work for addsi3 too? What "problem with offset during reload" actually
means?

Bye!

Re: Problem with modyfying "addsi3" in machine description

[Segher Boessenkool]

>> Expanders don't get constraints.
>
> This sounds reasonable, but it was not stated in documentation.

Constraints in expanders are ignored.  This is documented.
To not confuse yourself or others, it's best not to write a
constraint.

>> SET should be VOIDmode, not SImode.
>>
> Ok, but why? I saw in few places (I'm almost sure event in official
> GCC sources), that SET was used with SImode. Why it's so important?

Parts of the compiler might depend on it being VOIDmode always.
Yes, some targets get this wrong in places.

Use emit_move_insn :-)

>> I don't think it works.  You still have the problem with offsets  
>> during
>> reload.  addsi3 is special, I think you need const_int offsets.
>> Otherwise spill code generation cannot work.
>>
> But what's so special about addsi3? For mulsi3 GCC is able to easily
> change strategy and move immediate to register itself. Why doesn't it
> work for addsi3 too? What "problem with offset during reload" actually
> means?

GCC does the same thing with addsi3 as with mulsi3, _during expand_.
Reload however can create new addsi3 instructions out of thin air.
For example, if it wants to use some stack slot but it cannot access
it directly, it will compute the address of the stack slot into a
register: reg := sp+offset.  You need to be able to handle those
additions (well, some of them; at least the addsi2 things it tries
if you fail the addsi3 attempts).


Segher