[SPARC] Fix fp128 load/stores

[LemonBoy]

Summary

The generated code for the split fp128 load/stores was missing a small yet important adjustment to the pointer metadata being fed into getStore and getLoad, making it out of sync with the effective memory address.
This problem often resulted in instructions being scheduled in the wrong order.

I also took this chance to clean up some "wrong" uses of getAlignment as done in D77687.

Thanks @jrtc27 for finding the problem and providing a patch.

[craig.topper]

llvm/lib/Target/Sparc/SparcISelLowering.cpp

2735–2736

This dyn_cast is only checked by the assert below. The rest of the code just blindly dereferences it.

2787–2788

Same here

[LemonBoy]

The test is not so solid (is it?), ideally we should check the MI to make sure the load ops have the correct offset but I have no idea of what kind of test does that.

[jrtc27]

In D94345#2488047, @LemonBoy wrote:

The test is not so solid (is it?), ideally we should check the MI to make sure the load ops have the correct offset but I have no idea of what kind of test does that.

You want a MIR test I believe (though probably still needs IR input, SelectionDAG is a bit hard to target specific parts of).

[craig.topper]

In D94345#2488047, @LemonBoy wrote:

The test is not so solid (is it?), ideally we should check the MI to make sure the load ops have the correct offset but I have no idea of what kind of test does that.

You can use "-stop-after=finalize-isel" and check the MIR output which should print the memory operands. I think maybe update_mir_test_checks.py can generate the check lines for MIR output from a .ll test.

[jrtc27]

llvm/lib/Target/Sparc/SparcISelLowering.cpp

2735–2736

If you're looking for code quality don't look at the SPARC backend... it does suffer somewhat from being the first ever backend (and not having a lot of people care about it these days to clean it up).

[LemonBoy]

• Add MIR test
• Replace dyn_cast<> with cast<>

[LemonBoy]

You can use "-stop-after=finalize-isel" and check the MIR output which should print the memory operands. I think maybe update_mir_test_checks.py can generate the check lines for MIR output from a .ll test.

That's perfect, the test is much better now.

llvm/lib/Target/Sparc/SparcISelLowering.cpp

2735–2736

Turned that into a asserting cast<>.

[craig.topper]

This might need to be rebased after b1c304c4946506c0d00532829fb2f91276dde0c8 where I changed the printing of the memory operands in MIR

[LemonBoy]

Rebased after changes in master, feel free to land the changes right away as I have no commit access (yet?).

[craig.topper]

LGTM

llvm/lib/Target/Sparc/SparcISelLowering.cpp

