[TargetLowering] Extend bool args to inline-asm according to getBooleanType

[kees]

Summary

This extends Krzysztof Parzyszek's X86-specific solution
(https://reviews.llvm.org/D60208) to the generic code pointed out by
James Y Knight.

[kees]

For note, this is based on https://reviews.llvm.org/D60208

[efriedma]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

3376

Is getBooleanContents really the predicate here? Despite the name, it's not really an ABI hook; it's specifically supposed to describe the behavior of a few SelectionDAG instructions like ISD::SELECT. I'd prefer to just say booleans are always zero-extended, unless we come across some case where gcc doesn't do that.

[jyknight]

After looking at this a bit more, I realized that this is still insufficient. There's *many* more target-specific constraints than just x86 "i" which sign extend constant integer arguments -- and we'll need to apply this change to every one of them, in all targets.

I feel like perhaps there's another better place to do the bool handling, rather than adding it to every constraint's processing?

[kparzysz]

The problem is that in some contexts the explicit sign- or zero-extension may be deliberate, regardless of how boolean values are represented in wider types. Other than delegating that to the target experts, I don't know how to address that.

[efriedma]

What's the alternative? We can't dictate the behavior of target-specific constraints from target-independent code; some targets actually have boolean registers.

[kees]

Should I respin to make booleans always zero extended? I can adjust the X86 code at the same time...

[nickdesaulniers]

In D60224#1454229, @kees wrote:

Should I respin to make booleans always zero extended? I can adjust the X86 code at the same time...

Just to see what would happen:

diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index ca56e8b9328c..ebb4f500e4fa 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -146,7 +146,7 @@ public:
   /// that this method can assert if the value does not fit in 64 bits.
   /// Return the zero extended value.
   inline uint64_t getZExtValue() const {
-    return Val.getZExtValue();
+    return (getBitWidth() == 1) ? Val.getSExtValue() : Val.getZExtValue();
   }
 
   /// Return the constant as a 64-bit integer value after it has been sign

produces 34 test failures. They seem related:

LLVM :: CodeGen/X86/inline-asm-i-constraint-i1.ll
LLVM :: CodeGen/X86/pr32241.ll
LLVM :: CodeGen/X86/xaluo.ll
LLVM :: CodeGen/X86/xmulo.ll
LLVM :: ThinLTO/X86/cache-typeid-resolutions.ll
LLVM :: Transforms/LoopVectorize/AArch64/outer_loop_test1_no_explicit_vect_width.ll
LLVM :: Transforms/LoopVectorize/X86/metadata-enable.ll
LLVM :: Transforms/LoopVectorize/X86/no_fpmath.ll
LLVM :: Transforms/LoopVectorize/X86/no_fpmath_with_hotness.ll
LLVM :: Transforms/LoopVectorize/X86/outer_loop_test1_no_explicit_vect_width.ll
LLVM :: Transforms/LoopVectorize/X86/vect.omp.force.small-tc.ll
LLVM :: Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll
LLVM :: Transforms/LoopVectorize/no_switch.ll
LLVM :: Transforms/LoopVectorize/outer_loop_test1.ll
LLVM :: Transforms/LoopVectorize/outer_loop_test2.ll
LLVM :: Transforms/WholeProgramDevirt/constant-arg.ll
LLVM :: Transforms/WholeProgramDevirt/export-unique-ret-val.ll
LLVM :: Transforms/WholeProgramDevirt/export-vcp.ll
LLVM :: Transforms/WholeProgramDevirt/unique-retval.ll
LLVM :: Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
LLVM :: Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
LLVM :: Transforms/WholeProgramDevirt/virtual-const-prop-end.ll

@jyknight @kparzysz @eli.friedman thoughts? (is this a terrible idea?)

[nickdesaulniers]

omg I have that backwards, let me flip that around

[nickdesaulniers]

diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index ca56e8b9328c..50ece4b65990 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -154,7 +154,7 @@ public:
   /// this method can assert if the value does not fit in 64 bits.
   /// Return the sign extended value.
   inline int64_t getSExtValue() const {
-    return Val.getSExtValue();
+    return (getBitWidth() == 1) ? Val.getZExtValue() : Val.getSExtValue();
   }
 
   /// A helper method that can be used to determine if the constant contained

is what I meant. Produces 23 test failures (I didn't try @kees tests from this patch):

LLVM :: CodeGen/AMDGPU/cf-loop-on-constant.ll
LLVM :: CodeGen/AMDGPU/divergent-branch-uniform-condition.ll
LLVM :: CodeGen/AMDGPU/function-args.ll
LLVM :: CodeGen/AMDGPU/i1-copy-from-loop.ll
LLVM :: CodeGen/AMDGPU/i1-copy-phi-uniform-branch.ll
LLVM :: CodeGen/AMDGPU/i1-copy-phi.ll
LLVM :: CodeGen/AMDGPU/infinite-loop.ll
LLVM :: CodeGen/AMDGPU/inline-asm.ll
LLVM :: CodeGen/AMDGPU/llvm.amdgcn.div.fmas.ll
LLVM :: CodeGen/AMDGPU/llvm.amdgcn.kill.ll
LLVM :: CodeGen/AMDGPU/llvm.amdgcn.ps.live.ll
LLVM :: CodeGen/AMDGPU/loop_break.ll
LLVM :: CodeGen/AMDGPU/loop_exit_with_xor.ll
LLVM :: CodeGen/AMDGPU/multi-divergent-exit-region.ll
LLVM :: CodeGen/AMDGPU/multilevel-break.ll
LLVM :: CodeGen/AMDGPU/sub_i1.ll
LLVM :: CodeGen/AMDGPU/trunc-cmp-constant.ll
LLVM :: CodeGen/AMDGPU/valu-i1.ll
LLVM :: CodeGen/PowerPC/fast-isel-ret.ll
LLVM :: CodeGen/X86/avx512-fsel.ll
LLVM :: CodeGen/X86/pr32256.ll
LLVM :: CodeGen/X86/pr32284.ll

Seems like the test cases should probably be updated (they all seem to have -1s converted to 1s which I don't think the original -1s were intentional)? I'd probably also add an explicitly named method to ConstantInt named SExtBoolean or something that had the original semantics if you truly meant to sign extend a boolean, because chances are, you dont.

[nickdesaulniers]

Otherwise, if we didn't want to change the semantics of ConstantInt::getSExtValue(), it looks like there's ~145 call sites that would have to be updated/changed. Thoughts?

[efriedma]

What are you trying to do? We aren't going to change a fundamental APInt operation to return the wrong result for the sake of inline asm. The proposal was to centralize the bool processing for inline asm, specifically.

[nickdesaulniers]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

3376

Ok, sounds like ZExt'ing bools when asked to SExt is not the way to go. We'll have to rebase this or https://reviews.llvm.org/D61560, depending on which lands first ;) . I agree that always Zextending is probably simpler while being correct.

IIUC, the code in the patch and the previous one this is based on only Zext if the boolean is not zero.

[nickdesaulniers]

ToT LLVM fails w/ all 9 new tests. (which is expected, and shows the value of the tests added here, which expose the bug).

lib/CodeGen/SelectionDAG/TargetLowering.cpp

3376

This patch needs rebasing now due to https://reviews.llvm.org/rL360604 (sorry). Line 3563 should look familiar.

[kees]

Rebased to latest LLVM

[nickdesaulniers]

LGTM, thanks @kees . Parting thoughts @eli.friedman , @kparzysz , @jyknight ?

[efriedma]

I don't think you need separate tests for ARM and Thumb and Thumb2, since they're all the same backend. Just the ARM one is enough. Otherwise LGTM.

[kees]

Rebasing to monorepo...

lib/CodeGen/SelectionDAG/TargetLowering.cpp

if (GA) {   // Either &GV   or   &GV+C
       return;
     }
     if (C) {   // just C, no GV.
       // Simple constants are not allowed for 's'.
       if (ConstraintLetter != 's') {
         // gcc prints these as sign extended.  Sign extend value to 64 bits
         // now; without this it would get ZExt'd later in
         // ScheduleDAGSDNodes::EmitNode, which is very generic.
-        Ops.push_back(DAG.getTargetConstant(C->getSExtValue(),
-                                            SDLoc(C), MVT::i64));
+        bool IsBool = C->getConstantIntValue()->getBitWidth() == 1;
+        BooleanContent BCont = getBooleanContents(MVT::i64);
+        ISD::NodeType ExtOpc = IsBool ? getExtendForContent(BCont)
+                                      : ISD::SIGN_EXTEND;
+        int64_t ExtVal = ExtOpc == ISD::ZERO_EXTEND ? C->getZExtValue()
+                                                    : C->getSExtValue();
+        Ops.push_back(DAG.getTargetConstant(ExtVal, SDLoc(C), MVT::i64));
       }
       return;
     }
     break;
   }
   }
 }
 

