Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

Authored by kees on Apr 3 2019, 12:19 PM.

Diff Detail

Event Timeline

kees created this revision.Apr 3 2019, 12:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 12:19 PM
efriedma added inline comments.
3376 ↗(On Diff #193569)

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.

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?

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.

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

E5ten added a subscriber: E5ten.Apr 3 2019, 4:00 PM
kees added a comment.Apr 3 2019, 4:28 PM

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

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?)

omg I have that backwards, let me flip that around

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/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.

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?

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 added inline comments.May 6 2019, 1:54 PM
3376 ↗(On Diff #193569)

Ok, sounds like ZExt'ing bools when asked to SExt is not the way to go. We'll have to rebase this or, 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.

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

3376 ↗(On Diff #193569)

This patch needs rebasing now due to (sorry). Line 3563 should look familiar.

kees updated this revision to Diff 200153.May 18 2019, 9:23 AM

Rebased to latest LLVM

kees edited the summary of this revision. (Show Details)May 18 2019, 9:39 AM
kees added a reviewer: nickdesaulniers.
nickdesaulniers accepted this revision.May 19 2019, 7:28 PM

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

This revision is now accepted and ready to land.May 19 2019, 7:28 PM
efriedma accepted this revision.May 20 2019, 12:29 PM

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 updated this revision to Diff 200377.May 20 2019, 4:58 PM
kees edited the summary of this revision. (Show Details)

Rebasing to monorepo...

This revision was automatically updated to reflect the committed changes.