static SDValue LowerF64Op(SDValue SrcReg64, const SDLoc &dl, SelectionDAG &DAG,
   DstReg64 = DAG.getTargetInsertSubreg(SP::sub_odd, dl, MVT::f64,
                                        DstReg64, Lo32);
   return DstReg64;
 }
 
 // Lower a f128 load into two f64 loads.
 static SDValue LowerF128Load(SDValue Op, SelectionDAG &DAG)
 {
   SDLoc dl(Op);
-  LoadSDNode *LdNode = dyn_cast<LoadSDNode>(Op.getNode());
-  assert(LdNode && LdNode->getOffset().isUndef()
-         && "Unexpected node type");
-
-  unsigned alignment = LdNode->getAlignment();
-  if (alignment > 8)
-    alignment = 8;
+  LoadSDNode *LdNode = cast<LoadSDNode>(Op.getNode());
+  assert(LdNode->getOffset().isUndef() && "Unexpected node type");
+
+  Align Alignment = commonAlignment(LdNode->getOriginalAlign(), 8);
 
   SDValue Hi64 =
       DAG.getLoad(MVT::f64, dl, LdNode->getChain(), LdNode->getBasePtr(),
-                  LdNode->getPointerInfo(), alignment);
+                  LdNode->getPointerInfo(), Alignment);
   EVT addrVT = LdNode->getBasePtr().getValueType();
   SDValue LoPtr = DAG.getNode(ISD::ADD, dl, addrVT,
                               LdNode->getBasePtr(),
                               DAG.getConstant(8, dl, addrVT));
   SDValue Lo64 = DAG.getLoad(MVT::f64, dl, LdNode->getChain(), LoPtr,
-                             LdNode->getPointerInfo(), alignment);
+                             LdNode->getPointerInfo().getWithOffset(8),
+                             Alignment);
 
   SDValue SubRegEven = DAG.getTargetConstant(SP::sub_even64, dl, MVT::i32);
   SDValue SubRegOdd  = DAG.getTargetConstant(SP::sub_odd64, dl, MVT::i32);
 
   SDNode *InFP128 = DAG.getMachineNode(TargetOpcode::IMPLICIT_DEF,
                                        dl, MVT::f128);
   InFP128 = DAG.getMachineNode(TargetOpcode::INSERT_SUBREG, dl,
                                MVT::f128,
static SDValue LowerLOAD(SDValue Op, SelectionDAG &DAG)
   if (MemVT == MVT::f128)
     return LowerF128Load(Op, DAG);
 
   return Op;
 }
 
 // Lower a f128 store into two f64 stores.
 static SDValue LowerF128Store(SDValue Op, SelectionDAG &DAG) {
   SDLoc dl(Op);
-  StoreSDNode *StNode = dyn_cast<StoreSDNode>(Op.getNode());
-  assert(StNode && StNode->getOffset().isUndef()
-         && "Unexpected node type");
+  StoreSDNode *StNode = cast<StoreSDNode>(Op.getNode());
+  assert(StNode->getOffset().isUndef() && "Unexpected node type");
+
   SDValue SubRegEven = DAG.getTargetConstant(SP::sub_even64, dl, MVT::i32);
   SDValue SubRegOdd  = DAG.getTargetConstant(SP::sub_odd64, dl, MVT::i32);
 
   SDNode *Hi64 = DAG.getMachineNode(TargetOpcode::EXTRACT_SUBREG,
                                     dl,
                                     MVT::f64,
                                     StNode->getValue(),
                                     SubRegEven);
   SDNode *Lo64 = DAG.getMachineNode(TargetOpcode::EXTRACT_SUBREG,
                                     dl,
                                     MVT::f64,
                                     StNode->getValue(),
                                     SubRegOdd);
 
-  unsigned alignment = StNode->getAlignment();
-  if (alignment > 8)
-    alignment = 8;
+  Align Alignment = commonAlignment(StNode->getOriginalAlign(), 8);
 
   SDValue OutChains[2];
   OutChains[0] =
       DAG.getStore(StNode->getChain(), dl, SDValue(Hi64, 0),
-                   StNode->getBasePtr(), MachinePointerInfo(), alignment);
+                   StNode->getBasePtr(), StNode->getPointerInfo(),
+                   Alignment);
   EVT addrVT = StNode->getBasePtr().getValueType();
   SDValue LoPtr = DAG.getNode(ISD::ADD, dl, addrVT,
                               StNode->getBasePtr(),
                               DAG.getConstant(8, dl, addrVT));
   OutChains[1] = DAG.getStore(StNode->getChain(), dl, SDValue(Lo64, 0), LoPtr,
-                              MachinePointerInfo(), alignment);
+                              StNode->getPointerInfo().getWithOffset(8),
+                              Alignment);
   return DAG.getNode(ISD::TokenFactor, dl, MVT::Other, OutChains);
 }
 
 static SDValue LowerSTORE(SDValue Op, SelectionDAG &DAG)
 {
   SDLoc dl(Op);
   StoreSDNode *St = cast<StoreSDNode>(Op.getNode());
 
   EVT MemVT = St->getMemoryVT();
   if (MemVT == MVT::f128)
     return LowerF128Store(Op, DAG);
 
   if (MemVT == MVT::i64) {
     // Custom handling for i64 stores: turn it into a bitcast and a
     // v2i32 store.
     SDValue Val = DAG.getNode(ISD::BITCAST, dl, MVT::v2i32, St->getValue());
     SDValue Chain = DAG.getStore(
         St->getChain(), dl, Val, St->getBasePtr(), St->getPointerInfo(),
-        St->getAlignment(), St->getMemOperand()->getFlags(), St->getAAInfo());
+        St->getOriginalAlign(), St->getMemOperand()->getFlags(),
+        St->getAAInfo());
     return Chain;
   }
 
   return SDValue();
 }
 
 static SDValue LowerFNEGorFABS(SDValue Op, SelectionDAG &DAG, bool isV9) {
   assert((Op.getOpcode() == ISD::FNEG || Op.getOpcode() == ISD::FABS)
case ISD::LOAD: {
     // Custom handling only for i64: turn i64 load into a v2i32 load,
     // and a bitcast.
     if (Ld->getValueType(0) != MVT::i64 || Ld->getMemoryVT() != MVT::i64)
       return;
 
     SDLoc dl(N);
     SDValue LoadRes = DAG.getExtLoad(
         Ld->getExtensionType(), dl, MVT::v2i32, Ld->getChain(),
-        Ld->getBasePtr(), Ld->getPointerInfo(), MVT::v2i32, Ld->getAlignment(),
-        Ld->getMemOperand()->getFlags(), Ld->getAAInfo());
+        Ld->getBasePtr(), Ld->getPointerInfo(), MVT::v2i32,
+        Ld->getOriginalAlign(), Ld->getMemOperand()->getFlags(),
+        Ld->getAAInfo());
 
     SDValue Res = DAG.getNode(ISD::BITCAST, dl, MVT::i64, LoadRes);
     Results.push_back(Res);
     Results.push_back(LoadRes.getValue(1));
     return;
   }
   }
 }

