[ARM] Add support for the X asm constraint

[sbaranga]

Summary

This patch adds support for the X asm constraint.

To do this, we lower the constraint to either a "w" or "r" constraint
depending on the operand type (both constraints are supported on ARM).

Fixes PR26493

[rengolin]

Hi Silviu,

The documentation on the X constraint is... lacking. All it says is "Any operand whatsoever is allowed.". Sigh...

So, before we get support for it, I'd like to see a few examples from user code on how people expect it to work (probably the best we can do for now).

Also, you don't seem to be testing much. I think you need to have better instructions (adds, movs, loads) to make sure we're testing the whole range of the X constraint. I'm guessing *any* operand is a bit vague, but does it support immediates, as well as registers, expressions?

Maybe it'd be easier to create a bug, block PR27197 with it, and explain the expected behaviour, with reproducible C files and command lines.

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp

11502

What about VFP?

11509

No "t" for 32-bit FP registers?

test/CodeGen/ARM/inlineasm-X-constraint.ll

2

why do you need -no-integrated-as?

[sbaranga]

In D19061#399877, @rengolin wrote:

Hi Silviu,

The documentation on the X constraint is... lacking. All it says is "Any operand whatsoever is allowed.". Sigh...

Hi Renato,

Our definition is "Allows an operand of any kind, no constraint whatsoever. Typically useful to pass a label for an asm branch or call." (LangRef.rst).
So basically we're free to choose.

FWIW, our approach is similar to the X86 one.

So, before we get support for it, I'd like to see a few examples from user code on how people expect it to work (probably the best we can do for now).

The PR (https://llvm.org/bugs/show_bug.cgi?id=26493) text has some good pointers:
http://www.ethernut.de/en/documents/arm-inline-asm.html

Also, you don't seem to be testing much. I think you need to have better instructions (adds, movs, loads) to make sure we're testing the whole range of the X constraint. I'm guessing *any* operand is a bit vague, but does it support immediates, as well as registers, expressions?

I'll add more tests. I think the types of the operands in these instructions are more important than the instructions themselves.

Maybe it'd be easier to create a bug, block PR27197 with it, and explain the expected behaviour, with reproducible C files and command lines.

The only use case of this that I'm aware of is the one from http://www.ethernut.de/en/documents/arm-inline-asm.html.

Cheers,
Silviu

lib/Target/ARM/ARMISelLowering.cpp

11502

Good point.

11509

Why "t" (wouldn't "w" be enough)?

test/CodeGen/ARM/inlineasm-X-constraint.ll

2

I don't, I'll just remove this.

[rengolin]

Do'h! I missed the PR reference, my apologies.

Though, wrt the blog post, I think it's a very lean source for such a generic case like this. I suggest you try to use the constraint on GCC and come up with a general description for its behaviour, what's allowed and what's not. Maybe even talk to a few GCC engineers once you have a better idea, and then be able to accurately describe what we want to implement.

Without that, it'll be hard to review the patch, and adding a hack to make your specific use case pass wouldn't be very constructive, either. :)

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp

11509

"t" would select an S register, while "w" would select a D reg.

If this is "anything", shouldn't it also allow immediate values and expressions?

[sbaranga]

In D19061#400041, @rengolin wrote:

Do'h! I missed the PR reference, my apologies.

Though, wrt the blog post, I think it's a very lean source for such a generic case like this. I suggest you try to use the constraint on GCC and come up with a general description for its behaviour, what's allowed and what's not. Maybe even talk to a few GCC engineers once you have a better idea, and then be able to accurately describe what we want to implement.

Without that, it'll be hard to review the patch, and adding a hack to make your specific use case pass wouldn't be very constructive, either. :)

cheers,
--renato

Sure!

Cheers,
Silviu

lib/Target/ARM/ARMISelLowering.cpp

11509

Our 'w' matching strategy would also match SPRs, DPRs and QPRs (see getRegForInlineAsmConstraint), based on the size of ConstraintVT. If that fails, the generic getRegForInlineAsmConstraint will still be able to give this a register class.

It does need a regression test though.

Regarding immediates/expressions: at least our implementation doesn't allow it (on any targets). I'll check what gcc does.

[jgreenhalgh]

