release/19.x: [PowerPC] Respect endianness when bitcasting to fp128 (#95931) #105623
Conversation
|
@chenzheng1030 What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-backend-powerpc Author: None (llvmbot) ChangesBackport 408d82d Requested by: @chenzheng1030 Full diff: https://github.com/llvm/llvm-project/pull/105623.diff 2 Files Affected:
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index aaf0449a55387f..21cf4d9eeac173 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -9338,14 +9338,18 @@ SDValue PPCTargetLowering::LowerBITCAST(SDValue Op, SelectionDAG &DAG) const {
SDLoc dl(Op);
SDValue Op0 = Op->getOperand(0);
+ SDValue Lo = Op0.getOperand(0);
+ SDValue Hi = Op0.getOperand(1);
+
if ((Op.getValueType() != MVT::f128) ||
- (Op0.getOpcode() != ISD::BUILD_PAIR) ||
- (Op0.getOperand(0).getValueType() != MVT::i64) ||
- (Op0.getOperand(1).getValueType() != MVT::i64) || !Subtarget.isPPC64())
+ (Op0.getOpcode() != ISD::BUILD_PAIR) || (Lo.getValueType() != MVT::i64) ||
+ (Hi.getValueType() != MVT::i64) || !Subtarget.isPPC64())
return SDValue();
- return DAG.getNode(PPCISD::BUILD_FP128, dl, MVT::f128, Op0.getOperand(0),
- Op0.getOperand(1));
+ if (!Subtarget.isLittleEndian())
+ std::swap(Lo, Hi);
+
+ return DAG.getNode(PPCISD::BUILD_FP128, dl, MVT::f128, Lo, Hi);
}
static const SDValue *getNormalLoadInput(const SDValue &Op, bool &IsPermuted) {
diff --git a/llvm/test/CodeGen/PowerPC/f128-aggregates.ll b/llvm/test/CodeGen/PowerPC/f128-aggregates.ll
index b3d2457d31eebc..4be855e30ea1d4 100644
--- a/llvm/test/CodeGen/PowerPC/f128-aggregates.ll
+++ b/llvm/test/CodeGen/PowerPC/f128-aggregates.ll
@@ -283,7 +283,7 @@ define fp128 @testMixedAggregate([3 x i128] %a.coerce) {
;
; CHECK-BE-LABEL: testMixedAggregate:
; CHECK-BE: # %bb.0: # %entry
-; CHECK-BE-NEXT: mtvsrdd v2, r8, r7
+; CHECK-BE-NEXT: mtvsrdd v2, r7, r8
; CHECK-BE-NEXT: blr
;
; CHECK-P8-LABEL: testMixedAggregate:
@@ -310,7 +310,7 @@ define fp128 @testMixedAggregate_02([4 x i128] %a.coerce) {
;
; CHECK-BE-LABEL: testMixedAggregate_02:
; CHECK-BE: # %bb.0: # %entry
-; CHECK-BE-NEXT: mtvsrdd v2, r6, r5
+; CHECK-BE-NEXT: mtvsrdd v2, r5, r6
; CHECK-BE-NEXT: blr
;
; CHECK-P8-LABEL: testMixedAggregate_02:
@@ -344,7 +344,7 @@ define fp128 @testMixedAggregate_03([4 x i128] %sa.coerce) {
; CHECK-BE-LABEL: testMixedAggregate_03:
; CHECK-BE: # %bb.0: # %entry
; CHECK-BE-NEXT: mtvsrwa v2, r4
-; CHECK-BE-NEXT: mtvsrdd v3, r6, r5
+; CHECK-BE-NEXT: mtvsrdd v3, r5, r6
; CHECK-BE-NEXT: xscvsdqp v2, v2
; CHECK-BE-NEXT: xsaddqp v2, v3, v2
; CHECK-BE-NEXT: mtvsrd v3, r9
@@ -467,7 +467,7 @@ define fp128 @testUnion_01([1 x i128] %a.coerce) {
;
; CHECK-BE-LABEL: testUnion_01:
; CHECK-BE: # %bb.0: # %entry
-; CHECK-BE-NEXT: mtvsrdd v2, r4, r3
+; CHECK-BE-NEXT: mtvsrdd v2, r3, r4
; CHECK-BE-NEXT: blr
;
; CHECK-P8-LABEL: testUnion_01:
@@ -494,7 +494,7 @@ define fp128 @testUnion_02([1 x i128] %a.coerce) {
;
; CHECK-BE-LABEL: testUnion_02:
; CHECK-BE: # %bb.0: # %entry
-; CHECK-BE-NEXT: mtvsrdd v2, r4, r3
+; CHECK-BE-NEXT: mtvsrdd v2, r3, r4
; CHECK-BE-NEXT: blr
;
; CHECK-P8-LABEL: testUnion_02:
@@ -521,7 +521,7 @@ define fp128 @testUnion_03([4 x i128] %a.coerce) {
;
; CHECK-BE-LABEL: testUnion_03:
; CHECK-BE: # %bb.0: # %entry
-; CHECK-BE-NEXT: mtvsrdd v2, r8, r7
+; CHECK-BE-NEXT: mtvsrdd v2, r7, r8
; CHECK-BE-NEXT: blr
;
; CHECK-P8-LABEL: testUnion_03:
|
|
@chenzheng1030 Hi! Thanks for the backport. What's the impact of this PR and is it a regression fix or a major issue fix? At this point in the release we don't want to be to experimental. I would also love a review from someone with knowledge of the domain before I merge. |
|
(I'm not really involved with LLVM but I am doing a lot of f128 work and requested the backport) This seems unlikely to be a regression. There have a handful of f128-related bugs on various ppc platforms so I suspect this is just something that hadn't been tested before now. The problem this fixes is pretty fundamental, it means any sort of soft float operations are completely broken on BE ppc targets. This is how I originally identified the issue, in rust-lang/compiler-builtins#606 (comment). Backporting isn't critical; at this time it probably only affects those of us that are experimenting with f128 support, there can't be many/any end users. However, it would be nice to have this sooner rather than later since ppc is about the last popular linux target to still have bugs (our more complete list: https://github.com/rust-lang/rust/blob/3f121b9461cce02a703a0e7e450568849dfaa074/library/std/build.rs#L123-L141). Also, I can't speak for the code changes but the assembly changes do look correct (the problem was halves of the value getting reversed, the diff swaps them). I don't think this would be experimental, but in any case it can't be worse than the status quo. |
Fixes llvm#92246 Match the behaviour of `bitcast v2i64 (BUILD_PAIR %lo %hi)` when encountering `bitcast fp128 (BUILD_PAIR %lo $hi)`. by inserting a missing swap of the arguments based on endianness. ### Current behaviour: **fp128** bitcast fp128 (BUILD_PAIR %lo $hi) => BUILD_FP128 %lo %hi BUILD_FP128 %lo %hi => MTVSRDD %hi %lo **v2i64** bitcast v2i64 (BUILD_PAIR %lo %hi) => BUILD_VECTOR %hi %lo BUILD_VECTOR %hi %lo => MTVSRDD %lo %hi (cherry picked from commit 408d82d)
|
@chenzheng1030 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
Backport 408d82d
Requested by: @chenzheng1030