llvm dev - Mar 2013 - [LLVMdev] Problems with 64-bit register operands of inline asm on ARM

r175088 attempted to fix gcc inline asm compatibility with 64-bit
operands by forcing these into even/odd register pairs the same way gcc
always allocates such values.

While the fix appears to work as such, it is not always activated when
required.  The patch makes the assumption that any inline asm statement
relying on the even/odd allocation will make use of the %Hn syntax to
reference the high (odd) register.  However, this is not the case.

A common pattern where llvm still fails is code using the abbreviated
syntax for LDRD and friends supported by gas:

    __asm__ ("ldrd %0, [%1]" : "=r"(a) : "r"(b));

Here the second destination register is implicitly one higher than the
first.  Because of this, the %H0 construct is never used, so the forced
even/odd allocation is skipped.

One possible fix, which I have tested, is to look for the specific
instructions requiring such a pair (LDRD/STRD and LDREXD/STREXD) in
addition to the 'H' modifier.  However, there are probably other
creative ways in which inline asm might rely on the specific pairing.
Thus I believe the safest solution is to always force 64-bit operands
into even/odd pairs for any inline asm.  In other words, we should
probably do something like this (untested):

--- a/lib/Target/ARM/ARMISelDAGToDAG.cpp
+++ b/lib/Target/ARM/ARMISelDAGToDAG.cpp
@@ -3457,19 +3457,6 @@ SDNode *ARMDAGToDAGISel::SelectInlineAsm(SDNode *N){
   bool Changed = false;
   unsigned NumOps = N->getNumOperands();
 
-  ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(
-      N->getOperand(InlineAsm::Op_AsmString));
-  StringRef AsmString = StringRef(S->getSymbol());
-
-  // Normally, i64 data is bounded to two arbitrary GRPs for "%r"
constraint.
-  // However, some instrstions (e.g. ldrexd/strexd in ARM mode) require
-  // (even/even+1) GPRs and use %n and %Hn to refer to the individual regs
-  // respectively. Since there is no constraint to explicitly specify a
-  // reg pair, we search %H operand inside the asm string. If it is found, the
-  // transformation below enforces a GPRPair reg class for "%r" for
64-bit data.
-  if (AsmString.find(":H}") == StringRef::npos)
-    return NULL;
-
   DebugLoc dl = N->getDebugLoc();
   SDValue Glue = N->getOperand(NumOps-1);
 


-- 
Måns Rullgård
mans at mansr.com

On 13 March 2013 13:43, Måns Rullgård <mans at mansr.com> wrote:
> One possible fix, which I have tested, is to look for the specific
> instructions requiring such a pair (LDRD/STRD and LDREXD/STREXD) in
> addition to the 'H' modifier.  However, there are probably other
> creative ways in which inline asm might rely on the specific pairing.
>
Hi Mans,

Either that method is ignoring an inline asm parser or there isn't one, but
I agree, we should be able to have something better than just grep for
possible extensions for paired registers.


Thus I believe the safest solution is to always force 64-bit
operands
> into even/odd pairs for any inline asm.  In other words, we should
> probably do something like this (untested):
>
I tested this, and it fails on other inline assembly tests. I think that
the non-paired asm is correctly selected by the table generated parser, but
when you pair things that didn't need pairing, the parser goes bad.

I don't think we should force pairing on every inline assembly either.
Maybe someone knows how to parse inline assembly in a better way than it is
currently being done? If there isn't any, than possibly creating a function
to return "needsPairedRegister()" or something would still be ugly,
but
better than incrementing it inline.

cheers,
--renato
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130313/5ac714b8/attachment.html>

Hi  Måns,
Always forcing 64-bit operands into even/odd pairs may lead to subpoptimal
register allocation because not all 64 bit data requires paired regs. It
seems there is no general pattern to match. Maybe we should treat it case by
case.

Jakob, do you have any suggestions?

Thanks,
Weiming


Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


-----Original Message-----
From: Måns Rullgård [mailto:mans at mansr.com] 
Sent: Wednesday, March 13, 2013 6:43 AM
To: llvmdev at cs.uiuc.edu
Cc: weimingz at codeaurora.org; renato.golin at linaro.org
Subject: Problems with 64-bit register operands of inline asm on ARM

r175088 attempted to fix gcc inline asm compatibility with 64-bit operands
by forcing these into even/odd register pairs the same way gcc always
allocates such values.

While the fix appears to work as such, it is not always activated when
required.  The patch makes the assumption that any inline asm statement
relying on the even/odd allocation will make use of the %Hn syntax to
reference the high (odd) register.  However, this is not the case.

A common pattern where llvm still fails is code using the abbreviated syntax
for LDRD and friends supported by gas:

    __asm__ ("ldrd %0, [%1]" : "=r"(a) : "r"(b));

Here the second destination register is implicitly one higher than the
first.  Because of this, the %H0 construct is never used, so the forced
even/odd allocation is skipped.

One possible fix, which I have tested, is to look for the specific
instructions requiring such a pair (LDRD/STRD and LDREXD/STREXD) in addition
to the 'H' modifier.  However, there are probably other creative ways in
which inline asm might rely on the specific pairing.
Thus I believe the safest solution is to always force 64-bit operands into
even/odd pairs for any inline asm.  In other words, we should probably do
something like this (untested):

--- a/lib/Target/ARM/ARMISelDAGToDAG.cpp
+++ b/lib/Target/ARM/ARMISelDAGToDAG.cpp
@@ -3457,19 +3457,6 @@ SDNode *ARMDAGToDAGISel::SelectInlineAsm(SDNode *N){
   bool Changed = false;
   unsigned NumOps = N->getNumOperands();
 
-  ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(
-      N->getOperand(InlineAsm::Op_AsmString));
-  StringRef AsmString = StringRef(S->getSymbol());
-
-  // Normally, i64 data is bounded to two arbitrary GRPs for "%r"
constraint.
-  // However, some instrstions (e.g. ldrexd/strexd in ARM mode) require
-  // (even/even+1) GPRs and use %n and %Hn to refer to the individual regs
-  // respectively. Since there is no constraint to explicitly specify a
-  // reg pair, we search %H operand inside the asm string. If it is found,
the
-  // transformation below enforces a GPRPair reg class for "%r" for
64-bit
data.
-  if (AsmString.find(":H}") == StringRef::npos)
-    return NULL;
-
   DebugLoc dl = N->getDebugLoc();
   SDValue Glue = N->getOperand(NumOps-1);
 


--
Måns Rullgård
mans at mansr.com

Hi Renato,

 

It seems to me that LLVM doesn’t parse the inline asm body. It just checks
the constraints, (ie. Input/output interface). During ASM writing, it then
binding those constraints to placeholders like %0, %1.  

So it a constraint is a 64-integer type, it *probably* needs paired GPR.

 

Weiming

 

 

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation

 

From: Renato Golin [mailto:renato.golin at linaro.org] 
Sent: Wednesday, March 13, 2013 10:03 AM
To: Måns Rullgård
Cc: LLVM Dev; weimingz at codeaurora.org
Subject: Re: Problems with 64-bit register operands of inline asm on ARM

 

On 13 March 2013 13:43, Måns Rullgård <mans at mansr.com> wrote:

One possible fix, which I have tested, is to look for the specific
instructions requiring such a pair (LDRD/STRD and LDREXD/STREXD) in
addition to the 'H' modifier.  However, there are probably other
creative ways in which inline asm might rely on the specific pairing.

 

Hi Mans,

 

Either that method is ignoring an inline asm parser or there isn't one, but
I agree, we should be able to have something better than just grep for
possible extensions for paired registers.

 

 

Thus I believe the safest solution is to always force 64-bit operands
into even/odd pairs for any inline asm.  In other words, we should
probably do something like this (untested):

 

I tested this, and it fails on other inline assembly tests. I think that the
non-paired asm is correctly selected by the table generated parser, but when
you pair things that didn't need pairing, the parser goes bad.

 

I don't think we should force pairing on every inline assembly either. Maybe
someone knows how to parse inline assembly in a better way than it is
currently being done? If there isn't any, than possibly creating a function
to return "needsPairedRegister()" or something would still be ugly,
but
better than incrementing it inline.

 

cheers,

--renato

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130313/b2e7d2a3/attachment.html>

On Mar 13, 2013, at 10:03 AM, Weiming Zhao <weimingz at codeaurora.org>
wrote:
> Hi  Måns,
> Always forcing 64-bit operands into even/odd pairs may lead to subpoptimal
> register allocation because not all 64 bit data requires paired regs. It
> seems there is no general pattern to match. Maybe we should treat it case
by
> case.
> 
> Jakob, do you have any suggestions?
I'm not sure I understand. How does one use unpaired registers in inline
asm? Could you give an example?

/jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130313/7aea9ac0/attachment.html>

Renato Golin <renato.golin at linaro.org> writes:
> On 13 March 2013 13:43, Måns Rullgård <mans at mansr.com> wrote:
>
>> One possible fix, which I have tested, is to look for the specific
>> instructions requiring such a pair (LDRD/STRD and LDREXD/STREXD) in
>> addition to the 'H' modifier.  However, there are probably
other
>> creative ways in which inline asm might rely on the specific pairing.
>>
>
> Hi Mans,
>
> Either that method is ignoring an inline asm parser or there isn't one,
but
> I agree, we should be able to have something better than just grep for
> possible extensions for paired registers.
>
>> Thus I believe the safest solution is to always force 64-bit operands
>> into even/odd pairs for any inline asm.  In other words, we should
>> probably do something like this (untested):
>>
>
> I tested this, and it fails on other inline assembly tests. I think that
> the non-paired asm is correctly selected by the table generated parser, but
> when you pair things that didn't need pairing, the parser goes bad.
>
> I don't think we should force pairing on every inline assembly either.
> Maybe someone knows how to parse inline assembly in a better way than it is
> currently being done? If there isn't any, than possibly creating a
function
> to return "needsPairedRegister()" or something would still be
ugly, but
> better than incrementing it inline.
Yes, ideally we should be able to look at the inline assembly and
determine, for each operand, whether it requires pairing.  The problem
is that gcc _always_ allocates an even/odd pair, and I don't want to
attempt enumerating and detecting every possible way this could be
relied on.  Searching for LDRD-family instructions is one thing, but I'm
sure there are other ways code could rely on the way gcc allocates
registers.

-- 
Måns Rullgård
mans at mansr.com