test/CodeGen/AArch64/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "aarch64-unknown-linux-gnu"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/ARM/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=armv7-unknown-linux-gnueabi < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "armv7-unknown-linux-gnueabi"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/Mips/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=mips64el-unknown-linux-gnu < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "mips64el-unknown-linux-gnu"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/PowerPC/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "powerpc64le-unknown-linux-gnu"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/RISCV/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=riscv64-unknown-linux-gnu < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "riscv64-unknown-linux-gnu"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/SPARC/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=sparc64-unknown-linux-gnu < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "sparc64-unknown-linux-gnu"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/SystemZ/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=s390x-linux-gnu < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "s390x-linux-gnu"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/Thumb/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=thumbv7-linux-gnueabi < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "thumbv7-linux-gnueabi"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }

test/CodeGen/Thumb2/inline-asm-i-constraint-i1.ll

+; RUN: llc -mtriple=thumbv8-none-linux-gnueabi < %s | FileCheck %s
+
+; Make sure that boolean immediates are properly (zero) extended.
+; CHECK: TEST 42 + 1 - .
+
+target triple = "thumbv8-none-linux-gnueabi"
+
+define i32 @foo() #0 {
+entry:
+  tail call void asm sideeffect "#TEST 42 + ${0:c} - .\0A\09", "i,~{dirflag},~{fpsr},~{flags}"(i1 true) #0
+  ret i32 1
+}
+
+attributes #0 = { nounwind }