[CodeGen]: don't treat structures returned in registers as memory inputs

[glider]

Summary

The "=r" output constraint for a structure variable passed to inline asm
shouldn't be converted to "=*r", as this changes the asm directive
semantics and prevents DSE optimizations.
Instead, preserve the constraints and return such structures as integers
of corresponding size, which are converted back to structures when
storing the result.

Fixes PR42672.

[efriedma]

clang/lib/CodeGen/CGStmt.cpp

2287 ↗

(On Diff #211568)

Not "=="?

2325 ↗

(On Diff #211568)

Will this work if the struct is an unusual size, like sizeof(struct s) == 3 or sizeof(struct s) == 32? (3 is unlikely to show up in real code, but 32 could correspond to a vector register.)

clang/test/CodeGen/PR42672.c

2 ↗

(On Diff #211568)

We usually try to avoid tests that run "-O2"; it's simpler to just check the unoptimized clang output directly.

[nickdesaulniers]

clang/lib/CodeGen/CGStmt.cpp

1989 ↗

(On Diff #211568)

Are we able to use something other than std::vector<bool> here from ADT?
http://llvm.org/docs/ProgrammersManual.html#bit-storage-containers-bitvector-sparsebitvector

[glider]

Fixed comments from Eli and Nick, added tests for unusual struct sizes

[glider]

clang/lib/CodeGen/CGStmt.cpp

1989 ↗

(On Diff #211568)

I don't think std::vector<bool> is a bottleneck here, but since it might be deprecated someday let's just not use it.

2287 ↗

(On Diff #211568)

Turns out ResultRegDests can be also extended by addReturnRegisterOutputs above (line 2122), so no.
I've added a comment to clarify that.

2325 ↗

(On Diff #211568)

For a 3-byte struct this works as follows: a 3-byte structure is allocated on the stack with alignment 2, then the assembly returns a 4-byte integer that's written to that 3-byte structure.
This conforms to what GCC does, and I think it's legal to let the assembly write more than a structure size here, as the corresponding register size is bigger (i.e. it's a bug in the C code, not Clang).

For a 32-bit struct we crash now, whereas GCC reports an "impossible constraint in ‘asm’"
It's interesting that the crash happens in the frontend, i.e. it's somehow knows the maximum possible size for the register operand.
We can check that getContext().getIntTypeForBitwidth(Size, /*Signed*/ false) is a nonnull type to prevent the crashes.

clang/test/CodeGen/PR42672.c

50 ↗

(On Diff #211919)

The acceptable size actually depends on the target platform. Not sure we can test for all of them, and we'll probably need to restrict this test to e.g. x86

[glider]

Make big_struct() test triple-specific

[efriedma]

clang/test/CodeGen/PR42672.c

3 ↗

(On Diff #211920)

Might as well just make all these tests use an x86 triple; I don't really want to speculate how an unknown target will react to any specific size.

40 ↗

(On Diff #211920)

This isn't a three-byte struct; it's a four-byte struct where one of the bytes is only used for padding.

50 ↗

(On Diff #211919)

The interesting case for a 32-byte struct would actually be something like "=x"(str)... which currently passes the clang frontend, since 32 is a legal size for that constraint (although it eventually fails in the backend).

[glider]

clang/test/CodeGen/PR42672.c

40 ↗

(On Diff #211920)

You're right. Making this structure packed yields an error, which conforms to GCC behavior.
I've added a test for that as well.

50 ↗

(On Diff #211919)

Changing "=r" to "=X" indeed makes this particular test pass (there's nothing to fail in the backend, as we don't actually generate instructions that write to memory)
I'm however unsure adding a test for "=X" makes any difference, as our patch is irrelevant to this constraint.

[glider]

Addressed Eli's comments, added a test for a packed struct

[glider]

Test for =X somehow sneaked in - drop it.

[glider]

+srhines
Heads up: this patch reduces the size of the following Android kernel functions:

_raw_spin_lock and _raw_spin_lock_bh: 132 bytes->68 bytes
_raw_spin_lock_irq: 136 bytes->72 bytes
_raw_spin_lock_irqsave: 144 bytes->80 bytes
_raw_spin_trylock: 156 bytes->112 bytes
_raw_spin_trylock_bh: 136 bytes->92 bytes

[glider]

Eli, any other comments?

[efriedma]

For the "=x" thing I was talking about, try the following testcase:

void a() { struct S { unsigned x[4]; } z; asm volatile("%0":"=x"(z)); }
void a2() { struct S { unsigned x[8]; } z; asm volatile("%0":"=x"(z)); }

clang trunk gives "error: couldn't allocate output register for constraint 'x'" in the backend for both functions. gcc prints "%xmm0" for the first, and rejects the second; not exactly sure why it's rejecting the second, though. It would be nice if both worked, although I guess it's okay if we print a reasonable error message. Please add a testcase, at least.

clang/test/CodeGen/x86_64-PR42672.c

3 ↗

(On Diff #212293)

You can prefix a command in a RUN line with "not" if you expect it to return a non-zero error code. So you can write not %clang_cc1 [...] 2>&1 | FileCheck %s or something like that.

[glider]

In D65234#1615649, @efriedma wrote:

For the "=x" thing I was talking about, try the following testcase:

void a() { struct S { unsigned x[4]; } z; asm volatile("%0":"=x"(z)); }
void a2() { struct S { unsigned x[8]; } z; asm volatile("%0":"=x"(z)); }

clang trunk gives "error: couldn't allocate output register for constraint 'x'" in the backend for both functions. gcc prints "%xmm0" for the first, and rejects the second; not exactly sure why it's rejecting the second, though. It would be nice if both worked, although I guess it's okay if we print a reasonable error message. Please add a testcase, at least.

Note that this is invalid assembly, as it emits a register name instead of an assembly instruction. I don't think it should work at all.
If we replace "%0" with a "nop", Clang will reject the second test, but will accept the first one.

[glider]

Added test cases for =x, replaced grep with not

[efriedma]

LGTM

[glider]

Rebased the patch

[glider]

Landed r370335, thank you!

cfe/trunk/lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitAsmStmt(const AsmStmt &S) {
   std::string Constraints;
 
   std::vector<LValue> ResultRegDests;
   std::vector<QualType> ResultRegQualTys;
   std::vector<llvm::Type *> ResultRegTypes;
   std::vector<llvm::Type *> ResultTruncRegTypes;
   std::vector<llvm::Type *> ArgTypes;
   std::vector<llvm::Value*> Args;
+  llvm::BitVector ResultTypeRequiresCast;
 
   // Keep track of inout constraints.
   std::string InOutConstraints;
   std::vector<llvm::Value*> InOutArgs;
   std::vector<llvm::Type*> InOutArgTypes;
 
   // Keep track of out constraints for tied input operand.
   std::vector<std::string> OutputConstraints;
OutputConstraint = AddVariableConstraints(OutputConstraint, *OutExpr,
                                               Info.earlyClobber());
     OutputConstraints.push_back(OutputConstraint);
     LValue Dest = EmitLValue(OutExpr);
     if (!Constraints.empty())
       Constraints += ',';
 
     // If this is a register output, then make the inline asm return it
     // by-value.  If this is a memory result, return the value by-reference.
-    if (!Info.allowsMemory() && hasScalarEvaluationKind(OutExpr->getType())) {
+    bool isScalarizableAggregate =
+        hasAggregateEvaluationKind(OutExpr->getType());
+    if (!Info.allowsMemory() && (hasScalarEvaluationKind(OutExpr->getType()) ||
+                                 isScalarizableAggregate)) {
       Constraints += "=" + OutputConstraint;
       ResultRegQualTys.push_back(OutExpr->getType());
       ResultRegDests.push_back(Dest);
-      ResultRegTypes.push_back(ConvertTypeForMem(OutExpr->getType()));
-      ResultTruncRegTypes.push_back(ResultRegTypes.back());
-
+      ResultTruncRegTypes.push_back(ConvertTypeForMem(OutExpr->getType()));
+      if (Info.allowsRegister() && isScalarizableAggregate) {
+        ResultTypeRequiresCast.push_back(true);
+        unsigned Size = getContext().getTypeSize(OutExpr->getType());
+        llvm::Type *ConvTy = llvm::IntegerType::get(getLLVMContext(), Size);
+        ResultRegTypes.push_back(ConvTy);
+      } else {
+        ResultTypeRequiresCast.push_back(false);
+        ResultRegTypes.push_back(ResultTruncRegTypes.back());
+      }
       // If this output is tied to an input, and if the input is larger, then
       // we need to set the actual result type of the inline asm node to be the
       // same as the input type.
       if (Info.hasMatchingInput()) {
         unsigned InputNo;
         for (InputNo = 0; InputNo != S.getNumInputs(); ++InputNo) {
           TargetInfo::ConstraintInfo &Input = InputConstraintInfos[InputNo];
           if (Input.hasTiedOperand() && Input.getTiedOperand() == i)
llvm::CallInst *Result =
         Builder.CreateCall(IA, Args, getBundlesForFunclet(IA));
     UpdateAsmCallInst(cast<llvm::CallBase>(*Result), HasSideEffect, ReadOnly,
                       ReadNone, S, ResultRegTypes, *this, RegResults);
   }
 
   assert(RegResults.size() == ResultRegTypes.size());
   assert(RegResults.size() == ResultTruncRegTypes.size());
   assert(RegResults.size() == ResultRegDests.size());
+  // ResultRegDests can be also populated by addReturnRegisterOutputs() above,
+  // in which case its size may grow.
+  assert(ResultTypeRequiresCast.size() <= ResultRegDests.size());
   for (unsigned i = 0, e = RegResults.size(); i != e; ++i) {
     llvm::Value *Tmp = RegResults[i];
 
     // If the result type of the LLVM IR asm doesn't match the result type of
     // the expression, do the conversion.
     if (ResultRegTypes[i] != ResultTruncRegTypes[i]) {
       llvm::Type *TruncTy = ResultTruncRegTypes[i];
 
if (ResultRegTypes[i] != ResultTruncRegTypes[i]) {
         Tmp = Builder.CreateTrunc(Tmp, TruncTy);
       } else if (TruncTy->isIntegerTy()) {
         Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
       } else if (TruncTy->isVectorTy()) {
         Tmp = Builder.CreateBitCast(Tmp, TruncTy);
       }
     }
 
-    EmitStoreThroughLValue(RValue::get(Tmp), ResultRegDests[i]);
+    LValue Dest = ResultRegDests[i];
+    // ResultTypeRequiresCast elements correspond to the first
+    // ResultTypeRequiresCast.size() elements of RegResults.
+    if ((i < ResultTypeRequiresCast.size()) && ResultTypeRequiresCast[i]) {
+      unsigned Size = getContext().getTypeSize(ResultRegQualTys[i]);
+      Address A = Builder.CreateBitCast(Dest.getAddress(),
+                                        ResultRegTypes[i]->getPointerTo());
+      QualType Ty = getContext().getIntTypeForBitwidth(Size, /*Signed*/ false);
+      if (Ty.isNull()) {
+        const Expr *OutExpr = S.getOutputExpr(i);
+        CGM.Error(
+            OutExpr->getExprLoc(),
+            "impossible constraint in asm: can't store struct into a register");
+        return;
+      }
+      Dest = MakeAddrLValue(A, Ty);
+    }
+    EmitStoreThroughLValue(RValue::get(Tmp), Dest);
   }
 }
 
 LValue CodeGenFunction::InitCapturedStruct(const CapturedStmt &S) {
   const RecordDecl *RD = S.getCapturedRecordDecl();
   QualType RecordTy = getContext().getRecordType(RD);
 
   // Initialize the captured struct.

cfe/trunk/test/CodeGen/asm-attrs.c

 // RUN: %clang_cc1 -triple armv7-apple-darwin -emit-llvm %s -o - | FileCheck %s
 
 // CHECK: call i32 asm "foo0", {{.*}} [[READNONE:#[0-9]+]]
 // CHECK: call i32 asm "foo1", {{.*}} [[READNONE]]
 // CHECK: call i32 asm "foo2", {{.*}} [[NOATTRS:#[0-9]+]]
 // CHECK: call i32 asm sideeffect "foo3", {{.*}} [[NOATTRS]]
 // CHECK: call i32 asm "foo4", {{.*}} [[READONLY:#[0-9]+]]
 // CHECK: call i32 asm "foo5", {{.*}} [[READONLY]]
 // CHECK: call i32 asm "foo6", {{.*}} [[NOATTRS]]
 // CHECK: call void asm sideeffect "foo7", {{.*}} [[NOATTRS]]
-// CHECK: call void asm "foo8", {{.*}} [[NOATTRS]]
+// CHECK: call i32 asm "foo8", {{.*}} [[READNONE]]
 
 // CHECK: attributes [[READNONE]] = { nounwind readnone }
 // CHECK: attributes [[NOATTRS]] = { nounwind }
 // CHECK: attributes [[READONLY]] = { nounwind readonly }
 
 int g0, g1;
 
 struct S {

cfe/trunk/test/CodeGen/x86_64-PR42672.c

+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -DSTRUCT -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-STRUCT
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -USTRUCT -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-NOSTRUCT
+// RUN: not %clang_cc1 -triple=x86_64-unknown-linux-gnu -DIMPOSSIBLE_ODD -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-IMPOSSIBLE_ODD
+// RUN: not %clang_cc1 -triple=x86_64-unknown-linux-gnu -DIMPOSSIBLE_BIG -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-IMPOSSIBLE_BIG
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -DPOSSIBLE_X -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-X
+// RUN: not %clang_cc1 -triple=x86_64-unknown-linux-gnu -DIMPOSSIBLE_X -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK-IMPOSSIBLE_X
+
+// Make sure Clang doesn't treat |lockval| as asm input.
+void _raw_spin_lock(void) {
+#ifdef STRUCT
+  struct {
+    unsigned short owner, next;
+  } lockval;
+  lockval.owner = 1;
+  lockval.next = 2;
+#else
+  int lockval;
+  lockval = 3;
+#endif
+  asm("nop"
+      : "=r"(lockval));
+}
+// CHECK-LABEL: _raw_spin_lock
+// CHECK-LABEL: entry:
+
+// CHECK-STRUCT:  %lockval = alloca %struct.anon, align 2
+// CHECK-STRUCT:  store i16 1
+// CHECK-STRUCT:  store i16 2
+// CHECK-STRUCT: [[RES:%[0-9]+]] = call i32 asm "nop", "=r,~{dirflag},~{fpsr},~{flags}"()
+// CHECK-STRUCT: [[CAST:%[0-9]+]] = bitcast %struct.anon* %lockval to i32*
+// CHECK-STRUCT: store i32 [[RES]], i32* [[CAST]], align 2
+
+// CHECK-NOSTRUCT: %lockval = alloca i32, align 4
+// CHECK-NOSTRUCT: store i32 3
+// CHECK-NOSTRUCT:  [[RES:%[0-9]+]] = call i32 asm "nop", "=r,~{dirflag},~{fpsr},~{flags}"()
+// CHECK-NOSTRUCT: store i32 [[RES]], i32* %lockval, align 4
+
+// Check Clang correctly handles a structure with padding.
+void unusual_struct(void) {
+  struct {
+    unsigned short first;
+    unsigned char second;
+  } str;
+  asm("nop"
+      : "=r"(str));
+}
+
+// Check Clang reports an error if attempting to return a structure for which
+// no direct conversion to a register is possible.
+void odd_struct(void) {
+#ifdef IMPOSSIBLE_ODD
+  struct __attribute__((__packed__)) {
+    unsigned short first;
+    unsigned char second;
+  } str;
+  asm("nop"
+      : "=r"(str));
+#endif
+}
+// CHECK-IMPOSSIBLE_ODD: impossible constraint in asm
+
+// Check Clang reports an error if attempting to return a big structure via a register.
+void big_struct(void) {
+#ifdef IMPOSSIBLE_BIG
+  struct {
+    long long int v1, v2, v3, v4;
+  } str;
+  asm("nop"
+      : "=r"(str));
+#endif
+}
+// CHECK-IMPOSSIBLE_BIG: impossible constraint in asm
+
+// Clang is able to emit LLVM IR for an 16-byte structure.
+void x_constraint_fit() {
+#ifdef POSSIBLE_X
+  struct S {
+    unsigned x[4];
+  } z;
+  asm volatile("nop"
+               : "=x"(z));
+#endif
+}
+// CHECK-LABEL: x_constraint_fit
+// CHECK-X: %z = alloca %struct.S, align 4
+// CHECK-X: [[RES:%[0-9]+]] = call i128 asm sideeffect "nop", "=x,~{dirflag},~{fpsr},~{flags}"()
+// CHECK-X: [[CAST:%[0-9]+]] = bitcast %struct.S* %z to i128*
+// CHECK-X: store i128 [[RES]], i128* [[CAST]], align 4
+// CHECK-X: ret
+
+// Clang is unable to emit LLVM IR for a 32-byte structure.
+void x_constraint_nofit() {
+#ifdef IMPOSSIBLE_X
+  struct S {
+    unsigned x[8];
+  } z;
+  asm volatile("nop"
+               : "=x"(z));
+#endif
+}
+
+// CHECK-IMPOSSIBLE_X: invalid output size for constraint