Immediates propagated wrongly in stores

Immediates propagated wrongly in stores

[Jean Christophe Beyler]

Dear all,

I have this weird issue that I can't really understand:

In my architecture I do not have a store immediate into memory, I have
to go through a register.

However, the compiler is currently propagating constants into the
stores. So for example:

(insn 44 43 45 4 st3.c:21 (set (reg:DI 119)
        (const_int 1 [0x1])) -1 (nil))

(insn 45 44 46 4 st3.c:21 (set (mem/s:DI (reg:DI 109 [ ivtmp.43 ]) [2
data S8 A64])
        (reg:DI 119)) -1 (nil))

is transformed into :

(insn 46 44 48 3 st3.c:22 (set (mem/s:DI (reg:DI 109 [ ivtmp.43 ]) [2
data S8 A64])
        (const_int 1 [0x1])) 72 {movdi_internal1} (expr_list:REG_EQUAL
(const_int 1 [0x1])
        (nil)))


I tracked it down to the gcse pass. However, I thought I had turned
this off in the definition of a movdi in my machine description. But
apparently this is not sufficient, any ideas?

As always, thanks for your time,
Jean Christophe Beyler

Re: Immediates propagated wrongly in stores

[Richard Henderson]

On 07/01/2009 08:36 AM, Jean Christophe Beyler wrote:
> I tracked it down to the gcse pass. However, I thought I had turned
> this off in the definition of a movdi in my machine description. But
> apparently this is not sufficient, any ideas?

You probably just changed the constraint letters, but didn't change
the operand predicate.  The constraint letters are only used during
register allocation, and the predicate is used elsewhere.

   (match_operand:MODE N "predicate_function" "constraint_letters")

There are a number of pre-defined predicate functions you can use,

   register_operand
   nonmemory_operand
   nonimmediate_operand
   general_operand

and most ports find they need special ones to exactly match the
characteristics of some insns.  e.g.