llvm/test/CodeGen/SPARC/fp128-split.ll

+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -mtriple=sparcv9-unknown-linux -verify-machineinstrs -stop-after=finalize-isel < %s | FileCheck %s
+
+; Check that the fp128 load/store is correctly split.
+; The pointer metadata for the upper/lower halves of the load/store should be in
+; sync with the OP address.
+
+define fp128 @testcase(fp128 %0) {
+  ; CHECK-LABEL: name: testcase
+  ; CHECK: bb.0.Entry:
+  ; CHECK:   liveins: $q0
+  ; CHECK:   [[COPY:%[0-9]+]]:qfpregs = COPY $q0
+  ; CHECK:   [[COPY1:%[0-9]+]]:dfpregs = COPY [[COPY]].sub_odd64
+  ; CHECK:   [[LEAX_ADDri:%[0-9]+]]:i64regs = LEAX_ADDri %stack.0, 0
+  ; CHECK:   [[ORXri:%[0-9]+]]:i64regs = ORXri killed [[LEAX_ADDri]], 8
+  ; CHECK:   STDFrr [[ORXri]], $g0, killed [[COPY1]] :: (store 8 into %stack.0 + 8)
+  ; CHECK:   [[COPY2:%[0-9]+]]:dfpregs = COPY [[COPY]].sub_even64
+  ; CHECK:   STDFri %stack.0, 0, killed [[COPY2]] :: (store 8 into %stack.0, align 16)
+  ; CHECK:   [[LDXrr:%[0-9]+]]:i64regs = LDXrr [[ORXri]], $g0 :: (load 8 from %stack.0 + 8)
+  ; CHECK:   [[LDXri:%[0-9]+]]:i64regs = LDXri %stack.0, 0 :: (load 8 from %stack.0, align 16)
+  ; CHECK:   [[COPY3:%[0-9]+]]:intregs = COPY [[LDXrr]]
+  ; CHECK:   [[COPY4:%[0-9]+]]:intregs = COPY [[LDXri]]
+  ; CHECK:   [[SRLXri:%[0-9]+]]:i64regs = SRLXri [[LDXrr]], 32
+  ; CHECK:   [[COPY5:%[0-9]+]]:intregs = COPY [[SRLXri]]
+  ; CHECK:   [[SRLXri1:%[0-9]+]]:i64regs = SRLXri [[LDXri]], 32
+  ; CHECK:   [[COPY6:%[0-9]+]]:intregs = COPY [[SRLXri1]]
+  ; CHECK:   [[ADDCCri:%[0-9]+]]:intregs = ADDCCri killed [[COPY3]], -1, implicit-def $icc
+  ; CHECK:   [[ADDEri:%[0-9]+]]:intregs = ADDEri killed [[COPY5]], -1, implicit-def $icc, implicit $icc
+  ; CHECK:   [[ADDEri1:%[0-9]+]]:intregs = ADDEri killed [[COPY4]], -1, implicit-def $icc, implicit $icc
+  ; CHECK:   [[ADDEri2:%[0-9]+]]:intregs = ADDEri killed [[COPY6]], -1, implicit-def dead $icc, implicit $icc
+  ; CHECK:   [[SRLri:%[0-9]+]]:i64regs = SRLri killed [[ADDCCri]], 0
+  ; CHECK:   [[COPY7:%[0-9]+]]:i64regs = COPY [[ADDEri]]
+  ; CHECK:   [[SLLXri:%[0-9]+]]:i64regs = SLLXri killed [[COPY7]], 32
+  ; CHECK:   [[ORXrr:%[0-9]+]]:i64regs = ORXrr killed [[SLLXri]], killed [[SRLri]]
+  ; CHECK:   [[LEAX_ADDri1:%[0-9]+]]:i64regs = LEAX_ADDri %stack.1, 0
+  ; CHECK:   [[ORXri1:%[0-9]+]]:i64regs = ORXri killed [[LEAX_ADDri1]], 8
+  ; CHECK:   STXrr [[ORXri1]], $g0, killed [[ORXrr]] :: (store 8 into %stack.1 + 8, basealign 16)
+  ; CHECK:   [[SRLri1:%[0-9]+]]:i64regs = SRLri killed [[ADDEri1]], 0
+  ; CHECK:   [[COPY8:%[0-9]+]]:i64regs = COPY [[ADDEri2]]
+  ; CHECK:   [[SLLXri1:%[0-9]+]]:i64regs = SLLXri killed [[COPY8]], 32
+  ; CHECK:   [[ORXrr1:%[0-9]+]]:i64regs = ORXrr killed [[SLLXri1]], killed [[SRLri1]]
+  ; CHECK:   STXri %stack.1, 0, killed [[ORXrr1]] :: (store 8 into %stack.1, align 16)
+  ; CHECK:   [[LDDFri:%[0-9]+]]:dfpregs = LDDFri %stack.1, 0 :: (load 8 from %stack.1, align 16)
+  ; CHECK:   [[DEF:%[0-9]+]]:qfpregs = IMPLICIT_DEF
+  ; CHECK:   [[INSERT_SUBREG:%[0-9]+]]:qfpregs = INSERT_SUBREG [[DEF]], killed [[LDDFri]], %subreg.sub_even64
+  ; CHECK:   [[LDDFrr:%[0-9]+]]:dfpregs = LDDFrr [[ORXri1]], $g0 :: (load 8 from %stack.1 + 8)
+  ; CHECK:   [[INSERT_SUBREG1:%[0-9]+]]:qfpregs = INSERT_SUBREG [[INSERT_SUBREG]], killed [[LDDFrr]], %subreg.sub_odd64
+  ; CHECK:   $q0 = COPY [[INSERT_SUBREG1]]
+  ; CHECK:   RETL 8, implicit $q0
+Entry:
+  %1 = bitcast fp128 %0 to i128
+  %2 = add i128 %1, -1
+  %3 = bitcast i128 %2 to fp128
+  ret fp128 %3
+}