IRA changes rules of the game

IRA changes rules of the game

[Paulo J. Matos]

Hi,

And by rules of the game, I mean the semantics of the insn chain.

In comes in the sequence of a previous post where I am splitting a neghi 
operation like this:

op0 = negHI(op1)

expands to:
op0 = op1
op0_HIGH = xorQI(op0_HIGH, -1)
parallel(
    op0_LOW = negQI(op0_LOW)
    op0_HIGH = add(op0_HIGH, CARRY_FLAG)
   )

The parallel is then split after reload (due to flag read/write 
constraints).
In the end I expect something like:
xor op0_HIGH, -1
nadd op0_LOW, 0 // returns -(op0_LOW + 0)
addc op0_HIGH, 0 // here the carry is the one generated by nadd

The rules are:
(define_expand "neghi2"
   [(set (match_operand:HI 0 "register_operand")
         (neg:HI (match_operand:HI 1 "general_operand")))
    (clobber (reg:CC RCC))]
   ""
{
      rtx op0_low = gen_lowpart(QImode, operands[0]);
      rtx op0_high = gen_highpart(QImode, operands[0]);

      emit_move_insn(operands[0], operands[1]);
      emit_insn(gen_one_cmplqi2(op0_high, op0_high));
      emit_insn(gen_neghi_internal(op0_low, op0_high));
      DONE;
})

(define_insn_and_split "neghi_internal"
    [(set (match_operand:QI 0 "register_operand" "=c")
          (neg:QI (match_dup 0)))
     (set (match_operand:QI 1 "register_operand" "=c")
          (plus:QI
            (plus:QI
              (ltu:QI (neg:QI (match_dup 0)) (const_int 0))
              (match_dup 1))
            (const_int 0)))
     (clobber (reg:CC RCC))]
   ""
   "#"
   "reload_completed"
   [(const_int 0)]
{
   emit_insn(gen_negqi2(operands[0], operands[0]));
   emit_insn(gen_addc_internal(operands[1], operands[1], const0_rtx));
   DONE;
})


Everything looks ok until IRA:

(insn 11 81 12 3 (parallel [
             (set (reg:QI 81)
                 (xor:QI (reg:QI 73 [ uu ])
                     (const_int -1 [0xffffffff])))
             (clobber (reg:CC 13 CC))
         ]) _divdi3.i:76 37 {xorqi3}
      (expr_list:REG_DEAD (reg:QI 73 [ uu ])
         (expr_list:REG_UNUSED (reg:CC 13 CC)
             (nil))))

(insn 12 11 82 3 (parallel [
             (set (reg:QI 82 [+1 ])
                 (neg:QI (reg:QI 82 [+1 ])))
             (set (reg:QI 81)
                 (plus:QI (plus:QI (ltu:QI (neg:QI (reg:QI 82 [+1 ]))
                             (const_int 0 [0]))
                         (reg:QI 81))
                     (const_int 0 [0])))
             (clobber (reg:CC 13 CC))
         ]) _divdi3.i:76 46 {neghi_internal}
      (expr_list:REG_UNUSED (reg:CC 13 CC)
         (nil)))

The problem is that after IRA we have something like:
(insn 11 111 112 3 (parallel [
             (set (reg:QI 0 AH)
                 (xor:QI (reg:QI 0 AH)
                     (const_int -1 [0xffffffff])))
             (clobber (reg:CC 13 CC))
         ]) _divdi3.i:76 37 {xorqi3}
      (nil))

