splitting const_int's

splitting const_int's

[Omar Torres]

Hi All,
  Since the architecture I am working with only support byte size moves,
I created a define_split break HI-move insn's into two  QI-moves.

For example, the following insn:
(insn 84 5 9 (set (reg:HI 1 %r3)
         (const_int 32767 [0x7fff])) 3 {*movhi} (nil)
     (nil))

I split it into:
(set (reg:QI 1 %r3)
      (const_int 127 [0x7f])
(set (reg:QI 2 %r2)
      (const_int 255 [0x255])

I expect that the simplified RTL will be detected by the movqi pattern:
(define_insn "movqi"
   [(set (match_operand:QI 0 "nonimmediate_operand" "=r,m")
	(match_operand:QI 1 "general_operand"      "g,r"))]
   ""
   "move\t%0,%1")


The split split is working as I expect, but the problem is that the
movqi is not recognizing the the second part of the move:
error: unrecognizable insn:
(insn 94 93 9 (set (reg:QI 2 %r2 [orig:1+1 ] [1])
         (const_int 255 [0xff])) -1 (nil)
     (nil))


After further investigating this, I was able find why the movqi is not
recognizing this patter. The const_int 255 fails this statement in the
general_operand predicate:
   if (GET_CODE (op) == CONST_INT
       && mode != VOIDmode
       && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
     return 0;
When I debug this I can see that trunc_int_for_mode is returning -1
while the INTVAL macro returns 255.

Any suggestions on how to fix this issue?

Below is the complete define_split I am referring above.

(define_split
   [(set (match_operand:HI 0 "nonimmediate_operand" "")
         (match_operand:HI 1 "general_operand"      ""))]
   "reload_completed
    && REG_P (operands[0])"
[(set (match_dup 2) (match_dup 4))
  (set (match_dup 3) (match_dup 5))]
  "{
   gcc_assert (REG_P (operands[0]));
   operands[2] = gen_highpart(QImode, operands[0]);
   operands[3] = gen_lowpart (QImode, operands[0]);

   if (REG_P (operands[1])) {
       operands[4] = gen_highpart (QImode, operands[1]);
       operands[5] = gen_lowpart  (QImode, operands[1]);
   }
   else if (MEM_P (operands[1]) || CONST == GET_CODE (operands[1])) {
       int ioff;
       rtx base, off;
       base = XEXP (operands[1],0);
       off  = gen_rtx_CONST_INT (VOIDmode, 0);
       if (CONST == GET_CODE (base)) {
           base = XEXP(base,0);
       }
       if (PLUS == GET_CODE (base)) {
        off  = XEXP (base,1);
        base = XEXP (base,0);
       }
       gcc_assert (REG == GET_CODE (base)
                   && CONST_INT == GET_CODE (off));
       ioff = INTVAL (off);
       gcc_assert (254 >= ioff
                   && 0 <= ioff);
       operands[4] = gen_rtx_MEM (QImode,
                                  gen_rtx_PLUS (Pmode,
                                                base,
                                                gen_rtx_CONST_INT 		
                                            (QImode, ioff + 1)));
       operands[5] = gen_rtx_MEM (QImode,
                                  gen_rtx_PLUS (Pmode,
                                                base,

gen_rtx_CONST_INT(QImode, ioff)));
   }
   else if (LABEL_REF == GET_CODE (operands[1])
          || SYMBOL_REF == GET_CODE (operands[1])) {
       operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
       operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);
   }
   else if (CONST_INT == GET_CODE (operands[1])) {
       int val = INTVAL (operands[1]);
       int low = val & 0xff;
       int high = (val>>8) & 0xff;
       operands[4] = gen_rtx_CONST_INT (QImode, high);
       operands[5] = gen_rtx_CONST_INT (QImode, low);
   }
   else {
       fprintf (stderr, \"\nUnrecongnized:\");
       debug_rtx (operands[1]);
   }
  }")


Thanks,
-Omar

Re: splitting const_int's

[Eric Botcazou]

> Any suggestions on how to fix this issue?

gen_int_mode

-- 
Eric Botcazou

Re: splitting const_int's

[Richard Sandiford]

Just a few suggestions for simplfying this...

"Omar Torres" <gcc.omar@gmail.com> writes:
> (define_split
>    [(set (match_operand:HI 0 "nonimmediate_operand" "")
>          (match_operand:HI 1 "general_operand"      ""))]
>    "reload_completed
>     && REG_P (operands[0])"

You might as well make the first operand a register_operand and
avoid the REG_P check.  More importantly:

>        operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
>        operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);

...this code is designed to handle REGs and CONST_INTs correctly,
and avoid the problem you were seeing.  (As Eric says, gen_int_mode
is the canonical way of generating a correct CONST_INT in cases where
you do need to create one manually.  In this case it's simpler to use
simplify_gen_subreg though.)

I notice you're generating subregs of symbolic constants,
but I'm not sure that's really supported.

As far as the MEM_P case goes:

>    else if (MEM_P (operands[1]) || CONST == GET_CODE (operands[1])) {
>        int ioff;
>        rtx base, off;
>        base = XEXP (operands[1],0);
>        off  = gen_rtx_CONST_INT (VOIDmode, 0);
>        if (CONST == GET_CODE (base)) {
>            base = XEXP(base,0);
>        }
>        if (PLUS == GET_CODE (base)) {
>         off  = XEXP (base,1);
>         base = XEXP (base,0);
>        }
>        gcc_assert (REG == GET_CODE (base)
>                    && CONST_INT == GET_CODE (off));
>        ioff = INTVAL (off);
>        gcc_assert (254 >= ioff
>                    && 0 <= ioff);
>        operands[4] = gen_rtx_MEM (QImode,
>                                   gen_rtx_PLUS (Pmode,
>                                                 base,
>                                                 gen_rtx_CONST_INT 		
>                                             (QImode, ioff + 1)));
>        operands[5] = gen_rtx_MEM (QImode,
>                                   gen_rtx_PLUS (Pmode,
>                                                 base,
>
> gen_rtx_CONST_INT(QImode, ioff)));
>    }

you can use:

    operands[4] = adjust_address (operands[1], QImode, 1);
    operands[5] = adjust_address (operands[1], QImode, 0);

The CONST handling looks suspicious here.  CONSTs aren't memory references,
but you split them into memory references.

Also, you need to beware of cases in which operands[1] overlaps
operands[0].  The splitter says:

 [(set (match_dup 2) (match_dup 4))
  (set (match_dup 3) (match_dup 5))]

and operands[2] is always the highpart:

   operands[2] = gen_highpart(QImode, operands[0]);

but consider the case in which operands[1] (and thus operands[4])
is a memory reference that uses the high part of operands[0] as
a base register.  In that case, the base register will be modified
by the first split instruction and have the wrong value in the
second split instruction.  See other ports for the canonical way
of handling this.

Richard

Re: splitting const_int's

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> "Omar Torres" <gcc.omar@gmail.com> writes:
>> Richard Sandiford wrote:
>>> Also, you need to beware of cases in which operands[1] overlaps
>>> operands[0].  The splitter says:
>>>
>>>  [(set (match_dup 2) (match_dup 4))
>>>   (set (match_dup 3) (match_dup 5))]
>>>
>>> and operands[2] is always the highpart:
>>>
>>>    operands[2] = gen_highpart(QImode, operands[0]);
>>>
>>> but consider the case in which operands[1] (and thus operands[4])
>>> is a memory reference that uses the high part of operands[0] as
>>> a base register.  In that case, the base register will be modified
>>> by the first split instruction and have the wrong value in the
>>> second split instruction.  See other ports for the canonical way
>>> of handling this.
>>>
>>> Richard
>>
>> By looking at other ports, I learned that I can detect when this happens
>> by using the reg_overlap_mentioned_p(). Here is one case:
>> (insn 43 115 74 (set (reg:HI 7 %i0h)
>>          (mem/s/j:HI (plus:HI (reg/f:HI 7 %i0h [orig:39 source ] [39])
>>                  (const_int 2 [0x2])) [0 <variable>.r+0 S2 A8])) 3
>> {*movhi} (nil)
>>      (nil))
>>
>>    I need to tell the compiler not to use as destination the same base
>> register when doing index operations. Any suggestions on how do I that?
>
> Hmm.  If one destination register is the base and the other is the
> index, you're probably best off adding them together and freeing up
> a register that way.  Hopefully it'll be a rare case.
>
> Because you're doing this in a splitter, some of the later
> rtl optimisers _might_ be able to get rid of the addition,
> but I wouldn't bet on it.

Sorry, this was the same thing Paul suggested.  Doesn't seem
as hacky to me as it did to him ;)

Richard

Re: splitting const_int's

[Richard Sandiford]

"Omar Torres" <gcc.omar@gmail.com> writes:
> Richard Sandiford wrote:
>> Also, you need to beware of cases in which operands[1] overlaps
>> operands[0].  The splitter says:
>>
>>  [(set (match_dup 2) (match_dup 4))
>>   (set (match_dup 3) (match_dup 5))]
>>
>> and operands[2] is always the highpart:
>>
>>    operands[2] = gen_highpart(QImode, operands[0]);
>>
>> but consider the case in which operands[1] (and thus operands[4])
>> is a memory reference that uses the high part of operands[0] as
>> a base register.  In that case, the base register will be modified
>> by the first split instruction and have the wrong value in the
>> second split instruction.  See other ports for the canonical way
>> of handling this.
>>
>> Richard
>
> By looking at other ports, I learned that I can detect when this happens
> by using the reg_overlap_mentioned_p(). Here is one case:
> (insn 43 115 74 (set (reg:HI 7 %i0h)
>          (mem/s/j:HI (plus:HI (reg/f:HI 7 %i0h [orig:39 source ] [39])
>                  (const_int 2 [0x2])) [0 <variable>.r+0 S2 A8])) 3
> {*movhi} (nil)
>      (nil))
>
>    I need to tell the compiler not to use as destination the same base
> register when doing index operations. Any suggestions on how do I that?

Hmm.  If one destination register is the base and the other is the
index, you're probably best off adding them together and freeing up
a register that way.  Hopefully it'll be a rare case.

Because you're doing this in a splitter, some of the later
rtl optimisers _might_ be able to get rid of the addition,
but I wouldn't bet on it.

Richard

Re: splitting const_int's

[Andrew Pinski]

On Tue, May 20, 2008 at 2:47 PM, Paul Brook <paul@codesourcery.com> wrote:
> Does this work reliably for straight mov patterns and during reload? Sounds
> like in the general case it would need secondary reloads, which is a whole
> lot of extra magic.

Hmm, right.  Maybe something like what rs6000 does:
; List r->r after r->"o<>", otherwise reload will try to reload a
; non-offsettable address by using r->r which won't make progress.
(define_insn "*movdi_internal32"
  [(set (match_operand:DI 0 "nonimmediate_operand" "=o<>,r,r,*f,*f,m,r")
        (match_operand:DI 1 "input_operand" "r,r,m,f,m,f,IJKnGHF"))]



And then in the split it does:
      /* Move register range backwards, if we might have destructive
         overlap.  */
in rs6000_split_multireg_move.

-- Pinski

Re: splitting const_int's

[Paul Brook]

On Tuesday 20 May 2008, Andrew Pinski wrote:
> On Tue, May 20, 2008 at 2:30 PM, Omar Torres <gcc.omar@gmail.com> wrote:
> > By looking at other ports, I learned that I can detect when this happens
> > by using the reg_overlap_mentioned_p(). Here is one case:
> > (insn 43 115 74 (set (reg:HI 7 %i0h)
> >         (mem/s/j:HI (plus:HI (reg/f:HI 7 %i0h [orig:39 source ] [39])
> >                 (const_int 2 [0x2])) [0 <variable>.r+0 S2 A8])) 3
> > {*movhi} (nil)
> >     (nil))
> >
> >   I need to tell the compiler not to use as destination the same base
> > register when doing index operations. Any suggestions on how do I that?
>
> Early clobber, that is using "=&r" as the constraint.

Does this work reliably for straight mov patterns and during reload? Sounds 
like in the general case it would need secondary reloads, which is a whole 
lot of extra magic.

I remember having similar problems with ARM doubleword loads and ended up 
having to hack round the restriction in the asm output routines, and outlaw 
some of the more problematic problematic autoincrement modes.

Paul

Re: splitting const_int's

[Andrew Pinski]

On Tue, May 20, 2008 at 2:30 PM, Omar Torres <gcc.omar@gmail.com> wrote:
> By looking at other ports, I learned that I can detect when this happens
> by using the reg_overlap_mentioned_p(). Here is one case:
> (insn 43 115 74 (set (reg:HI 7 %i0h)
>         (mem/s/j:HI (plus:HI (reg/f:HI 7 %i0h [orig:39 source ] [39])
>                 (const_int 2 [0x2])) [0 <variable>.r+0 S2 A8])) 3
> {*movhi} (nil)
>     (nil))
>
>   I need to tell the compiler not to use as destination the same base
> register when doing index operations. Any suggestions on how do I that?

Early clobber, that is using "=&r" as the constraint.  Though may I
suggest that you split before reload/ra.

-- Pinski

Re: splitting const_int's

[Omar Torres]

Richard Sandiford wrote:
> Also, you need to beware of cases in which operands[1] overlaps
> operands[0].  The splitter says:
>
>  [(set (match_dup 2) (match_dup 4))
>   (set (match_dup 3) (match_dup 5))]
>
> and operands[2] is always the highpart:
>
>    operands[2] = gen_highpart(QImode, operands[0]);
>
> but consider the case in which operands[1] (and thus operands[4])
> is a memory reference that uses the high part of operands[0] as
> a base register.  In that case, the base register will be modified
> by the first split instruction and have the wrong value in the
> second split instruction.  See other ports for the canonical way
> of handling this.
>
> Richard

By looking at other ports, I learned that I can detect when this happens
by using the reg_overlap_mentioned_p(). Here is one case:
(insn 43 115 74 (set (reg:HI 7 %i0h)
         (mem/s/j:HI (plus:HI (reg/f:HI 7 %i0h [orig:39 source ] [39])
                 (const_int 2 [0x2])) [0 <variable>.r+0 S2 A8])) 3
{*movhi} (nil)
     (nil))

   I need to tell the compiler not to use as destination the same base
register when doing index operations. Any suggestions on how do I that?

Thanks for your help.
-Omar

Re: splitting const_int's

[Omar Torres]

Richard Sandiford wrote:
> "Omar Torres" <gcc.omar@gmail.com> writes:
> More importantly:
>
>>        operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
>>        operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);
>
> ..this code is designed to handle REGs and CONST_INTs correctly,
> and avoid the problem you were seeing.  (As Eric says, gen_int_mode
> is the canonical way of generating a correct CONST_INT in cases where
> you do need to create one manually.  In this case it's simpler to use
> simplify_gen_subreg though.)

Now, the insn:
(insn 84 5 9 (set (reg:HI 1 %r3)
         (const_int 32767 [0x7fff])) 3 {*movhi} (nil)
     (nil))

get's split into
  (set (reg:QI 1 %r3)
       (const_int 127 [0x7f])
  (set (reg:QI 2 %r2)
       (const_int -1 [0xffffffff])

and my movqi patter is able to match both of this patterns as expected.

But, now the output asm is
  move %r2,#0xffffffff
Instead of the expected:
  move %r2,#0xff

I am not fully familiar with how to fix this, but seems like there are
at least two ways of fixing this:
  1- Detect the sign in the rtl template and manipulate the operand so
that the asm value is outputted. That is, use something like this:

(define_insn "movqi"
   [(set (match_operand:QI 0 "nonimmediate_operand" "=r,m")
	(match_operand:QI 1 "general_operand"      "g,r"))]
   ""
   "*
   if (CONST_INT == GET_CODE (operands[1])) {
     if (0 > INTVAL(operands[1])) {
       fprintf (stderr, \"\nOperand 1 Value: %d, %d\", INTVAL
(operands[1]),0xff & INTVAL (operands[1]) );
       operands[1] = gen_rtx_CONST_INT (VOIDmode, 0xff & INTVAL
(operands[1]));
     }
   }
  return \"move.a\t%0,%1\";
  ")

  2- Use a special letter in the asm template, to communicate with the
print_operand() function.

  Is there any other way of doing this? If not, is there any advantage
of using one option over the other above?

Thanks,
-Omar

Re: splitting const_int's

[Omar Torres]

Richard Sandiford wrote:
>
> You might as well make the first operand a register_operand and
> avoid the REG_P check.
  I agree. I implemented this change and it works as expected.

>More importantly:
>
>>        operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
>>        operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);
>
> ..this code is designed to handle REGs and CONST_INTs correctly,
> and avoid the problem you were seeing.  (As Eric says, gen_int_mode
> is the canonical way of generating a correct CONST_INT in cases where
> you do need to create one manually.  In this case it's simpler to use
> simplify_gen_subreg though.)
  I modified the code as below and now I do not have the problems I had
before.

   else if (CONST_INT == GET_CODE (operands[1])
          || REG_P (operands[1])) {
       operands[4] = simplify_gen_subreg(QImode,operands[1],HImode,0);
       operands[5] = simplify_gen_subreg(QImode,operands[1],HImode,1);
   }

> I notice you're generating subregs of symbolic constants,
> but I'm not sure that's really supported.
   I did this so that I can generate rtl code that I can latter trap
with the define_insn's below.

(define_insn "movqi_hiword"
  [(set (match_operand:QI 0 "register_operand" "")
        (subreg:QI (match_operand:HI 1 "general_operand" "") 0))]
  ""
  "move\t%0,#HIWORD(%1)" )

(define_insn "*movqi_lowword"
  [(set (match_operand:QI 0 "register_operand" "")
        (subreg:QI (match_operand:HI 1 "general_operand" "") 1))]
  ""
  "move\t%0,#LOWWORD(%1)" )

  Do you recommend a better way to do this?



> As far as the MEM_P case goes:
[...]
>
> you can use:
>
>     operands[4] = adjust_address (operands[1], QImode, 1);
>     operands[5] = adjust_address (operands[1], QImode, 0);
>
  I also implemented this and it works nicely. Many thanks!

> The CONST handling looks suspicious here.  CONSTs aren't memory references,
> but you split them into memory references.
  In my debugging session, all cases I say CONSTs they were used for
memory. I will look at this in more detail.


> Also, you need to beware of cases in which operands[1] overlaps
> operands[0].  The splitter says:
>
>  [(set (match_dup 2) (match_dup 4))
>   (set (match_dup 3) (match_dup 5))]
>
> and operands[2] is always the highpart:
>
>    operands[2] = gen_highpart(QImode, operands[0]);
>
> but consider the case in which operands[1] (and thus operands[4])
> is a memory reference that uses the high part of operands[0] as
> a base register.  In that case, the base register will be modified
> by the first split instruction and have the wrong value in the
> second split instruction.  See other ports for the canonical way
> of handling this.

Thanks for the heads up. Could you point me to a specific target/file?


Best regards,
-Omar

Re: splitting const_int's

[Omar Torres]

Just correcting a typo...

Omar Torres wrote:
> I split it into:
  (set (reg:QI 1 %r3)
       (const_int 127 [0x7f])
  (set (reg:QI 2 %r2)
       (const_int 255 [0xFF])