Decaying this to "w" or "r" would potentially pessimize code generation, and might well break use cases. The whole point of "X" is to prevent the compiler from having to reload an operand you don't actually care about using in the output assembly (a scratch, or in that blog post, a hidden dependency). As GCC isn't going to put any effort in to forcing the form of the operand, I'd expect most uses that actually try to print out an "X" constrained register to be using it as a shorthand for getting constants or labels out. There are normally more expressive constraints if that is what you need.

[sbaranga]

Thanks, James!

The current implementation will only perform the lowering part (and force this to a register by converting to "r" or "w") if the operand is not a basic block, function, or integer constant - so this should cover some of those use cases.

It is true that GCC would be more efficient in some cases (one example would be FP constants), but we would still fit into the definition of "no constraint whatsoever" and therefore correct - which is an improvement from the current situation, where we'll simply crash on this constraint.

We can still improve our handling of this constraint, but that would be something that is target independent and outside the scope of this patch.

Thanks,
Silviu

[rengolin]

In D19061#401361, @sbaranga wrote:

It is true that GCC would be more efficient in some cases (one example would be FP constants), but we would still fit into the definition of "no constraint whatsoever" and therefore correct - which is an improvement from the current situation, where we'll simply crash on this constraint.

I agree bad codegen trumps ICE crashes, but James mentioned it "might well break use cases". I'm interested in those...

James, do you have some pointers on the expected usage of this constraint in the wild? The more the merrier!

cheers,
--renato

[jgreenhalgh]

In D19061#402576, @rengolin wrote:

In D19061#401361, @sbaranga wrote:

It is true that GCC would be more efficient in some cases (one example would be FP constants), but we would still fit into the definition of "no constraint whatsoever" and therefore correct - which is an improvement from the current situation, where we'll simply crash on this constraint.

I agree bad codegen trumps ICE crashes, but James mentioned it "might well break use cases". I'm interested in those...

James, do you have some pointers on the expected usage of this constraint in the wild? The more the merrier!

A Debian grep gets a few examples out from x86 mmx headers (and a couple for other architectures) https://codesearch.debian.net/results/%22%3DX%22/page_0

I'm making no promises as to how correct those uses are (http://sources.debian.net/src/php7.0/7.0.5-3/Zend/zend_multiply.h/?hl=56#L56 looks pretty fragile!), but I don't know of any better sources for real world code. I was only guessing this would break somebody's use case (because it is so liberal at the moment), rather than having one in particular in mind.

[rengolin]

In D19061#402641, @jgreenhalgh wrote:

A Debian grep gets a few examples out from x86 mmx headers (and a couple for other architectures) https://codesearch.debian.net/results/%22%3DX%22/page_0

Hey, I didn't know about this code search, nice!

Silviu, it'd be good to get a few examples from that search result page and at least understand what's going on, and maybe prepare some better error messages than seg faults.

I'm making no promises as to how correct those uses are (http://sources.debian.net/src/php7.0/7.0.5-3/Zend/zend_multiply.h/?hl=56#L56 looks pretty fragile!), but I don't know of any better sources for real world code. I was only guessing this would break somebody's use case (because it is so liberal at the moment), rather than having one in particular in mind.

Crap. That example can go wrong in so many ways... The complete lack of type safety makes that macro a nightmare for compilers... sigh...

I think we can retain our current approach that, whatever makes sense, passes, and what doesn't breaks with an error message in the lines of "stupid user".

--renato

[sbaranga]

In D19061#402662, @rengolin wrote:

In D19061#402641, @jgreenhalgh wrote:

A Debian grep gets a few examples out from x86 mmx headers (and a couple for other architectures) https://codesearch.debian.net/results/%22%3DX%22/page_0

Hey, I didn't know about this code search, nice!

Silviu, it'd be good to get a few examples from that search result page and at least understand what's going on, and maybe prepare some better error messages than seg faults.

Yeah, I'm currently working through a list of additional use cases to see what works and why other things don't work.
Printing integer immediates and function addresses works.
For labels, we hit an issue where the label's basic block is not considered to have its address taken which can cause the basic block that's being pointed to by the label to be removed (at least in some cases), resulting in invalid output. I'll raise a PR for that.

I'm making no promises as to how correct those uses are (http://sources.debian.net/src/php7.0/7.0.5-3/Zend/zend_multiply.h/?hl=56#L56 looks pretty fragile!), but I don't know of any better sources for real world code. I was only guessing this would break somebody's use case (because it is so liberal at the moment), rather than having one in particular in mind.

