Question regarding constraint usage within inline asm

Question regarding constraint usage within inline asm

[Peter Bergner]

I have a question about constraint usage in inline asm when we have
an early clobber output operand.  The test case is from PR89313 and
looks like the code below (I'm using "r3" for the reg on ppc, but
you could also use "rax" on x86_64, etc.).

long input;
long
bug (void)
{
  register long output asm ("r3");
  asm ("blah %0, %1, %2" : "=&r" (output) : "r" (input), "0" (input));
  return output;
}

I know an input operand can have a matching constraint associated with
an early clobber operand, as there seems to be code that explicitly
mentions this scenario.  In this case, the user has to manually ensure
that the input operand is not clobbered by the early clobber operand.
In the case that the input operand uses an "r" constraint, we just
ensure that the early clobber operand and the input operand are assigned
different registers.  My question is, what about the case above where
we have the same variable being used for two different inputs with
constraints that seem to be incompatible?  Clearly, we cannot assign
a register to the "input" variable that is both the same and different
to the register that is assigned to "output".

Is this outright invalid to have "input" use both a matching and
non-matching constraint with an early clobber operand?  Or is is
expected that reload/LRA will come along and fix up the "r" usage
to use a different register?

My guess is that this is invalid usage and I have a patch to
expand_asm_stmt() to catch this, but it only works if we've
preassigned "output" to a hard register.  If this is truly
invalid, should I flag this even if "output" isn't preassigned?

If it is valid, then should match_asm_constraints_1() really rewrite
all of the uses of "input" with the register assigned to output as
it is doing now, which is what is causing the problems in LRA.
LRA sees that both input operands are using r3 and it catches the
constraint violation of the "r" input and tries to spill it, but
it's not a pseudo, but an explicit hard register already.  I'm not
sure LRA can really safely spill an operand that is an explicit hard
register.

Thoughts?

Peter

Re: Question regarding constraint usage within inline asm

[Alan Modra]

On Mon, Feb 18, 2019 at 01:13:31PM -0600, Peter Bergner wrote:
> I have a question about constraint usage in inline asm when we have
> an early clobber output operand.  The test case is from PR89313 and
> looks like the code below (I'm using "r3" for the reg on ppc, but
> you could also use "rax" on x86_64, etc.).
> 
> long input;
> long
> bug (void)
> {
>   register long output asm ("r3");
>   asm ("blah %0, %1, %2" : "=&r" (output) : "r" (input), "0" (input));
>   return output;
> }
> 
> I know an input operand can have a matching constraint associated with
> an early clobber operand, as there seems to be code that explicitly
> mentions this scenario.  In this case, the user has to manually ensure
> that the input operand is not clobbered by the early clobber operand.
> In the case that the input operand uses an "r" constraint, we just
> ensure that the early clobber operand and the input operand are assigned
> different registers.  My question is, what about the case above where
> we have the same variable being used for two different inputs with
> constraints that seem to be incompatible?

Without the asm("r3") gcc will provide your "blah" instruction with
one register for %0 and %2, and another register for %1.  Both
registers will be initialised with the value of "input".

>  Clearly, we cannot assign
> a register to the "input" variable that is both the same and different
> to the register that is assigned to "output".

No, you certainly can do that.  I think you have found a bug in lra.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Question regarding constraint usage within inline asm

[Peter Bergner]

On 2/19/19 9:09 PM, Alan Modra wrote:
> On Mon, Feb 18, 2019 at 01:13:31PM -0600, Peter Bergner wrote:
>> long input;
>> long
>> bug (void)
>> {
>>   register long output asm ("r3");
>>   asm ("blah %0, %1, %2" : "=&r" (output) : "r" (input), "0" (input));
>>   return output;
>> }
>>
>> I know an input operand can have a matching constraint associated with
>> an early clobber operand, as there seems to be code that explicitly
>> mentions this scenario.  In this case, the user has to manually ensure
>> that the input operand is not clobbered by the early clobber operand.
>> In the case that the input operand uses an "r" constraint, we just
>> ensure that the early clobber operand and the input operand are assigned
>> different registers.  My question is, what about the case above where
>> we have the same variable being used for two different inputs with
>> constraints that seem to be incompatible?
> 
> Without the asm("r3") gcc will provide your "blah" instruction with
> one register for %0 and %2, and another register for %1.  Both
> registers will be initialised with the value of "input".