cpu.md:
(define_insn "*adddi_internal"
   [(set (match_operand:DI 0 "register_operand" "=r,r,r")
         (plus:DI (match_operand:DI 1 "register_operand" "%r,r,r")
                  (match_operand:DI 2 "add_operand" "r,K,L")))]
   ""
   "@
    addq %1,%2,%0
    lda %0,%2(%1)
    ldah %0,%h2(%1)")

predicates.md:
(define_predicate "add_operand"
   (if_then_else (match_code "const_int")
     (match_test "satisfies_constraint_K (op) ||
                  satisfies_constraint_L (op)")
     (match_operand 0 "register_operand")))

You should strive to ensure that the predicate function exactly
matches the constraint letters.  If the predicate function accepts
more than the constraint letters, the reload pass will fix it up.
But better code is generated when reload doesn't need to do this.


r~

Re: Immediates propagated wrongly in stores

[Jean Christophe Beyler]

Ok, I think I understand, I've been therefore playing with this. I
initially had:

(define_insn "movdi_internal1"
  [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
        (match_operand:DI 1 "general_operand"      "r,iF,R,J,J,o,r,r"))]


For my movdi. I therefore first wanted to handle the fact that I won't
allow stores to use immediates so I thought of changing the above
into:

(define_insn "movdi_internal1"
  [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
        (match_operand:DI 1 "move_operand"      "r,iF,R,J,J,o,r,r"))]

And then defining a simple predicate:

(define_predicate "move_operand"
 (if_then_else (match_operand 0 "register_operand")
        (match_operand 1 "general_operand")
        (match_operand 1 "register_operand")))

For me this means :
If the first operand is a register (that would then either be a
register-register move or a load),
   Then return whether or not operand 1 is a general operand
   Else it's a memory operand (by definition of the
nonimmediate_operand predicate) and
                  return whether or not operand 1 is a register operand

In essence, it seemed to me that this should work as a first draft in
rewriting the whole movdi (and let me test it). However, this makes
the compiler fail on this instruction (unrecognisable)

st4.c:66: error: unrecognizable insn:
(insn 41 40 42 3 st4.c:22 (set (reg/f:DI 119)
        (symbol_ref:DI ("data") <var_decl 0xb7c8ebb0 data>)) -1 (nil))


For me this doesn't make sense since in this set, the predicate should
check the first register. The test of the if should be true, therefore
matching the second operand to a "general_operand" that it is, no?

Thanks for your help,
Jean Christophe Beyler

On Wed, Jul 1, 2009 at 12:23 PM, Richard Henderson<rth@redhat.com> wrote:
> On 07/01/2009 08:36 AM, Jean Christophe Beyler wrote:
>>
>> I tracked it down to the gcse pass. However, I thought I had turned
>> this off in the definition of a movdi in my machine description. But
>> apparently this is not sufficient, any ideas?
>
> You probably just changed the constraint letters, but didn't change
> the operand predicate.  The constraint letters are only used during
> register allocation, and the predicate is used elsewhere.
>
>  (match_operand:MODE N "predicate_function" "constraint_letters")
>
> There are a number of pre-defined predicate functions you can use,
>
>  register_operand
>  nonmemory_operand
>  nonimmediate_operand
>  general_operand
>
> and most ports find they need special ones to exactly match the
> characteristics of some insns.  e.g.
>
> cpu.md:
> (define_insn "*adddi_internal"
>  [(set (match_operand:DI 0 "register_operand" "=r,r,r")
>        (plus:DI (match_operand:DI 1 "register_operand" "%r,r,r")
>                 (match_operand:DI 2 "add_operand" "r,K,L")))]
>  ""
>  "@
>   addq %1,%2,%0
>   lda %0,%2(%1)
>   ldah %0,%h2(%1)")
>
> predicates.md:
> (define_predicate "add_operand"
>  (if_then_else (match_code "const_int")
>    (match_test "satisfies_constraint_K (op) ||
>                 satisfies_constraint_L (op)")
>    (match_operand 0 "register_operand")))
>
> You should strive to ensure that the predicate function exactly
> matches the constraint letters.  If the predicate function accepts
> more than the constraint letters, the reload pass will fix it up.
> But better code is generated when reload doesn't need to do this.
>
>
> r~
>

Re: Immediates propagated wrongly in stores

[Richard Henderson]

On 07/01/2009 11:28 AM, Jean Christophe Beyler wrote:
> Ok, I think I understand, I've been therefore playing with this. I
> initially had:
>
> (define_insn "movdi_internal1"
>    [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
>          (match_operand:DI 1 "general_operand"      "r,iF,R,J,J,o,r,r"))]
>
>
> For my movdi. I therefore first wanted to handle the fact that I won't
> allow stores to use immediates so I thought of changing the above
> into:
>
> (define_insn "movdi_internal1"
>    [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
>          (match_operand:DI 1 "move_operand"      "r,iF,R,J,J,o,r,r"))]
>
> And then defining a simple predicate:
>
> (define_predicate "move_operand"
>   (if_then_else (match_operand 0 "register_operand")
>          (match_operand 1 "general_operand")
>          (match_operand 1 "register_operand")))

Predicates can only examine one operand, not two.

However, you can write

(define_expand "movdi"
   [(set (match_operand:DI "nonimmediate_operand" "")
         (match_operand:DI "general_operand" ""))]
   ""
   "my_expand_move (operands[0], operands[1]); DONE;")

(define_insn "*movdi_internal"
   [(set (match_operand:DI 0 "nonimmediate_operand" "...")
	(match_operand:DI 1 "general_operand" "..."))]
   "my_move_ok (operands[0], operands[1])"
   ...)

void my_expand_move (rtx op0, rtx op1)
{
    // Handle TLS, PIC and other special cases...

   if (MEM_P (op0))
     op1 = force_reg (mode, op1);

   emit_insn (gen_rtx_SET (VOIDmode, op0, op1));
}

bool my_move_ok(rtx op0, rtx op1)
{
   if (MEM_P (op0))
     return register_operand (op1, VOIDmode);
   return true;
}

Compare this with similar code in other ports.



r~

Re: Immediates propagated wrongly in stores

[Jean Christophe Beyler]

I tried something similar:

(define_expand "movdi"
  [(set (match_operand:DI 0 "nonimmediate_operand" "")
        (match_operand:DI 1 "general_operand" ""))]
  ""
  "
    if(my_expand_move (operands[0], operands[1]))
       DONE;
  ")

(define_insn "movdi_internal1"
  [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
        (match_operand:DI 1 "general_operand"      "r,iF,R,J,J,o,r,r"))]
  "my_move_ok (operands[0], operands[1])"
  "* return my_move_2words (operands, insn); "
  [(set_attr "type"     "move,arith,load,store,store,load,store,store")
   (set_attr "mode"     "DI,DI,DI,DI,DI,DI,DI,DI")
   (set_attr "length"   "1,1,1,1,1,1,1,1")])


int my_expand_move (rtx op0, rtx op1)
{
  /* Removed PIC, etc. for the moment */
    if (
            ((reload_in_progress | reload_completed) == 0 &&
            MEM_P (op0) && !REG_P (op1)))
    {
        op1 = force_reg (GET_MODE (op0), op1);
        emit_move_insn (op0, op1);
        return 1;
    }
   return 0;
}

bool my_move_ok(rtx op0, rtx op1)
{
    if (MEM_P (op0))
        return register_operand (op1, VOIDmode);

    return true;
}


This seems to do what I want, now I'm going to clean up all those
constraints and see if they are compatible and work on from there.

Thanks for your help, tell me if my solution seems very bad,
Jc


On Wed, Jul 1, 2009 at 3:26 PM, Richard Henderson<rth@redhat.com> wrote:
> On 07/01/2009 11:28 AM, Jean Christophe Beyler wrote:
>>
>> Ok, I think I understand, I've been therefore playing with this. I
>> initially had:
>>
>> (define_insn "movdi_internal1"
>>   [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
>>         (match_operand:DI 1 "general_operand"      "r,iF,R,J,J,o,r,r"))]
>>
>>
>> For my movdi. I therefore first wanted to handle the fact that I won't
>> allow stores to use immediates so I thought of changing the above
>> into:
>>
>> (define_insn "movdi_internal1"
>>   [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,R,T,r,R,o")
>>         (match_operand:DI 1 "move_operand"      "r,iF,R,J,J,o,r,r"))]
>>
>> And then defining a simple predicate:
>>
>> (define_predicate "move_operand"
>>  (if_then_else (match_operand 0 "register_operand")
>>         (match_operand 1 "general_operand")
>>         (match_operand 1 "register_operand")))
>
> Predicates can only examine one operand, not two.
>
> However, you can write
>
> (define_expand "movdi"
>  [(set (match_operand:DI "nonimmediate_operand" "")
>        (match_operand:DI "general_operand" ""))]
>  ""
>  "my_expand_move (operands[0], operands[1]); DONE;")
>
> (define_insn "*movdi_internal"
>  [(set (match_operand:DI 0 "nonimmediate_operand" "...")
>        (match_operand:DI 1 "general_operand" "..."))]
>  "my_move_ok (operands[0], operands[1])"
>  ...)
>
> void my_expand_move (rtx op0, rtx op1)
> {
>   // Handle TLS, PIC and other special cases...
>
>  if (MEM_P (op0))
>    op1 = force_reg (mode, op1);
>
>  emit_insn (gen_rtx_SET (VOIDmode, op0, op1));
> }
>
> bool my_move_ok(rtx op0, rtx op1)
> {
>  if (MEM_P (op0))
>    return register_operand (op1, VOIDmode);
>  return true;
> }
>
> Compare this with similar code in other ports.
>
>
>
> r~
>

Re: Immediates propagated wrongly in stores

[Richard Henderson]

On 07/01/2009 02:02 PM, Jean Christophe Beyler wrote:
>              ((reload_in_progress | reload_completed) == 0&&
>              MEM_P (op0)&&  !REG_P (op1)))
>      {
>          op1 = force_reg (GET_MODE (op0), op1);
>          emit_move_insn (op0, op1);
>          return 1;

I wouldn't think you'd actually need these reload checks.
At least some other ports I glanced at don't need them.


r~

Re: Immediates propagated wrongly in stores

[Jean Christophe Beyler]

Sorry about the delay in the answer (4th of July weekend...).

Anyway, I agree it might not be necessary. I put it there because of
the force_reg call. Since that call might in turn call gen_reg_rtx, it
will then start off with an assertion of can_create_pseudo_p.

That is why, I thought best to put that test.

On Wed, Jul 1, 2009 at 5:33 PM, Richard Henderson<rth@redhat.com> wrote:
> On 07/01/2009 02:02 PM, Jean Christophe Beyler wrote:
>>
>>             ((reload_in_progress | reload_completed) == 0&&
>>             MEM_P (op0)&&  !REG_P (op1)))
>>     {
>>         op1 = force_reg (GET_MODE (op0), op1);
>>         emit_move_insn (op0, op1);
>>         return 1;
>
> I wouldn't think you'd actually need these reload checks.
> At least some other ports I glanced at don't need them.
>
>
> r~
>