Crap. That example can go wrong in so many ways... The complete lack of type safety makes that macro a nightmare for compilers... sigh...

I think we can retain our current approach that, whatever makes sense, passes, and what doesn't breaks with an error message in the lines of "stupid user".

You'll at least get an assembler error, which is much nicer.

Cheers,
Silviu

[sbaranga]

Added further regression tests.

Only use a 'w' constraint if we know that we have at least VFPv2.

[sbaranga]

Ping?

[rengolin]

Hi Silviu,

I'm ok with this approach, since it's simple and seems to cover the cases pretty well. I'm guessing that the error message when the prediction goes wrong is "invalid constraint for asm operand", which is *much* better than a segfault. Also, confusion around macros are really the user's fault for writing such an egregious code in the first place.

It'd be good if James could have a final look, but I'm ok with the change.

cheers,
--renato

test/CodeGen/ARM/inlineasm-X-constraint.ll

5

shouldn't you verify that the register used in vmsr is also used in vadd?

Same comment for other CHECK lines below.

[sbaranga]

test/CodeGen/ARM/inlineasm-X-constraint.ll

5

The vadd operand ($0) doesn't appear in the vmsr instruction. It's there just to add the dependency (which is the usecase shown in the PR).

[jgreenhalgh]

It'd be good if James could have a final look, but I'm ok with the change.

I think this will be fine. Missing the constants and labels might come back to bite you (I found an example of s390 using the "X" constraint in the kernel [ arch/s390/include/asm/jump_label.h ] to get at a label for hotpatching), but the break is no worse than you have at the moment (and that's s390, I didn't see anything scary in arch/arm or arch/arm64).

If you're happy to be more restrictive than GCC here, then I think think the patch is fine.

Please heavily comment the testcases to reflect that they are not representative of the way the "X" constraint should be used in the real world, and you may want to add further comments to the lowering function to make it clear that it is tighter than neccessary.

[rengolin]

In D19061#410492, @jgreenhalgh wrote:

If you're happy to be more restrictive than GCC here, then I think think the patch is fine.

We're always happy to be more restrictive than GCC. :)

Please heavily comment the testcases to reflect that they are not representative of the way the "X" constraint should be used in the real world, and you may want to add further comments to the lowering function to make it clear that it is tighter than neccessary.

Indeed, please do! Also, if you could add the asm() statement that generated each example and a short explanation where the match is binding to, it'd be most helpful, given that you can't match the parameter in some cases.

With the additional comments in the test, LGTM. Thanks!

test/CodeGen/ARM/inlineasm-X-constraint.ll

5

Right, ok.

[sbaranga]

Thanks! Yes, I think being more restrictive than GCC should be fine at least for now.

[sbaranga]

Add more comments for the regresion tests (explaining what's the
tests are doint and what's going on when we fail to emit constants/
labels).

Also add some additional comments in the lowering function
(pointing out that lowering to "r" or "w" is more restrictive than
necessary, possibly not handling as expected some code).

[rengolin]

Excellent! Please go ahead and commit. Thanks!

[jgreenhalgh]

Thanks, that looks perfect now.

[sbaranga]

Thanks for reviewing! Committed in r267411.

Cheers,
Silviu

lib/Target/ARM/ARMISelLowering.h

public:
     /// The operand object must already have been set up with the operand type.
     ConstraintWeight getSingleConstraintMatchWeight(
       AsmOperandInfo &info, const char *constraint) const override;
 
     std::pair<unsigned, const TargetRegisterClass *>
     getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                  StringRef Constraint, MVT VT) const override;
 
+    const char *LowerXConstraint(EVT ConstraintVT) const override;
+
     /// LowerAsmOperandForConstraint - Lower the specified operand into the Ops
     /// vector.  If it is invalid, don't add anything to Ops. If hasMemory is
     /// true it means one of the asm constraint of the inline asm instruction
     /// being processed is 'm'.
     void LowerAsmOperandForConstraint(SDValue Op, std::string &Constraint,
                                       std::vector<SDValue> &Ops,
                                       SelectionDAG &DAG) const override;
 

lib/Target/ARM/ARMISelLowering.cpp

if (AsmPieces.size() == 3 &&
         return IntrinsicLowering::LowerToByteSwap(CI);
     }
     break;
   }
 
   return false;
 }
 