That's not what I'm seeing.  I see one pseudo (123) used for the output
operand and one pseudo (121) used for both input operands.  Like so:

(insn 8 6 7 (parallel [
            (set (reg:DI 123 [ outputD.2831 ])
                (asm_operands:DI ("blah %0, %1, %2") ("=&r") 0 [
                        (reg/v:DI 121 [ <retval> ]) repeated x2
                    ]
                     [
                        (asm_input:DI ("r") bug.i:6)
                        (asm_input:DI ("0") bug.i:6)
                    ]
                     [] bug.i:6))
            (clobber (reg:SI 76 ca))
        ]) "bug.i":6:3 -1
     (nil))

The only difference between using asm("r3") and not using it is that
pseudo 123 is replaced with hard reg 3 in the output operand.  The input
operands use pseudo 121 in both cases.  It stays this way up until the
asmcons pass (ie, match_asm_constraints_1) which notices that operand %2
has a matching constraint with operand %0, so it emits a copy before
the asm that writes "input"'s pseudo into "output"'s pseudo and then
rewrites the asm operand %2 to use "output"'s pseudo.  But then it goes
ahead and rewrites all other uses of "input"'s pseudos with "output"'s
pseudo, so operand %1 also gets rewritten.  So we end up with:

(insn 15 6 8 2 (set (reg:DI 123 [ outputD.2831 ])
        (reg/v:DI 121 [ <retval> ])) "bug.i":6:3 -1
     (nil))
(insn 8 15 12 2 (parallel [
            (set (reg:DI 123 [ outputD.2831 ])
                (asm_operands:DI ("blah %0, %1, %2") ("=&r") 0 [
                        (reg:DI 123 [ outputD.2831 ]) repeated x2
                    ]
                     [
                        (asm_input:DI ("r") bug.i:6)
                        (asm_input:DI ("0") bug.i:6)
                    ]
                     [] bug.i:6))
            (clobber (reg:SI 76 ca))
        ]) "bug.i":6:3 -1
     (expr_list:REG_DEAD (reg/v:DI 121 [ <retval> ])
        (expr_list:REG_UNUSED (reg:SI 76 ca)
            (nil))))

Now the case above (ie, not using asm("r3")) compiles fine.  We assign
pseudo 123 to r3 and LRA's constraint checking code notices that operand
%1 should not be assigned to the same register as the early clobber
output operand, so it spills it.  However, when we use asm("r3"),
LRA's constraint checking code again sees that operand %1 shouldn't
have the same register as operand %0, but since it's a preassigned
hard register, it cannot spill it, since there may have been a valid
reason why that particular operand is supposed to be in r3, so we ICE.
I'm not sure we can ever safely spill a hard register.

That said, talking with Segher and Uli offline, they both think the
inline asm usage in the test case should be legal, so that tells me
then that the bug is in the asmcons pass when it rewrites operand %1's
pseudo.  It really should check that operand %1's pseudo should not
be updated because it conflicts with the early clobber operand %0.
That would then allow operand %1 and operand %2 to have different
registers.  I'll try and prepare a patch that checks for that scenario.

Peter

Re: Question regarding constraint usage within inline asm

[Segher Boessenkool]

On Wed, Feb 20, 2019 at 10:08:07AM -0600, Peter Bergner wrote:
> On 2/19/19 9:09 PM, Alan Modra wrote:
> > On Mon, Feb 18, 2019 at 01:13:31PM -0600, Peter Bergner wrote:
> >> long input;
> >> long
> >> bug (void)
> >> {
> >>   register long output asm ("r3");
> >>   asm ("blah %0, %1, %2" : "=&r" (output) : "r" (input), "0" (input));
> >>   return output;
> >> }

