Fix PR34170: Crash on inline asm with 64bit output in 32bit GPR

[thopre]

Summary

Add support for inline assembly with output operand that do not naturally go in the register class it is constrained to (eg. double in a 32-bit GPR as in the PR).

[thopre]

Fix code style issue

[thopre]

Ping?

[efriedma]

Could you separate the "add support for matching input" part into a separate patch? I'm having trouble following what exactly you're trying to do.

[thopre]

Split the matching input handling into a separate patch.

[thopre]

In D45376#1070014, @efriedma wrote:

Could you separate the "add support for matching input" part into a separate patch? I'm having trouble following what exactly you're trying to do.

Done. Sorry for the delay, was in extended leave away from computer.

[thopre]

In D45376#1139317, @thopre wrote:

In D45376#1070014, @efriedma wrote:

Could you separate the "add support for matching input" part into a separate patch? I'm having trouble following what exactly you're trying to do.

Done. Sorry for the delay, was in extended leave away from computer.

Ping?

Best regards,

Thomas

[efriedma]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7733

Is this change necessary? getCopyFromRegs should do the appropriate int->float bitcast, I think.

test/CodeGen/ARM/pr34170.ll

1 ↗

(On Diff #152299)

Please pick a different filename for this file. And I'd like to see tests for float and double inputs and outputs, in both soft-float and hard-float modes.

[thopre]

Hi Eli,

Again my apologies for the delay, my mail filter put the notification for your comments in the wrong folder. Should be fixed now and I'll keep an eye on phabricator directly to make sure it is. I've replied to your comments inline, will add the float testcase that you asked shortly.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7733

It's not about int->float but in this case about i32 (ResultType) -> i64 (Val). It's much in the same spirit as what happens in GetRegistersForValue for input operands. As you can see here we only record the type for output but don't do the bitcase since it must be done *after* the asm call (as opposed to inputs where it must be done before).

test/CodeGen/ARM/pr34170.ll

1 ↗

(On Diff #152299)

So as not to repeat the same mistake what is the rule on using the PR number for the test? I can see quite a few tests with the same sort of name, some with very close PR number. Would inline-asm-convert-crash.ll be ok?

[efriedma]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7733

Oh, right, getCopyFromRegs doesn't do the bitcast because it thinks the destination type is i64.

I'm not really convinced going through i64 is the right approach here (it only does the right thing on ARM because i64 isn't legal), but I guess it's okay given the existing precedent.

test/CodeGen/ARM/pr34170.ll

1 ↗

(On Diff #152299)

Sometimes people use the PR number because there isn't any good alternative, but best practice is to describe what the file is testing, not why you're adding the test. So something like inline-asm-float-gpr.ll. (You can put a reference to the bug as a comment in the test if you think it's helpful.)

[thopre]

Rename testcase and test both soft and hard float ABI

[efriedma]

LGTM

[thopre]

Fixed in trunk

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

static void GetRegistersForValue(SelectionDAG &DAG, const TargetLowering &TLI,
   // If this is a constraint for a single physreg, or a constraint for a
   // register class, find it.
   std::pair<unsigned, const TargetRegisterClass *> PhysReg =
       TLI.getRegForInlineAsmConstraint(&TRI, OpInfo.ConstraintCode,
                                        OpInfo.ConstraintVT);
 
   unsigned NumRegs = 1;
   if (OpInfo.ConstraintVT != MVT::Other) {
-    // If this is a FP input in an integer register (or visa versa) insert a bit
-    // cast of the input value.  More generally, handle any case where the input
-    // value disagrees with the register class we plan to stick this in.
-    if (OpInfo.Type == InlineAsm::isInput && PhysReg.second &&
+    // If this is a FP operand in an integer register (or visa versa), or more
+    // generally if the operand value disagrees with the register class we plan
+    // to stick it in, fix the operand type.
+    //
+    // If this is an input value, the bitcast to the new type is done now.
+    // Bitcast for output value is done at the end of visitInlineAsm().
+    if ((OpInfo.Type == InlineAsm::isOutput ||
+         OpInfo.Type == InlineAsm::isInput) &&
+        PhysReg.second &&
         !TRI.isTypeLegalForClass(*PhysReg.second, OpInfo.ConstraintVT)) {
       // Try to convert to the first EVT that the reg class contains.  If the
       // types are identical size, use a bitcast to convert (e.g. two differing
-      // vector types).
+      // vector types).  Note: output bitcast is done at the end of
+      // visitInlineAsm().
       MVT RegVT = *TRI.legalclasstypes_begin(*PhysReg.second);
-      if (RegVT.getSizeInBits() == OpInfo.CallOperand.getValueSizeInBits()) {
-        OpInfo.CallOperand = DAG.getNode(ISD::BITCAST, DL,
-                                         RegVT, OpInfo.CallOperand);
+      if (RegVT.getSizeInBits() == OpInfo.ConstraintVT.getSizeInBits()) {
+        // Exclude indirect inputs while they are unsupported because the code
+        // to perform the load is missing and thus OpInfo.CallOperand still
+        // refer to the input address rather than the pointed-to value.
+        if (OpInfo.Type == InlineAsm::isInput && !OpInfo.isIndirect)
+          OpInfo.CallOperand =
+              DAG.getNode(ISD::BITCAST, DL, RegVT, OpInfo.CallOperand);
         OpInfo.ConstraintVT = RegVT;
+        // If the operand is a FP value and we want it in integer registers,
+        // use the corresponding integer type. This turns an f64 value into
+        // i64, which can be passed with two i32 values on a 32-bit machine.
       } else if (RegVT.isInteger() && OpInfo.ConstraintVT.isFloatingPoint()) {
-        // If the input is a FP value and we want it in FP registers, do a
-        // bitcast to the corresponding integer type.  This turns an f64 value
-        // into i64, which can be passed with two i32 values on a 32-bit
-        // machine.
         RegVT = MVT::getIntegerVT(OpInfo.ConstraintVT.getSizeInBits());
-        OpInfo.CallOperand = DAG.getNode(ISD::BITCAST, DL,
-                                         RegVT, OpInfo.CallOperand);
+        if (OpInfo.Type == InlineAsm::isInput)
+          OpInfo.CallOperand =
+              DAG.getNode(ISD::BITCAST, DL, RegVT, OpInfo.CallOperand);
         OpInfo.ConstraintVT = RegVT;
       }
     }
 
     NumRegs = TLI.getNumRegisters(Context, OpInfo.ConstraintVT);
   }
 
   MVT RegVT;
void SelectionDAGBuilder::visitInlineAsm(ImmutableCallSite CS) {
   if (!RetValRegs.Regs.empty()) {
     SDValue Val = RetValRegs.getCopyFromRegs(DAG, FuncInfo, getCurSDLoc(),
                                              Chain, &Flag, CS.getInstruction());
 
     // FIXME: Why don't we do this for inline asms with MRVs?
     if (CS.getType()->isSingleValueType() && CS.getType()->isSized()) {
       EVT ResultType = TLI.getValueType(DAG.getDataLayout(), CS.getType());
 
-      // If any of the results of the inline asm is a vector, it may have the
-      // wrong width/num elts.  This can happen for register classes that can
-      // contain multiple different value types.  The preg or vreg allocated may
-      // not have the same VT as was expected.  Convert it to the right type
-      // with bit_convert.
-      if (ResultType != Val.getValueType() && Val.getValueType().isVector()) {
+      // If the type of the inline asm call site return value is different but
+      // has same size as the type of the asm output bitcast it.  One example
+      // of this is for vectors with different width / number of elements.
+      // This can happen for register classes that can contain multiple
+      // different value types.  The preg or vreg allocated may not have the
+      // same VT as was expected.
+      //
+      // This can also happen for a return value that disagrees with the
+      // register class it is put in, eg. a double in a general-purpose
+      // register on a 32-bit machine.
+      if (ResultType != Val.getValueType() &&
+          ResultType.getSizeInBits() == Val.getValueSizeInBits()) {
         Val = DAG.getNode(ISD::BITCAST, getCurSDLoc(),
                           ResultType, Val);
 
       } else if (ResultType != Val.getValueType() &&
                  ResultType.isInteger() && Val.getValueType().isInteger()) {
         // If a result value was tied to an input value, the computed result may
         // have a wider width than the expected result.  Extract the relevant
         // portion.

test/CodeGen/ARM/inline-asm-operand-implicit-cast.ll

+; RUN: llc -mtriple armv7-arm-linux-gnueabihf -O2 -mcpu=cortex-a7 < %s | FileCheck %s
+
+; Check support for returning a float in GPR with soft float ABI
+define arm_aapcscc float @zerobits_float_soft() #0 {
+; CHECK-LABEL: zerobits_float_soft
+; CHECK: mov r0, #0
+  %1 = tail call float asm "mov ${0}, #0", "=&r"()
+  ret float %1
+}
+
+; Check support for returning a double in GPR with soft float ABI
+define arm_aapcscc double @zerobits_double_soft() #0 {
+; CHECK-LABEL: zerobits_double_soft
+; CHECK: mov r0, #0
+; CHECK-NEXT: mov r1, #0
+  %1 = tail call double asm "mov ${0:Q}, #0\0Amov ${0:R}, #0", "=&r"()
+  ret double %1
+}
+
+attributes #0 = { nounwind "target-features"="+d16,+vfp2,+vfp3,-fp-only-sp" "use-soft-float"="true" }
+
+
+; Check support for returning a float in GPR with hard float ABI
+define float @zerobits_float_hard() #1 {
+; CHECK-LABEL: zerobits_float_hard
+; CHECK: mov r0, #0
+; CHECK: vmov s0, r0
+  %1 = tail call float asm "mov ${0}, #0", "=&r"()
+  ret float %1
+}
+
+; Check support for returning a double in GPR with hard float ABI
+define double @zerobits_double_hard() #1 {
+; CHECK-LABEL: zerobits_double_hard
+; CHECK: mov r0, #0
+; CHECK-NEXT: mov r1, #0
+; CHECK: vmov d0, r0, r1
+  %1 = tail call double asm "mov ${0:Q}, #0\0Amov ${0:R}, #0", "=&r"()
+  ret double %1
+}
+
+attributes #1 = { nounwind "target-features"="+d16,+vfp2,+vfp3,-fp-only-sp" "use-soft-float"="false" }