+const char *ARMTargetLowering::LowerXConstraint(EVT ConstraintVT) const {
+  // At this point, we have to lower this constraint to something else, so we
+  // lower it to an "r" or "w". However, by doing this we will force the result
+  // to be in register, while the X constraint is much more permissive.
+  //
+  // Although we are correct (we are free to emit anything, without
+  // constraints), we might break use cases that would expect us to be more
+  // efficient and emit something else.
+  if (!Subtarget->hasVFP2())
+    return "r";
+  if (ConstraintVT.isFloatingPoint())
+    return "w";
+  if (ConstraintVT.isVector() && Subtarget->hasNEON() &&
+     (ConstraintVT.getSizeInBits() == 64 ||
+      ConstraintVT.getSizeInBits() == 128))
+    return "w";
+
+  return "r";
+}
+
 /// getConstraintType - Given a constraint letter, return the type of
 /// constraint it is for this target.
 ARMTargetLowering::ConstraintType
 ARMTargetLowering::getConstraintType(StringRef Constraint) const {
   if (Constraint.size() == 1) {
     switch (Constraint[0]) {
     default:  break;
     case 'l': return C_RegisterClass;

test/CodeGen/ARM/inlineasm-X-allocation.ll

+; RUN: llc -mtriple=armv7-none-eabi -mattr=-neon,-vfpv2 %s -o - | FileCheck %s  -check-prefix=novfp
+; RUN: llc -mtriple=armv7-none-eabi -mattr=+neon %s -float-abi=hard -o - | FileCheck %s -check-prefix=vfp
+
+; vfp-LABEL: f1
+; vfp-CHECK: vadd.f32 s0, s0, s0
+
+; In the novfp case, the compiler is forced to assign a core register.
+; Although this register class can't be used with the vadd.f32 instruction,
+; the compiler behaved as expected since it is allowed to emit anything.
+
+; novfp-LABEL: f1
+; novfp-CHECK: vadd.f32 r0, r0, r0
+
+; This can be generated by a function such as:
+;  void f1(float f) {asm volatile ("add.f32 $0, $0, $0" : : "X" (f));}
+
+define arm_aapcs_vfpcc void @f1(float %f) {
+entry:
+  call void asm sideeffect "vadd.f32 $0, $0, $0", "X" (float %f) nounwind
+  ret void
+}

test/CodeGen/ARM/inlineasm-X-constraint.ll

+; RUN: llc -mtriple=armv7-none-eabi -mattr=+neon %s -o - | FileCheck %s
+
+; The following functions test the use case where an X constraint is used to
+; add a dependency between an assembly instruction (vmsr in this case) and
+; another instruction. In each function, we use a different type for the
+; X constraint argument.
+;
+; We can something similar from the following C code:
+; double f1(double f, int pscr_value) {
+;   asm volatile("vmsr fpscr,%0" : "=X" ((f)): "r" (pscr_value));
+;   return f+f;
+; }
+
+; CHECK-LABEL: f1
+; CHECK: vmsr fpscr
+; CHECK: vadd.f64
+
+define arm_aapcs_vfpcc double @f1(double %f, i32 %pscr_value) {
+entry:
+  %f.addr = alloca double, align 8
+  store double %f, double* %f.addr, align 8
+  call void asm sideeffect "vmsr fpscr,$1", "=*X,r"(double* nonnull %f.addr, i32 %pscr_value) nounwind
+  %0 = load double, double* %f.addr, align 8
+  %add = fadd double %0, %0
+  ret double %add
+}
+
+; int f2(int f, int pscr_value) {
+;   asm volatile("vmsr fpscr,%0" : "=X" ((f)): "r" (pscr_value));
+;   return f+f;
+; }
+
+; CHECK-LABEL: f2
+; CHECK: vmsr fpscr
+; CHECK: mul
+define arm_aapcs_vfpcc i32 @f2(i32 %f, i32 %pscr_value) {
+entry:
+  %f.addr = alloca i32, align 4
+  store i32 %f, i32* %f.addr, align 4
+  call void asm sideeffect "vmsr fpscr,$1", "=*X,r"(i32* nonnull %f.addr, i32 %pscr_value) nounwind
+  %0 = load i32, i32* %f.addr, align 4
+  %mul = mul i32 %0, %0
+  ret i32 %mul
+}
+
+
+; int f3(int f, int pscr_value) {
+;   asm volatile("vmsr fpscr,%0" : "=X" ((f)): "r" (pscr_value));
+;   return f+f;
+; }
+
+; typedef signed char int8_t;
+; typedef __attribute__((neon_vector_type(8))) int8_t int8x8_t;
+; void f3 (void)
+; {
+;   int8x8_t vector_res_int8x8;
+;   unsigned int fpscr;
+;   asm volatile ("vmsr fpscr,%1" : "=X" ((vector_res_int8x8)) : "r" (fpscr));
+;   return vector_res_int8x8 * vector_res_int8x8;
+; }
+
+; CHECK-LABEL: f3
+; CHECK: vmsr fpscr
+; CHECK: vmul.i8
+define arm_aapcs_vfpcc <8 x i8> @f3() {
+entry:
+  %vector_res_int8x8 = alloca <8 x i8>, align 8
+  %0 = getelementptr inbounds <8 x i8>, <8 x i8>* %vector_res_int8x8, i32 0, i32 0
+  call void asm sideeffect "vmsr fpscr,$1", "=*X,r"(<8 x i8>* nonnull %vector_res_int8x8, i32 undef) nounwind
+  %1 = load <8 x i8>, <8 x i8>* %vector_res_int8x8, align 8
+  %mul = mul <8 x i8> %1, %1
+  ret <8 x i8> %mul
+}
+
+; We can emit integer constants.
+; We can get this from:
+; void f() {
+;   int x = 2;
+;   asm volatile ("add r0, r0, %0" : : "X" (x));
+; }
+;
+; CHECK-LABEL: f4
+; CHECK: add r0, r0, #2
+define void @f4() {
+entry:
+  tail call void asm sideeffect "add r0, r0, $0", "X"(i32 2)
+  ret void
+}
+
+; We can emit function labels. This is equivalent to the following C code:
+; void f(void) {
+;   void (*x)(void) = &foo;
+;   asm volatile ("bl %0" : : "X" (x));
+; }
+; CHECK-LABEL: f5
+; CHECK: bl f4
+define void @f5() {
+entry:
+  tail call void asm sideeffect "bl $0", "X"(void ()* nonnull @f4)
+  ret void
+}
+
+declare void @foo(...)
+
+; This tests the behavior of the X constraint when used on functions pointers,
+; or functions with a cast. In the first asm call we figure out that this
+; is a function pointer and emit the label. However, in the second asm call
+; we can't see through the bitcast and we end up having to lower this constraint
+; to something else. This is not ideal, but it is a correct behaviour according
+; to the definition of the X constraint.
+;
+; In this case (and other cases where we could have emitted something else),
+; what we're doing with the X constraint is not particularly useful either,
+; since the user could have used "r" in this situation for the same effect.
+
+; CHECK-LABEL: f6
+; CHECK: bl foo
+; CHECK: bl r
+
+define void @f6() nounwind {
+entry:
+  tail call void asm sideeffect "bl $0", "X"(void (...)* @foo) nounwind
+  tail call void asm sideeffect "bl $0", "X"(void (...)* bitcast (void ()* @f4 to void (...)*)) nounwind
+  ret void
+}
+
+; The following IR can be generated from C code with a function like:
+; void a() {
+;   void* a = &&A;
+;   asm volatile ("bl %0" : : "X" (a));
+;  A:
+;   return;
+; }
+;
+; Ideally this would give the block address of bb, but it requires us to see
+; through blockaddress, which we can't do at the moment. This might break some
+; existing use cases where a user would expect to get a block label and instead
+; gets the block address in a register. However, note that according to the
+; "no constraints" definition this behaviour is correct (although not very nice).
+
+; CHECK-LABEL: f7
+; CHECK: bl
+define void @f7() {
+  call void asm sideeffect "bl $0", "X"( i8* blockaddress(@f7, %bb) )
+  br label %bb
+bb:
+  ret void
+}
+
+; If we use a constraint "=*X", we should get a store back to *%x (in r0).
+; CHECK-LABEL: f8
+; CHECK: str  r{{.*}}, [r0]
+define void @f8(i32 *%x) {
+entry:
+  tail call void asm sideeffect "add $0, r0, r0", "=*X"(i32 *%x)
+  ret void
+}