> > Without the asm("r3") gcc will provide your "blah" instruction with
> > one register for %0 and %2, and another register for %1.  Both
> > registers will be initialised with the value of "input".
> 
> That's not what I'm seeing.  I see one pseudo (123) used for the output
> operand and one pseudo (121) used for both input operands.  Like so:
> 
> (insn 8 6 7 (parallel [
>             (set (reg:DI 123 [ outputD.2831 ])
>                 (asm_operands:DI ("blah %0, %1, %2") ("=&r") 0 [
>                         (reg/v:DI 121 [ <retval> ]) repeated x2
>                     ]
>                      [
>                         (asm_input:DI ("r") bug.i:6)
>                         (asm_input:DI ("0") bug.i:6)
>                     ]
>                      [] bug.i:6))
>             (clobber (reg:SI 76 ca))
>         ]) "bug.i":6:3 -1
>      (nil))

expand already uses only one pseudo:

;; __asm__("blah %0, %1, %2" : "=&r" output : "r" input.0_1, "0" input.0_1);

(insn 7 6 0 (parallel [
            (set (reg/v:DI 3 3 [ output ])
                (asm_operands:DI ("blah %0, %1, %2") ("=&r") 0 [
                        (reg:DI 121 [ input.0_1 ]) repeated x2
                    ]
                     [
                        (asm_input:DI ("r") test.c:6)
                        (asm_input:DI ("0") test.c:6)
                    ]
                     [] test.c:6))
            (clobber (reg:SI 76 ca))
        ]) "test.c":6:3 -1
     (nil))

and that is a bad idea.  The asmcons pass makes pseudo 121 equal to hard
reg 3, and there is no way to recover from that.

Without the local register asm you get the same pseudo for the output as
well as both inputs, just as bad, but LRA can handle this:

            0 Early clobber: reject++
            0 Conflict early clobber reload: reject--
          alt=0,overall=6,losers=1,rld_nregs=0
         Choosing alt 0 in insn 8:  (0) =&r  (1) r  (2) 0
      Creating newreg=126 from oldreg=123, assigning class GENERAL_REGS to r126
    8: {r123:DI=asm_operands;clobber ca:SI;}
      REG_UNUSED ca:SI
    Inserting insn reload before:
   19: r126:DI=r123:DI

So expand shouldn't do this, but also asmcons should probably be improved


Segher

Re: Question regarding constraint usage within inline asm

[Alan Modra]

On Wed, Feb 20, 2019 at 10:08:07AM -0600, Peter Bergner wrote:
> On 2/19/19 9:09 PM, Alan Modra wrote:
> > On Mon, Feb 18, 2019 at 01:13:31PM -0600, Peter Bergner wrote:
> >> long input;
> >> long
> >> bug (void)
> >> {
> >>   register long output asm ("r3");
> >>   asm ("blah %0, %1, %2" : "=&r" (output) : "r" (input), "0" (input));
> >>   return output;
> >> }
> >>
> >> I know an input operand can have a matching constraint associated with
> >> an early clobber operand, as there seems to be code that explicitly
> >> mentions this scenario.  In this case, the user has to manually ensure
> >> that the input operand is not clobbered by the early clobber operand.
> >> In the case that the input operand uses an "r" constraint, we just
> >> ensure that the early clobber operand and the input operand are assigned
> >> different registers.  My question is, what about the case above where
> >> we have the same variable being used for two different inputs with
> >> constraints that seem to be incompatible?
> > 
> > Without the asm("r3") gcc will provide your "blah" instruction with
> > one register for %0 and %2, and another register for %1.  Both
> > registers will be initialised with the value of "input".
> 
> That's not what I'm seeing.  I see one pseudo (123) used for the output
> operand and one pseudo (121) used for both input operands.  Like so:

