[NFC][LLVM][CodeGen][X86] Add ConstantInt/FP based vector support to MachineInstr fixup and printing code. #137331
Conversation
…neInst fixup and printing code.
When -use-constant-{int,fp}-for-fixed-length-splat are enabled,
constant vector splats take the form of ConstantInt/FP instead
of ConstantVector. These constants get linked to MachineInsts
via constant pools for later processing. The processing assumes
ConstantInt/FP to always represent scalar constants with this PR
extending the code to support vector types.
NOTE: The test choices are somewhat artificial because pretty much all
the vector tests failed without these changes when the new constants
are enabled.
|
@llvm/pr-subscribers-backend-x86 Author: Paul Walker (paulwalker-arm) ChangesWhen -use-constant-{int,fp}-for-fixed-length-splat are enabled, constant vector splats take the form of ConstantInt/FP instead of ConstantVector. These constants get linked to MachineInstrs via constant pools for later processing. The processing assumes ConstantInt/FP to always represent scalar constants with this PR extending the code to support vector types. NOTE: The test choices are somewhat artificial because pretty much all the vector tests failed without these changes when the new constants are enabled. Full diff: https://github.com/llvm/llvm-project/pull/137331.diff 4 Files Affected:
diff --git a/llvm/lib/Target/X86/X86FixupVectorConstants.cpp b/llvm/lib/Target/X86/X86FixupVectorConstants.cpp
index 2c870d1171658..a415a45775984 100644
--- a/llvm/lib/Target/X86/X86FixupVectorConstants.cpp
+++ b/llvm/lib/Target/X86/X86FixupVectorConstants.cpp
@@ -88,11 +88,19 @@ static std::optional<APInt> extractConstantBits(const Constant *C) {
if (isa<UndefValue>(C))
return APInt::getZero(NumBits);
- if (auto *CInt = dyn_cast<ConstantInt>(C))
+ if (auto *CInt = dyn_cast<ConstantInt>(C)) {
+ if (isa<VectorType>(CInt->getType()))
+ return APInt::getSplat(NumBits, CInt->getValue());
+
return CInt->getValue();
+ }
+
+ if (auto *CFP = dyn_cast<ConstantFP>(C)) {
+ if (isa<VectorType>(CFP->getType()))
+ return APInt::getSplat(NumBits, CFP->getValue().bitcastToAPInt());
- if (auto *CFP = dyn_cast<ConstantFP>(C))
return CFP->getValue().bitcastToAPInt();
+ }
if (auto *CV = dyn_cast<ConstantVector>(C)) {
if (auto *CVSplat = getSplatValueAllowUndef(CV)) {
diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp
index d9945bdf2db60..07cbd54c160df 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1568,9 +1568,23 @@ static void printConstant(const Constant *COp, unsigned BitWidth,
if (isa<UndefValue>(COp)) {
CS << "u";
} else if (auto *CI = dyn_cast<ConstantInt>(COp)) {
- printConstant(CI->getValue(), CS, PrintZero);
+ if (auto VTy = dyn_cast<FixedVectorType>(CI->getType())) {
+ for (unsigned I = 0, E = VTy->getNumElements(); I != E; ++I) {
+ if (I != 0)
+ CS << ",";
+ printConstant(CI->getValue(), CS, PrintZero);
+ }
+ } else
+ printConstant(CI->getValue(), CS, PrintZero);
} else if (auto *CF = dyn_cast<ConstantFP>(COp)) {
- printConstant(CF->getValueAPF(), CS, PrintZero);
+ if (auto VTy = dyn_cast<FixedVectorType>(CF->getType())) {
+ for (unsigned I = 0, E = VTy->getNumElements(); I != E; ++I) {
+ if (I != 0)
+ CS << ",";
+ printConstant(CF->getValueAPF(), CS, PrintZero);
+ }
+ } else
+ printConstant(CF->getValueAPF(), CS, PrintZero);
} else if (auto *CDS = dyn_cast<ConstantDataSequential>(COp)) {
Type *EltTy = CDS->getElementType();
bool IsInteger = EltTy->isIntegerTy();
diff --git a/llvm/test/CodeGen/X86/sse2.ll b/llvm/test/CodeGen/X86/sse2.ll
index cf5f527b16114..3e5d76eae0bb3 100644
--- a/llvm/test/CodeGen/X86/sse2.ll
+++ b/llvm/test/CodeGen/X86/sse2.ll
@@ -6,6 +6,8 @@
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx | FileCheck %s --check-prefixes=AVX,X64-AVX,AVX1,X64-AVX1
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx512f,+avx512bw,+avx512dq,+avx512vl | FileCheck %s --check-prefixes=AVX,X64-AVX,AVX512,X64-AVX512
+; RUN: llc < %s -mtriple=i386-unknown-unknown -mattr=+sse2 -use-constant-int-for-fixed-length-splat -use-constant-fp-for-fixed-length-splat | FileCheck %s --check-prefixes=SSE,X86-SSE
+
; Tests for SSE2 and below, without SSE3+.
define void @test1(ptr %r, ptr %A, double %B) nounwind {
diff --git a/llvm/test/CodeGen/X86/vector-shuffle-128-v16.ll b/llvm/test/CodeGen/X86/vector-shuffle-128-v16.ll
index 442bfde6a9e2e..152814fbc631b 100644
--- a/llvm/test/CodeGen/X86/vector-shuffle-128-v16.ll
+++ b/llvm/test/CodeGen/X86/vector-shuffle-128-v16.ll
@@ -13,6 +13,8 @@
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+xop,+avx | FileCheck %s --check-prefixes=ALL,AVX,AVX1OR2,XOP,XOPAVX1
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+xop,+avx2 | FileCheck %s --check-prefixes=ALL,AVX,AVX1OR2,XOP,XOPAVX2
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -use-constant-int-for-fixed-length-splat -use-constant-fp-for-fixed-length-splat | FileCheck %s --check-prefixes=ALL,SSE,SSE2
+
define <16 x i8> @shuffle_v16i8_00_00_00_00_00_00_00_00_00_00_00_00_00_00_00_00(<16 x i8> %a, <16 x i8> %b) {
; SSE2-LABEL: shuffle_v16i8_00_00_00_00_00_00_00_00_00_00_00_00_00_00_00_00:
; SSE2: # %bb.0:
|
Co-authored-by: Matt Arsenault <arsenm2@gmail.com>
| if (auto *CInt = dyn_cast<ConstantInt>(C)) | ||
| if (auto *CInt = dyn_cast<ConstantInt>(C)) { | ||
| if (isa<VectorType>(CInt->getType())) | ||
| return APInt::getSplat(NumBits, CInt->getValue()); |
There was a problem hiding this comment.
I'm not sure I understand what this is doing. How is this different than the direct getValue? How is this a splat if APInt doesn't have vectors?
There was a problem hiding this comment.
My guess is that vector constants are represented as vector-size-integers. I just copied the handling done for ConstantVector based splats.
There was a problem hiding this comment.
Instead of having to do this everywhere, could we not add a helper function to ConstantInt/ConstantFP ?
There was a problem hiding this comment.
No objections but is it worth waiting to see what everywhere means first? because up to press this is the first time I've needed this idiom and the IR side is mostly complete.
I don't understand what you mean. I've added RUN lines to show the output now remains unchanged when the new code paths are enabled. |
We usually test what modified by the change rather than silence test cases. Only NFC change don't need to test. |
I see and thanks for the explanation. The problem I have is my changes are to core IR and during all previous refactoring it has been acceptable (dare I say sensible) to add RUN lines to existing tests to verify the new code paths do not change behaviour. The only time I've needed to add new tests are for cases where the existing ones do not provably exercise the affected code. In this instance pretty much all the X86 vector tests exercise the code and so I just picked two which I know fail if any part of the PR is removed. The new RUN lines will be removed when the new code paths are enabled by default. Is this agreeable? or should I copy the relevant tests into a dedicated file? |
Sounds like it's just a preventive patch for the follow up patches. Then I think you can add an NFC in the title. |
paulwalker-arm
changed the title
When -use-constant-{int,fp}-for-fixed-length-splat are enabled, constant vector splats take the form of ConstantInt/FP instead of ConstantVector. These constants get linked to MachineInstrs via constant pools for later processing. The processing assumes ConstantInt/FP to always represent scalar constants with this PR extending the code to support vector types.
NOTE: The test choices are somewhat artificial because pretty much all the vector tests failed without these changes when the new constants are enabled.