-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AArch64] Lower extending sitofp using tbl #92528
Conversation
@llvm/pr-subscribers-backend-aarch64 Author: Momchil Velikov (momchil-velikov) ChangesIn a similar manner as in https://reviews.llvm.org/D133494
Full diff: https://github.com/llvm/llvm-project/pull/92528.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 339a1f1f2f002..4c52dbfa23903 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -8333,7 +8333,8 @@ bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
if (OptimizeNoopCopyExpression(CI, *TLI, *DL))
return true;
- if ((isa<UIToFPInst>(I) || isa<FPToUIInst>(I) || isa<TruncInst>(I)) &&
+ if ((isa<UIToFPInst>(I) || isa<SIToFPInst>(I) || isa<FPToUIInst>(I) ||
+ isa<TruncInst>(I)) &&
TLI->optimizeExtendOrTruncateConversion(
I, LI->getLoopFor(I->getParent()), *TTI))
return true;
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 1e0071fffe666..619616869b8b3 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -15691,48 +15691,69 @@ bool AArch64TargetLowering::shouldSinkOperands(
return false;
}
-static bool createTblShuffleForZExt(ZExtInst *ZExt, FixedVectorType *DstTy,
- bool IsLittleEndian) {
- Value *Op = ZExt->getOperand(0);
- auto *SrcTy = cast<FixedVectorType>(Op->getType());
- auto SrcWidth = cast<IntegerType>(SrcTy->getElementType())->getBitWidth();
- auto DstWidth = cast<IntegerType>(DstTy->getElementType())->getBitWidth();
+static bool createTblShuffleMask(unsigned SrcWidth, unsigned DstWidth,
+ unsigned NumElts, bool IsLittleEndian,
+ SmallVectorImpl<int> &Mask) {
if (DstWidth % 8 != 0 || DstWidth <= 16 || DstWidth >= 64)
return false;
- assert(DstWidth % SrcWidth == 0 &&
- "TBL lowering is not supported for a ZExt instruction with this "
- "source & destination element type.");
- unsigned ZExtFactor = DstWidth / SrcWidth;
+ if (DstWidth % SrcWidth != 0)
+ return false;
+
+ unsigned Factor = DstWidth / SrcWidth;
+ unsigned MaskLen = NumElts * Factor;
+
+ Mask.clear();
+ Mask.resize(MaskLen, NumElts);
+
+ unsigned SrcIndex = 0;
+ for (unsigned I = 0; I < MaskLen; I += Factor)
+ Mask[I] = SrcIndex++;
+
+ if (!IsLittleEndian)
+ std::rotate(Mask.rbegin(), Mask.rbegin() + Factor - 1, Mask.rend());
+
+ return true;
+}
+
+static Value *createTblShuffleForZExt(IRBuilderBase &Builder, Value *Op,
+ FixedVectorType *ZExtTy,
+ FixedVectorType *DstTy,
+ bool IsLittleEndian) {
+ auto *SrcTy = cast<FixedVectorType>(Op->getType());
unsigned NumElts = SrcTy->getNumElements();
- IRBuilder<> Builder(ZExt);
+ auto SrcWidth = cast<IntegerType>(SrcTy->getElementType())->getBitWidth();
+ auto DstWidth = cast<IntegerType>(DstTy->getElementType())->getBitWidth();
+
SmallVector<int> Mask;
- // Create a mask that selects <0,...,Op[i]> for each lane of the destination
- // vector to replace the original ZExt. This can later be lowered to a set of
- // tbl instructions.
- for (unsigned i = 0; i < NumElts * ZExtFactor; i++) {
- if (IsLittleEndian) {
- if (i % ZExtFactor == 0)
- Mask.push_back(i / ZExtFactor);
- else
- Mask.push_back(NumElts);
- } else {
- if ((i + 1) % ZExtFactor == 0)
- Mask.push_back((i - ZExtFactor + 1) / ZExtFactor);
- else
- Mask.push_back(NumElts);
- }
- }
+ if (!createTblShuffleMask(SrcWidth, DstWidth, NumElts, IsLittleEndian, Mask))
+ return nullptr;
auto *FirstEltZero = Builder.CreateInsertElement(
PoisonValue::get(SrcTy), Builder.getInt8(0), uint64_t(0));
Value *Result = Builder.CreateShuffleVector(Op, FirstEltZero, Mask);
Result = Builder.CreateBitCast(Result, DstTy);
- if (DstTy != ZExt->getType())
- Result = Builder.CreateZExt(Result, ZExt->getType());
- ZExt->replaceAllUsesWith(Result);
- ZExt->eraseFromParent();
- return true;
+ if (DstTy != ZExtTy)
+ Result = Builder.CreateZExt(Result, ZExtTy);
+ return Result;
+}
+
+static Value *createTblShuffleForSExt(IRBuilderBase &Builder, Value *Op,
+ FixedVectorType *DstTy,
+ bool IsLittleEndian) {
+ auto *SrcTy = cast<FixedVectorType>(Op->getType());
+ auto SrcWidth = cast<IntegerType>(SrcTy->getElementType())->getBitWidth();
+ auto DstWidth = cast<IntegerType>(DstTy->getElementType())->getBitWidth();
+
+ SmallVector<int> Mask;
+ if (!createTblShuffleMask(SrcWidth, DstWidth, SrcTy->getNumElements(),
+ !IsLittleEndian, Mask))
+ return nullptr;
+
+ auto *FirstEltZero = Builder.CreateInsertElement(
+ PoisonValue::get(SrcTy), Builder.getInt8(0), uint64_t(0));
+
+ return Builder.CreateShuffleVector(Op, FirstEltZero, Mask);
}
static void createTblForTrunc(TruncInst *TI, bool IsLittleEndian) {
@@ -15897,21 +15918,47 @@ bool AArch64TargetLowering::optimizeExtendOrTruncateConversion(
DstTy = TruncDstType;
}
-
- return createTblShuffleForZExt(ZExt, DstTy, Subtarget->isLittleEndian());
+ IRBuilder<> Builder(ZExt);
+ Value *Result = createTblShuffleForZExt(
+ Builder, ZExt->getOperand(0), cast<FixedVectorType>(ZExt->getType()),
+ DstTy, Subtarget->isLittleEndian());
+ if (!Result)
+ return false;
+ ZExt->replaceAllUsesWith(Result);
+ ZExt->eraseFromParent();
+ return true;
}
auto *UIToFP = dyn_cast<UIToFPInst>(I);
if (UIToFP && SrcTy->getElementType()->isIntegerTy(8) &&
DstTy->getElementType()->isFloatTy()) {
IRBuilder<> Builder(I);
- auto *ZExt = cast<ZExtInst>(
- Builder.CreateZExt(I->getOperand(0), VectorType::getInteger(DstTy)));
+ Value *ZExt = createTblShuffleForZExt(
+ Builder, I->getOperand(0), FixedVectorType::getInteger(DstTy),
+ FixedVectorType::getInteger(DstTy), Subtarget->isLittleEndian());
+ if (!ZExt)
+ return false;
auto *UI = Builder.CreateUIToFP(ZExt, DstTy);
I->replaceAllUsesWith(UI);
I->eraseFromParent();
- return createTblShuffleForZExt(ZExt, cast<FixedVectorType>(ZExt->getType()),
- Subtarget->isLittleEndian());
+ return true;
+ }
+
+ auto *SIToFP = dyn_cast<SIToFPInst>(I);
+ if (SIToFP && SrcTy->getElementType()->isIntegerTy(8) &&
+ DstTy->getElementType()->isFloatTy()) {
+ IRBuilder<> Builder(I);
+ auto *Shuffle = createTblShuffleForSExt(Builder, I->getOperand(0),
+ FixedVectorType::getInteger(DstTy),
+ Subtarget->isLittleEndian());
+ if (!Shuffle)
+ return false;
+ auto *Cast = Builder.CreateBitCast(Shuffle, VectorType::getInteger(DstTy));
+ auto *AShr = Builder.CreateAShr(Cast, 24);
+ auto *SI = Builder.CreateSIToFP(AShr, DstTy);
+ I->replaceAllUsesWith(SI);
+ I->eraseFromParent();
+ return true;
}
// Convert 'fptoui <(8|16) x float> to <(8|16) x i8>' to a wide fptoui
@@ -17840,6 +17887,26 @@ static SDValue performVectorCompareAndMaskUnaryOpCombine(SDNode *N,
return SDValue();
}
+static SDValue performVectorIntToFpCombine(SDNode *N, SelectionDAG &DAG) {
+ if (N->getOpcode() != ISD::SINT_TO_FP || N->getValueType(0) != MVT::v4f32)
+ return SDValue();
+
+ SDNode *VASHR = N->getOperand(0).getNode();
+ if (VASHR->getOpcode() != AArch64ISD::VASHR ||
+ VASHR->getValueType(0) != MVT::v4i32)
+ return SDValue();
+
+ if (!isa<ConstantSDNode>(VASHR->getOperand(1).getNode()) ||
+ VASHR->getConstantOperandVal(1) != 24)
+ return SDValue();
+
+ return SDValue(DAG.getMachineNode(
+ AArch64::SCVTFv4i32_shift, SDLoc(N), N->getValueType(0),
+ {VASHR->getOperand(0),
+ DAG.getTargetConstant(24, SDLoc(N), MVT::i32)}),
+ 0);
+}
+
static SDValue performIntToFpCombine(SDNode *N, SelectionDAG &DAG,
const AArch64Subtarget *Subtarget) {
// First try to optimize away the conversion when it's conditionally from
@@ -17847,6 +17914,9 @@ static SDValue performIntToFpCombine(SDNode *N, SelectionDAG &DAG,
if (SDValue Res = performVectorCompareAndMaskUnaryOpCombine(N, DAG))
return Res;
+ if (SDValue Res = performVectorIntToFpCombine(N, DAG))
+ return Res;
+
EVT VT = N->getValueType(0);
if (VT != MVT::f32 && VT != MVT::f64)
return SDValue();
diff --git a/llvm/test/CodeGen/AArch64/sitofp-to-tbl.ll b/llvm/test/CodeGen/AArch64/sitofp-to-tbl.ll
new file mode 100644
index 0000000000000..dbf15ab9936f6
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/sitofp-to-tbl.ll
@@ -0,0 +1,196 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -verify-machineinstrs < %s | FileCheck %s
+
+target triple = "aarch64-linux"
+
+; CHECK-LABEL: .LCPI0_0:
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 4
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 5
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 6
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 7
+; CHECK-NEXT: .LCPI0_1:
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 0
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 1
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 2
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+
+define void @sitofp_v8i8_to_v8f32(ptr %src, ptr %dst) {
+; CHECK-LABEL: sitofp_v8i8_to_v8f32:
+; CHECK: // %bb.0: // %entry
+; CHECK-NEXT: adrp x8, .LCPI0_0
+; CHECK-NEXT: adrp x9, .LCPI0_1
+; CHECK-NEXT: ldr q0, [x8, :lo12:.LCPI0_0]
+; CHECK-NEXT: ldr q1, [x9, :lo12:.LCPI0_1]
+; CHECK-NEXT: mov x8, xzr
+; CHECK-NEXT: .LBB0_1: // %loop
+; CHECK-NEXT: // =>This Inner Loop Header: Depth=1
+; CHECK-NEXT: ldr d2, [x0, x8, lsl #3]
+; CHECK-NEXT: add x9, x1, x8, lsl #5
+; CHECK-NEXT: add x8, x8, #1
+; CHECK-NEXT: cmp x8, #1000
+; CHECK-NEXT: tbl v3.16b, { v2.16b }, v0.16b
+; CHECK-NEXT: tbl v2.16b, { v2.16b }, v1.16b
+; CHECK-NEXT: scvtf v3.4s, v3.4s, #24
+; CHECK-NEXT: scvtf v2.4s, v2.4s, #24
+; CHECK-NEXT: stp q2, q3, [x9]
+; CHECK-NEXT: b.eq .LBB0_1
+; CHECK-NEXT: // %bb.2: // %exit
+; CHECK-NEXT: ret
+entry:
+ br label %loop
+
+loop:
+ %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop ]
+ %gep.src = getelementptr inbounds <8 x i8>, ptr %src, i64 %iv
+ %l = load <8 x i8>, ptr %gep.src
+ %conv = sitofp <8 x i8> %l to <8 x float>
+ %gep.dst = getelementptr inbounds <8 x float>, ptr %dst, i64 %iv
+ store <8 x float> %conv, ptr %gep.dst
+ %iv.next = add i64 %iv, 1
+ %ec = icmp eq i64 %iv.next, 1000
+ br i1 %ec, label %loop, label %exit
+
+exit:
+ ret void
+}
+
+; CHECK-LABEL: .LCPI1_0:
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 12
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 13
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 14
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 15
+; CHECK-NEXT: .LCPI1_1:
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 8
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 9
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 10
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 11
+; CHECK-NEXT: .LCPI1_2:
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 4
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 5
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 6
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 7
+; CHECK-NEXT: .LCPI1_3:
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 0
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 1
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 2
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 255
+; CHECK-NEXT: .byte 3
+
+define void @sitofp_v16i8_to_v16f32(ptr %src, ptr %dst) {
+; CHECK-LABEL: sitofp_v16i8_to_v16f32:
+; CHECK: // %bb.0: // %entry
+; CHECK-NEXT: adrp x8, .LCPI1_0
+; CHECK-NEXT: adrp x9, .LCPI1_1
+; CHECK-NEXT: adrp x10, .LCPI1_2
+; CHECK-NEXT: ldr q0, [x8, :lo12:.LCPI1_0]
+; CHECK-NEXT: adrp x8, .LCPI1_3
+; CHECK-NEXT: ldr q1, [x9, :lo12:.LCPI1_1]
+; CHECK-NEXT: ldr q2, [x10, :lo12:.LCPI1_2]
+; CHECK-NEXT: ldr q3, [x8, :lo12:.LCPI1_3]
+; CHECK-NEXT: mov x8, xzr
+; CHECK-NEXT: .LBB1_1: // %loop
+; CHECK-NEXT: // =>This Inner Loop Header: Depth=1
+; CHECK-NEXT: ldr q4, [x0, x8, lsl #4]
+; CHECK-NEXT: add x9, x1, x8, lsl #6
+; CHECK-NEXT: add x8, x8, #1
+; CHECK-NEXT: cmp x8, #1000
+; CHECK-NEXT: tbl v5.16b, { v4.16b }, v0.16b
+; CHECK-NEXT: tbl v6.16b, { v4.16b }, v1.16b
+; CHECK-NEXT: tbl v7.16b, { v4.16b }, v2.16b
+; CHECK-NEXT: tbl v4.16b, { v4.16b }, v3.16b
+; CHECK-NEXT: scvtf v5.4s, v5.4s, #24
+; CHECK-NEXT: scvtf v6.4s, v6.4s, #24
+; CHECK-NEXT: scvtf v7.4s, v7.4s, #24
+; CHECK-NEXT: scvtf v4.4s, v4.4s, #24
+; CHECK-NEXT: stp q6, q5, [x9, #32]
+; CHECK-NEXT: stp q4, q7, [x9]
+; CHECK-NEXT: b.eq .LBB1_1
+; CHECK-NEXT: // %bb.2: // %exit
+; CHECK-NEXT: ret
+entry:
+ br label %loop
+
+loop:
+ %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop ]
+ %gep.src = getelementptr inbounds <16 x i8>, ptr %src, i64 %iv
+ %l = load <16 x i8>, ptr %gep.src
+ %conv = sitofp <16 x i8> %l to <16 x float>
+ %gep.dst = getelementptr inbounds <16 x float>, ptr %dst, i64 %iv
+ store <16 x float> %conv, ptr %gep.dst
+ %iv.next = add i64 %iv, 1
+ %ec = icmp eq i64 %iv.next, 1000
+ br i1 %ec, label %loop, label %exit
+
+exit:
+ ret void
+}
|
e6cfa82
to
550b838
Compare
Ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this operates on sitofp I think it should be OK. LGTM
b17f3b6
to
fec1355
Compare
... in preparation for #92528
In a similar manner as in https://reviews.llvm.org/D133494 use `TBL` to place bytes in the *upper* part of `i32` elements and then convert to float using fixed-point `scvtf`, i.e. scvtf Vd.4s, Vn.4s, llvm#24 Change-Id: Ib9df3e4243612cbee8560907b24b14e76b61f265
Hi, at Google we are currently seeing two test failures after this commit:
We are still trying to reduce the Eigen3 test, which is pure C++. I know that this change was landed over a week ago, but with the two differences both point to this commit, is it possible to roll it back until further investigation? [edit] Actually, the Eigen failure is not internal -- the test infra is, but the test that fails is https://gitlab.com/libeigen/eigen/-/blob/master/test/array_cwise.cpp?ref_type=heads |
This reverts commit d1a4f0c. There are reports about test failures with Eigen and JAX.
Thank you! I'm working on reducing the case. |
The .ii file is still over 500k bytes long because eigen is a header library. But I've seen the assembly diff boils down to two snippets of exactly this. Before this commit
At this commit:
The other part is identical except for the input registers (
I'm not sure how the two snippets behave differently. And if they do behave differently, it feels like a latent bug uncovered by this commit? |
Oh, "MOV (from general)" used in the new assembly sequences does not clear other bits, while the shift left then right method does, doesn't it? That would explain that the differences seem to be precision loss. |
I'm pretty confident that this is a pre-existing bug somewhere in instruction selection that was revealed by this commit.
The instruction after this commit, Here's the repro: https://godbolt.org/z/686fbPjKz
results in
|
Thank you very much, that's incredibly helpful ! |
This reverts commit d1a4f0c. There are reports about test failures with Eigen and JAX.
When building a vector which contains zero elements, the AArch64 ISel replaces those elements with `undef`, if they are right shifted out. However, these elements need to stay zero if the right shift is exact, or otherwise we will be introducing undefined behavior. Should allow #92528 to be recommitted.
Should be OK to re-commit after #97448 |
When building a vector which contains zero elements, the AArch64 ISel replaces those elements with `undef`, if they are right shifted out. However, these elements need to stay zero if the right shift is exact, or otherwise we will be introducing undefined behavior. Should allow llvm#92528 to be recommitted.
This reverts commit d1a4f0c. There are reports about test failures with Eigen and JAX.
In a similar manner as in https://reviews.llvm.org/D133494
use
TBL
to place bytes in the upper part ofi32
elementsand then convert to float using fixed-point
scvtf
, i.e.