I meant by the time you get to assembly.

	blah 3, 9, 3

> That said, talking with Segher and Uli offline, they both think the
> inline asm usage in the test case should be legal

Good, it seems we are in agreement.  Incidentally, the single pseudo
for the inputs happens even for testcases like

long input;
long
bug (void)
{
  register long output /* asm ("r3") */;
  asm ("blah %0, %1, %2" : "=r" (output) : "wi" (input), "0" (input));
  return output;
}

-- 
Alan Modra
Australia Development Lab, IBM

Re: Question regarding constraint usage within inline asm

[Alan Modra]

I forgot to say, gcc-6, gcc-7 and gcc-8 handle your original testcase
with the register asm just fine.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Question regarding constraint usage within inline asm

[Peter Bergner]

On 2/20/19 4:19 PM, Alan Modra wrote:
> I forgot to say, gcc-6, gcc-7 and gcc-8 handle your original testcase
> with the register asm just fine.

Yes, because they don't have my IRA and LRA patches that exposed this
problem. I would say they were buggy for not complaining and silently
spilling a hard register in the case where we used asm reg("...").

Peter

Re: Question regarding constraint usage within inline asm

[Alan Modra]

On Wed, Feb 20, 2019 at 08:57:52PM -0600, Peter Bergner wrote:
> On 2/20/19 4:19 PM, Alan Modra wrote:
> > I forgot to say, gcc-6, gcc-7 and gcc-8 handle your original testcase
> > with the register asm just fine.
> 
> Yes, because they don't have my IRA and LRA patches that exposed this
> problem. I would say they were buggy for not complaining and silently
> spilling a hard register in the case where we used asm reg("...").

I don't follow your reasoning.  It seems to me that giving some
variable a register asm doesn't mean that the value of that variable
can't appear in some other register.  An obvious example is when
passing that variable to a function.

So why shouldn't a hard reg be reloaded in order to satisfy
incompatible constraints?

-- 
Alan Modra
Australia Development Lab, IBM

Re: Question regarding constraint usage within inline asm

[Peter Bergner]

On 2/20/19 9:39 PM, Alan Modra wrote:
> On Wed, Feb 20, 2019 at 08:57:52PM -0600, Peter Bergner wrote:
>> Yes, because they don't have my IRA and LRA patches that exposed this
>> problem. I would say they were buggy for not complaining and silently
>> spilling a hard register in the case where we used asm reg("...").
> 
> I don't follow your reasoning.  It seems to me that giving some
> variable a register asm doesn't mean that the value of that variable
> can't appear in some other register.  An obvious example is when
> passing that variable to a function.

I don't disagree with you here.  For sure, multiple registers can hold
the same value, the same that multiple variables can hold the same value.



> So why shouldn't a hard reg be reloaded in order to satisfy
> incompatible constraints?

About the only usage of register asm that is guaranteed, is their
usage in inline asm.  If you specify a hard register for a variable
and then use that variable in an inline asm, you are guaranteed
that that variable will use that register in the inline asm.
Now in this case, "input" doesn't have the register asm, but
asmcons rewrites the rtl such that it looks like "input" was
assigned via a register asm.

LRA doesn't know about register asms, it just sees pseudos and hard
registers, so I think it needs to be conservative and assume the
explicit hard registers it sees could have come from a register asm,
and not spill it, but rather error out and let the user fix it.

That said, the "bug" in the case we're seeing, is that asmcons
rewrote all of "input"'s pseudos, and it should be more careful
to not create rtl with illegal constraint usage that LRA cannot
fix up.  With the fix, operand %1 in the inline asm is no longer
hard coded to r3 and it uses the pseudo instead, so everything
is copacetic.

Peter

Re: Question regarding constraint usage within inline asm

[Segher Boessenkool]