(insn 112 11 12 3 (parallel [
             (set (reg:QI 7 @H'fff8 [81])
                 (reg:QI 0 AH))
             (clobber (reg:CC 13 CC))
         ]) _divdi3.i:76 6 {*movqi}
      (nil))

(insn 12 112 82 3 (parallel [
             (set (reg:QI 1 AL [orig:82+1 ] [82])
                 (neg:QI (reg:QI 1 AL [orig:82+1 ] [82])))
             (set (reg:QI 3 X)
                 (plus:QI (plus:QI (ltu:QI (neg:QI (reg:QI 1 AL 
[orig:82+1 ] [82]))
                             (const_int 0 [0]))
                         (reg:QI 3 X))
                     (const_int 0 [0])))
             (clobber (reg:CC 13 CC))
         ]) _divdi3.i:76 46 {neghi_internal}
      (nil))


I actually expected reg 81 to be the same in insn 11 and insn 12. 
Instead IRA, puts reg 81 in AH, then moves it into FFF8 and uses X in 
insn 12 where reg 81 was expected. This is definitely not correct since 
the result of the xor won't be an argument to the add with carry (addc) 
and the final result is incorrect.

Am I missing something or something here is broken?

Cheers,

-- 
PMatos

Re: IRA changes rules of the game

[Ulrich Weigand]

Paulo J. Matos wrote:

> (define_insn_and_split "neghi_internal"
>     [(set (match_operand:QI 0 "register_operand" "=c")
>           (neg:QI (match_dup 0)))
>      (set (match_operand:QI 1 "register_operand" "=c")
>           (plus:QI
>             (plus:QI
>               (ltu:QI (neg:QI (match_dup 0)) (const_int 0))
>               (match_dup 1))
>             (const_int 0)))
>      (clobber (reg:CC RCC))]

> Am I missing something or something here is broken?

When reload looks at the above pattern, it will see just
two operands, both of which are output-only.  So when it
decides to reload one of the operands, it will only provide
an output reload, no input reload.

For operands that are actually used for both input and
output, you need to provide two match_operand clauses,
and tie them using a matching constraint.  Simply using
match_dup doesn't accomplish that.

Something along the lines of:

 (define_insn_and_split "neghi_internal"
     [(set (match_operand:QI 0 "register_operand" "=c")
           (neg:QI (match_operand:QI 1 "register_operand" "0")))
      (set (match_operand:QI 2 "register_operand" "=c")
           (plus:QI
             (plus:QI
               (ltu:QI (neg:QI (match_dup 1)) (const_int 0))
               (match_operand:QI 3 "register_operand" "2"))
             (const_int 0)))
      (clobber (reg:CC RCC))]

ought to work as expected.

(The remaining match_dup is fine, since reload already knows
operand 1 is used as input, so the dup doesn't introduce a
new type of use.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: IRA changes rules of the game

[Joern Rennecke]

Quoting Ulrich Weigand <uweigand@de.ibm.com>:

> Paulo J. Matos wrote:
>
>> (define_insn_and_split "neghi_internal"
>>     [(set (match_operand:QI 0 "register_operand" "=c")
>>           (neg:QI (match_dup 0)))
>>      (set (match_operand:QI 1 "register_operand" "=c")
>>           (plus:QI
>>             (plus:QI
>>               (ltu:QI (neg:QI (match_dup 0)) (const_int 0))
>>               (match_dup 1))
>>             (const_int 0)))
>>      (clobber (reg:CC RCC))]
>
>> Am I missing something or something here is broken?
>
> When reload looks at the above pattern, it will see just
> two operands, both of which are output-only.  So when it
> decides to reload one of the operands, it will only provide
> an output reload, no input reload.
>
> For operands that are actually used for both input and
> output, you need to provide two match_operand clauses,

Or just change the constraint to "+c" .

Re: IRA changes rules of the game

[Paulo J. Matos]

On 20/10/11 18:12, Joern Rennecke wrote:
>
> Or just change the constraint to "+c" .
>

After trying Ulrichs suggestion and getting it to work I decided to give 
yours a try since it looked cleaner using +c and dups elsewhere.

However, it failed to compile libgcc with:
../../../../../../../devHost/gcc46/gcc/libgcc/../gcc/libgcc2.c:272:1: 
internal compiler error: in df_uses_record, at df-scan.c:3178

This feels like a GCC bug. I will try to get a better look at it tomorrow.

-- 
PMatos

Re: IRA changes rules of the game

[Paulo J. Matos]

On 20/10/11 16:25, Ulrich Weigand wrote:
>
> When reload looks at the above pattern, it will see just
> two operands, both of which are output-only.  So when it
> decides to reload one of the operands, it will only provide
> an output reload, no input reload.
>
> For operands that are actually used for both input and
> output, you need to provide two match_operand clauses,
> and tie them using a matching constraint.  Simply using
> match_dup doesn't accomplish that.
>

I should have seen that. Perfect explanation. It works as expected now, 
thanks.

-- 
PMatos

Re: IRA changes rules of the game

[Paolo Bonzini]

On 10/20/2011 07:46 PM, Paulo J. Matos wrote:
>
> However, it failed to compile libgcc with:
> ../../../../../../../devHost/gcc46/gcc/libgcc/../gcc/libgcc2.c:272:1:
> internal compiler error: in df_uses_record, at df-scan.c:3178
>
> This feels like a GCC bug. I will try to get a better look at it tomorrow.

What's the SET it is failing on?

Paolo

Re: IRA changes rules of the game

[Paulo J. Matos]

On 21/10/11 10:02, Paolo Bonzini wrote:
> On 10/20/2011 07:46 PM, Paulo J. Matos wrote:
>>
>> However, it failed to compile libgcc with:
>> ../../../../../../../devHost/gcc46/gcc/libgcc/../gcc/libgcc2.c:272:1:
>> internal compiler error: in df_uses_record, at df-scan.c:3178
>>
>> This feels like a GCC bug. I will try to get a better look at it
>> tomorrow.
>
> What's the SET it is failing on?
>
> Paolo
>


This is a very strange insn indeed:
(set (ge (reg:QI 0 AH [orig:26 w ] [26])
         (const_int 0 [0]))
     (plus:QI (plus:QI (ltu:QI (reg:CC 13 CC)
                 (const_int 0 [0]))
             (lt (reg:QI 0 AH [orig:30 a ] [30])
                 (const_int 0 [0])))
         (const_int 0 [0])))

-- 
PMatos

Re: IRA changes rules of the game

[Joern Rennecke]

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

> This is a very strange insn indeed:
> (set (ge (reg:QI 0 AH [orig:26 w ] [26])
>         (const_int 0 [0]))
>     (plus:QI (plus:QI (ltu:QI (reg:CC 13 CC)
>                 (const_int 0 [0]))
>             (lt (reg:QI 0 AH [orig:30 a ] [30])
>                 (const_int 0 [0])))
>         (const_int 0 [0])))

Smells like rtl sharing.

Re: IRA changes rules of the game

[Richard Henderson]

On 10/20/2011 07:41 AM, Paulo J. Matos wrote:
> (define_insn_and_split "neghi_internal"
>    [(set (match_operand:QI 0 "register_operand" "=c")
>          (neg:QI (match_dup 0)))
>     (set (match_operand:QI 1 "register_operand" "=c")
>          (plus:QI
>            (plus:QI
>              (ltu:QI (neg:QI (match_dup 0)) (const_int 0))
>              (match_dup 1))
>            (const_int 0)))
>     (clobber (reg:CC RCC))]

This instruction is modeled incorrectly.

Either use match_operand instead of the match_dups for the
inputs, or use "+c" to indicate an in-out register.


r~