Use correct registers for "A" inline asm constraint

[dim]

Summary

In PR32594, inline assembly using the 'A' constraint on x86_64 causes
llvm to crash with a "Cannot select" stack trace. This is because
X86TargetLowering::getRegForInlineAsmConstraint hardcodes that 'A'
means the EAX and EDX registers.

However, on x86_64 it means the RAX and RDX registers, and on 16-bit x86
(ia16?) it means the old AX and DX registers.

Add new register classes in X86RegisterInfo.td to support these cases,
and amend the logic in getRegForInlineAsmConstraint to cope with
different subtargets. Also add a test case, derived from PR32594.

[dim]

Remove extraneous space at the end of llvm_unreachable.

[dim]

Minor ping, since I am unsure whether any of Phab's emails were lost in the recent outage.

[ab]

LGTM, thanks!

16-bit is weird, but it'd be great if you could add a testcase for that.

test/CodeGen/X86/inline-asm-A-constraint.ll

2

'-march=x86_64' -> '-mtriple=x86_64--' ? Helps avoid sensitivity to the host (not that it matters in this file) and lets you get rid of the IR triple.

Also, '-no-integrated-as' doesn't have an effect here, right?

10

Can you add CHECK lines for the generated asm?

[dim]

test/CodeGen/X86/inline-asm-A-constraint.ll

2

I copied this example from other inline asm tests, most of which seem to use -no-integrated-as. I'm not sure why that was done, maybe because in some cases invalid mnemonics (e.g. foo) are used, and that could trip up the test?

Similar for the -march flag, most test cases seem to use plain x86_64. But it's a good idea to remove host influences, thanks.

10

Since the asm statement is completely empty, there is nothing generated except #APP and #NO_APP. Does it make sense to check for that?

[dim]

Address comments by changing the test case to something more explicit, derived from PR32594.

This uses x86_64-- as triple, and checks the resulting assembly.

lib/Target/X86/X86ISelLowering.cpp

 
     // flags -> EFLAGS
     if (StringRef("{flags}").equals_lower(Constraint)) {
       Res.first = X86::EFLAGS;
       Res.second = &X86::CCRRegClass;
       return Res;
     }
 
-    // 'A' means EAX + EDX.
+    // 'A' means [ER]AX + [ER]DX.
     if (Constraint == "A") {
-      Res.first = X86::EAX;
-      Res.second = &X86::GR32_ADRegClass;
+      if (Subtarget.is64Bit()) {
+        Res.first = X86::RAX;
+        Res.second = &X86::GR64_ADRegClass;
+      } else if (Subtarget.is32Bit()) {
+        Res.first = X86::EAX;
+        Res.second = &X86::GR32_ADRegClass;
+      } else if (Subtarget.is16Bit()) {
+        Res.first = X86::AX;
+        Res.second = &X86::GR16_ADRegClass;
+      } else {
+        llvm_unreachable("Expecting 64, 32 or 16 bit subtarget");
+      }
       return Res;
     }
     return Res;
   }
 
   // Otherwise, check to see if this is a register class of the wrong value
   // type.  For example, we want to map "{ax},i32" -> {eax}, we don't want it to
   // turn into {ax},{dx}.

lib/Target/X86/X86RegisterInfo.td

 
 // When RBP is used as a base pointer in a 32-bit addresses environement,
 // this is also safe to use the full register to access addresses.
 // Since RBP will never be spilled, stick to a 32 alignment to save
 // on memory consumption.
 def LOW32_ADDR_ACCESS_RBP : RegisterClass<"X86", [i32], 32,
                                           (add LOW32_ADDR_ACCESS, RBP)>;
 
-// A class to support the 'A' assembler constraint: EAX then EDX.
+// A class to support the 'A' assembler constraint: [ER]AX then [ER]DX.
+def GR16_AD : RegisterClass<"X86", [i16], 16, (add AX, DX)>;
 def GR32_AD : RegisterClass<"X86", [i32], 32, (add EAX, EDX)>;
+def GR64_AD : RegisterClass<"X86", [i64], 64, (add RAX, RDX)>;
 
 // Scalar SSE2 floating point registers.
 def FR32 : RegisterClass<"X86", [f32], 32, (sequence "XMM%u", 0, 15)>;
 
 def FR64 : RegisterClass<"X86", [f64], 64, (add FR32)>;
 
 def FR128 : RegisterClass<"X86", [i128, f128], 128, (add FR32)>;
 

test/CodeGen/X86/inline-asm-A-constraint.ll

+; RUN: llc -mtriple=x86_64-- < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64--"
+
+; Function Attrs: nounwind uwtable
+define { i64, i64 } @foo(i8* %ptr, i128* nocapture readonly %src, i128* nocapture readonly %dst) local_unnamed_addr #0 {
+entry:
+  %0 = load i128, i128* %dst, align 16, !tbaa !1
+  %shr = lshr i128 %0, 64
+  %conv = trunc i128 %shr to i64
+  %conv1 = trunc i128 %0 to i64
+  %1 = load i128, i128* %src, align 16, !tbaa !1
+  %2 = tail call i128 asm sideeffect "lock; cmpxchg16b $1", "=A,=*m,{cx},{bx},0,*m,~{dirflag},~{fpsr},~{flags}"(i8* %ptr, i64 %conv, i64 %conv1, i128 %1, i8* %ptr) #1, !srcloc !5
+  %retval.sroa.0.0.extract.trunc = trunc i128 %2 to i64
+  %retval.sroa.2.0.extract.shift = lshr i128 %2, 64
+  %retval.sroa.2.0.extract.trunc = trunc i128 %retval.sroa.2.0.extract.shift to i64
+  %.fca.0.insert = insertvalue { i64, i64 } undef, i64 %retval.sroa.0.0.extract.trunc, 0
+  %.fca.1.insert = insertvalue { i64, i64 } %.fca.0.insert, i64 %retval.sroa.2.0.extract.trunc, 1
+  ret { i64, i64 } %.fca.1.insert
+}
+; CHECK: lock
+; CHECK-NEXT: cmpxchg16b
+
+attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 5.0.0 (trunk 300088)"}
+!1 = !{!2, !2, i64 0}
+!2 = !{!"__int128", !3, i64 0}
+!3 = !{!"omnipotent char", !4, i64 0}
+!4 = !{!"Simple C/C++ TBAA"}
+!5 = !{i32 269}