On Thu, Feb 21, 2019 at 05:16:48PM -0600, Peter Bergner wrote:
> About the only usage of register asm that is guaranteed, is their
> usage in inline asm.  If you specify a hard register for a variable
> and then use that variable in an inline asm, you are guaranteed
> that that variable will use that register in the inline asm.

Yup.

> That said, the "bug" in the case we're seeing, is that asmcons
> rewrote all of "input"'s pseudos, and it should be more careful
> to not create rtl with illegal constraint usage that LRA cannot
> fix up.  With the fix, operand %1 in the inline asm is no longer
> hard coded to r3 and it uses the pseudo instead, so everything
> is copacetic.

And that expand used one pseudo for everything in the asm is bad already.
If two inputs have identical constraints, sure, then it makes sense perhaps
(but does it really safe anything anyway?)  Otherwise it does not.


Segher

Re: Question regarding constraint usage within inline asm

[Michael Matz]

Hi,

On Thu, 21 Feb 2019, Segher Boessenkool wrote:

> > That said, the "bug" in the case we're seeing, is that asmcons rewrote 
> > all of "input"'s pseudos, and it should be more careful to not create 
> > rtl with illegal constraint usage that LRA cannot fix up.  With the 
> > fix, operand %1 in the inline asm is no longer hard coded to r3 and it 
> > uses the pseudo instead, so everything is copacetic.
> 
> And that expand used one pseudo for everything in the asm is bad 
> already.

I'll contest that.  Asms aren't that special from an input-output 
perspective and shouldn't be (makes for clearer and hence more correct 
code).  hardregs are special, OTOH.  expand doesn't (and shouldn't) do 
anything special for

   _2 = _1 + _1;

(i.e. it should assign the same pseudo to both inputs), from which follows 
that it shouldn't do anything special for

  asm("" : "=r" (_2) : "r" (_1), "w" (_1))

either.  The picture changes if _1 corresponds to a local decl with an 
associated hard reg; those don't get SSA names for a reason.  But I 
believe you're worrying about the common case of off-the-mill SSA 
names.

Anything that would cause breakage by such behaviour of expand is to be 
rectified by LRA/reload, not by hackery within expand.

> If two inputs have identical constraints, sure, then it makes 
> sense perhaps (but does it really safe anything anyway?)  Otherwise it 
> does not.

The thing is that it doesn't make sense to do anything special for asms in 
this respect, and it actually goes against the natural flow of things to 
do anything special.  Think about how you would retain the necessary 
copies if we were to implement the restriction you want.  Say, given

  asm ("" :: "r" (_1), "w" (_1));

and you want to enforce that both inputs get different pseudo; so you 
start generating

  (set (p60) (p59))   // assume _1 sits in pseudo 59
  (set (p61) (p59))
  (asm (...) ("r" p60) ("w" p61))

How do you propose the (right now) obviously useless copies between two 
perfectly normal pseudos to be retained during the course of RTL 
optimization to not immediately end up with:

  (asm (...) ("r" p59) ("w" p59))

?  Sure, you could implement more of the hackery that looks at users of 
the copies and disable forwarding if they feed asms, but what for?  You 
can't remove LRA/reload code that deals with this situation anyway, so why 
not let it deal with it?  Another way to look at this is that LRA makes it 
so that those copies are implicitely always there but are only 
materialized if necessary because of conflicting constraints.

(Again the picture completely changes if one of the decls, and hence 
associated pseudo, has a hard-reg; and for that we do deal mostly 
correctly with it, except for the asmcons problem discussed here).


Ciao,
Michael.

Re: Question regarding constraint usage within inline asm

[Segher Boessenkool]

Hi!

On Mon, Feb 25, 2019 at 06:32:53PM +0000, Michael Matz wrote:
> On Thu, 21 Feb 2019, Segher Boessenkool wrote:
> > > That said, the "bug" in the case we're seeing, is that asmcons rewrote 
> > > all of "input"'s pseudos, and it should be more careful to not create 
> > > rtl with illegal constraint usage that LRA cannot fix up.  With the 
> > > fix, operand %1 in the inline asm is no longer hard coded to r3 and it 
> > > uses the pseudo instead, so everything is copacetic.
> > 
> > And that expand used one pseudo for everything in the asm is bad 
> > already.
> 
> I'll contest that.  Asms aren't that special from an input-output 
> perspective and shouldn't be (makes for clearer and hence more correct 
> code).  hardregs are special, OTOH.  expand doesn't (and shouldn't) do 
> anything special for
> 
>    _2 = _1 + _1;
> 
> (i.e. it should assign the same pseudo to both inputs), from which follows 
> that it shouldn't do anything special for
> 
>   asm("" : "=r" (_2) : "r" (_1), "w" (_1))
> 
> either.  The picture changes if _1 corresponds to a local decl with an 
> associated hard reg; those don't get SSA names for a reason.  But I 
> believe you're worrying about the common case of off-the-mill SSA 
> names.

Yup.  All good points, I didn't think this through enough obviously.

The _1+_1 isn't great if that single pseudo then ends up in mem (it will
need a reload again on most archs, probably causing another spill), btw.

> Anything that would cause breakage by such behaviour of expand is to be 
> rectified by LRA/reload, not by hackery within expand.

You get worse code that way.  Reload is much too late to make optimised
code, and that isn't its task anyway: its task is to make something that
works where the passes before it couldn't manage.

Yes, reload should be able to fix up anything (well, within some limits).
That is not an excuse to generate code that we know will need such fixups
though.

I suppose check_asm_operands needs to be taught a bit more strict rules
as well about what is a valid asm.  Hrm.  A lot to think about :-)


Segher

Re: Question regarding constraint usage within inline asm

[Michael Matz]

Hi,

On Mon, 25 Feb 2019, Segher Boessenkool wrote:

> Yup.  All good points, I didn't think this through enough obviously.
> 
> The _1+_1 isn't great if that single pseudo then ends up in mem (it will 
> need a reload again on most archs, probably causing another spill), btw.

But at least it'll get only a single reload, not two ...

> You get worse code that way.  Reload is much too late to make optimised
> code, and that isn't its task anyway: its task is to make something that
> works where the passes before it couldn't manage.

... but yes, if we're entering the optimization- (in difference to 
correctness-) territory, then sure, expand (or any other pass between it 
and reload) might want to handle some sitations in a special way to ensure 
good code with the least amount of work.

> Yes, reload should be able to fix up anything (well, within some 
> limits). That is not an excuse to generate code that we know will need 
> such fixups though.

(Within some limits :) ), I agree.


Ciao,
Michael.

Re: Question regarding constraint usage within inline asm

[Peter Bergner]

On 2/20/19 4:04 PM, Alan Modra wrote:
> On Wed, Feb 20, 2019 at 10:08:07AM -0600, Peter Bergner wrote:
>> On 2/19/19 9:09 PM, Alan Modra wrote:
>> That said, talking with Segher and Uli offline, they both think the
>> inline asm usage in the test case should be legal
> 
> Good, it seems we are in agreement.  Incidentally, the single pseudo
> for the inputs happens even for testcases like
> 
> long input;
> long
> bug (void)
> {
>   register long output /* asm ("r3") */;
>   asm ("blah %0, %1, %2" : "=r" (output) : "wi" (input), "0" (input));
>   return output;
> }

This is a different problem than I'm fixing, but you are correct that
asmcons shouldn't replace operand %1 since it has a non-compatible
constraint than the output operand.  In this case, it's probably "ok"
to spill even though it's a hard register, because it doesn't match
the regclass it is supposed to have.  I'm not sure how important
this is to fix.  It can also imagine that this would be hard to
handle, since we'd have to call into the backend to see whether the
two constraints are compatible and with the overlap between different
constraints, that could be very very messy!

Peter