[Sparc] Custom bitcast between f64 and v2i32

[dcederman]

Summary

Currently bitcasting constants from f64 to v2i32 is done by storing the value to the stack and then loading it again. This is not necessary, but seems to happen because v2i32 is a valid type for Sparc V8. If it had not been legal, we would have gotten help from the type legalizer.

This patch tries to do the same work as the legalizer would have done by bitcasting the floating point constant and splitting the value up into a vector of two i32 values.

[jrtc27]

lib/Target/Sparc/SparcISelLowering.cpp

3061 ↗

(On Diff #155125)

Should we also be checking Src.getSimpleValueType()? This only works if both source and result are v2i32/f64.

[dcederman]

Added assert for Src.getSimpleValueType() and improved formatting.

[jyknight]

lib/Target/Sparc/SparcISelLowering.cpp

3060 ↗

(On Diff #160355)

I think it's probably better to move all of this into a PerformDAGCombine function. That's where the usual target-independent BITCAST constant handling happens, so doing this stuff there too seems reasonable and will make some of the below issues easier.

3070 ↗

(On Diff #160355)

Doesn't this end up duplicating the load instruction? I think the original load won't disappear even if it has no other value uses, because its chain is used (and elimination of loads used only for their chains only happens in DAG Combine, which is already done by this point, possibly).

This also doesn't properly handle the chain output, so the new load might be misordered w.r.t. dependencies. Needs to replace the chain out of the original load with a tokenfactor of both loads.

If the load is volatile, duplicating it is invalid.

3073 ↗

(On Diff #160355)

Is it guaranteed here that VT is only v2i32? Maybe just add that to the if.

3075 ↗

(On Diff #160355)

This code works right in both endiannesses, right? (I forgot how build-vector endianness goes).

test/CodeGen/SPARC/float-constants.ll

32–33 ↗

(On Diff #160355)

Leave a note that there is still non-optimal codegen in this case?

[dcederman]

Moved code into a PerformDAGCombine function.

Removed code related to loads. Loads were already handled properly so the code should not have been included in the first place.

Check both source and target for the correct type.

Added code to handle little-endian systems and added new test cases.

The test_intrins_call test should be generating optimal code now.

llvm/trunk/lib/Target/Sparc/SparcISelLowering.h

public:
     SDValue LowerF128Op(SDValue Op, SelectionDAG &DAG,
                         const char *LibFuncName,
                         unsigned numArgs) const;
     SDValue LowerF128Compare(SDValue LHS, SDValue RHS, unsigned &SPCC,
                              const SDLoc &DL, SelectionDAG &DAG) const;
 
     SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
 
+    SDValue PerformBITCASTCombine(SDNode *N, DAGCombinerInfo &DCI) const;
+
+    SDValue bitcastConstantFPToInt(ConstantFPSDNode *C, const SDLoc &DL,
+                                   SelectionDAG &DAG) const;
+
+    SDValue PerformDAGCombine(SDNode *N, DAGCombinerInfo &DCI) const override;
+
     bool ShouldShrinkFPConstant(EVT VT) const override {
       // Do not shrink FP constpool if VT == MVT::f128.
       // (ldd, call _Q_fdtoq) is more expensive than two ldds.
       return VT != MVT::f128;
     }
 
     bool shouldInsertFencesForAtomic(const Instruction *I) const override {
       // FIXME: We insert fences for each atomics and generate

llvm/trunk/lib/Target/Sparc/SparcISelLowering.cpp

if (VA.needsCustom()) {
               DAG.getStore(Chain, dl, Arg, PtrOff, MachinePointerInfo()));
           continue;
         }
       }
 
       if (VA.getLocVT() == MVT::f64) {
         // Move from the float value from float registers into the
         // integer registers.
-
-        // TODO: The f64 -> v2i32 conversion is super-inefficient for
-        // constants: it sticks them in the constant pool, then loads
-        // to a fp register, then stores to temp memory, then loads to
-        // integer registers.
+        if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Arg))
+          Arg = bitcastConstantFPToInt(C, dl, DAG);
+        else
           Arg = DAG.getNode(ISD::BITCAST, dl, MVT::v2i32, Arg);
       }
 
       SDValue Part0 = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, MVT::i32,
                                   Arg,
                                   DAG.getConstant(0, dl, getVectorIdxTy(DAG.getDataLayout())));
       SDValue Part1 = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, MVT::i32,
                                   Arg,
                                   DAG.getConstant(1, dl, getVectorIdxTy(DAG.getDataLayout())));
if (Subtarget->fixAllFDIVSQRT()) {
     setOperationAction(ISD::FDIV, MVT::f32, Promote);
     setOperationAction(ISD::FSQRT, MVT::f32, Promote);
   }
 
   if (Subtarget->hasNoFMULS()) {
     setOperationAction(ISD::FMUL, MVT::f32, Promote);
   }
 
+  // Custom combine bitcast between f64 and v2i32
+  if (!Subtarget->is64Bit())
+    setTargetDAGCombine(ISD::BITCAST);
+
   setOperationAction(ISD::INTRINSIC_WO_CHAIN, MVT::Other, Custom);
 
   setMinFunctionAlignment(2);
 
   computeRegisterProperties(Subtarget->getRegisterInfo());
 }
 
 bool SparcTargetLowering::useSoftFloat() const {
LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   case ISD::UMULO:
   case ISD::SMULO:              return LowerUMULO_SMULO(Op, DAG, *this);
   case ISD::ATOMIC_LOAD:
   case ISD::ATOMIC_STORE:       return LowerATOMIC_LOAD_STORE(Op, DAG);
   case ISD::INTRINSIC_WO_CHAIN: return LowerINTRINSIC_WO_CHAIN(Op, DAG);
   }
 }
 
+SDValue SparcTargetLowering::bitcastConstantFPToInt(ConstantFPSDNode *C,
+                                                    const SDLoc &DL,
+                                                    SelectionDAG &DAG) const {
+  APInt V = C->getValueAPF().bitcastToAPInt();
+  SDValue Lo = DAG.getConstant(V.zextOrTrunc(32), DL, MVT::i32);
+  SDValue Hi = DAG.getConstant(V.lshr(32).zextOrTrunc(32), DL, MVT::i32);
+  if (DAG.getDataLayout().isLittleEndian())
+    std::swap(Lo, Hi);
+  return DAG.getBuildVector(MVT::v2i32, DL, {Hi, Lo});
+}
+
+SDValue SparcTargetLowering::PerformBITCASTCombine(SDNode *N,
+                                                   DAGCombinerInfo &DCI) const {
+  SDLoc dl(N);
+  SDValue Src = N->getOperand(0);
+
+  if (isa<ConstantFPSDNode>(Src) && N->getSimpleValueType(0) == MVT::v2i32 &&
+      Src.getSimpleValueType() == MVT::f64)
+    return bitcastConstantFPToInt(cast<ConstantFPSDNode>(Src), dl, DCI.DAG);
+
+  return SDValue();
+}
+
+SDValue SparcTargetLowering::PerformDAGCombine(SDNode *N,
+                                               DAGCombinerInfo &DCI) const {
+  switch (N->getOpcode()) {
+  default:
+    break;
+  case ISD::BITCAST:
+    return PerformBITCASTCombine(N, DCI);
+  }
+  return SDValue();
+}
+
 MachineBasicBlock *
 SparcTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
                                                  MachineBasicBlock *BB) const {
   switch (MI.getOpcode()) {
   default: llvm_unreachable("Unknown SELECT_CC!");
   case SP::SELECT_CC_Int_ICC:
   case SP::SELECT_CC_FP_ICC:
   case SP::SELECT_CC_DFP_ICC:

llvm/trunk/test/CodeGen/SPARC/float-constants.ll

 ; RUN: llc < %s -march=sparc | FileCheck %s
-
-; TODO: actually fix the codegen to be optimal. At least we don't
-; crash for now, though...
+; RUN: llc < %s -march=sparcel | FileCheck %s --check-prefix=CHECK-LE
 
 ;; Bitcast should not do a runtime conversion, but rather emit a
 ;; constant into integer registers directly.
 
 ; CHECK-LABEL: bitcast:
-; TODO-CHECK: sethi 1049856, %o0
-; TODO-CHECK: sethi 0, %o1
+; CHECK: sethi 1049856, %o0
+; CHECK: mov %g0, %o1
+; CHECK-LE: mov %g0, %o0
+; CHECK-LE: sethi 1049856, %o1
 define <2 x i32> @bitcast() {
   %1 = bitcast double 5.0 to <2 x i32>
   ret <2 x i32> %1
 }
 
 ;; Same thing for a call using a double (which gets passed in integer
 ;; registers)
 
 ; CHECK-LABEL: test_call
-; TODO-CHECK: sethi 1049856, %o0
-; TODO-CHECK: sethi 0, %o1
+; CHECK: sethi 1049856, %o0
+; CHECK: mov %g0, %o1
+; CHECK-LE: mov %g0, %o0
+; CHECK-LE: sethi 1049856, %o1
 declare void @a(double)
 define void @test_call() {
   call void @a(double 5.0)
   ret void
 }
 
 ;; And for a libcall emitted from the pow intrinsic.  (libcall
 ;; emission happens after SelectionDAG type legalization, so is a bit
 ;; different than a normal function call. This was crashing before,
 ;; due to an earlier broken workaround for this issue.)
 
 ; CHECK-LABEL: test_intrins_call
-; TODO-CHECK: sethi 1049856, %o0
-; TODO-CHECK: sethi 0, %o1
+; CHECK: sethi 1048576, %o0
+; CHECK: mov %g0, %o1
+; CHECK: mov %o0, %o2
+; CHECK: mov %o1, %o3
+; CHECK-LE: mov %g0, %o0
+; CHECK-LE: sethi 1048576, %o1
 declare double @llvm.pow.f64(double, double)
 define double @test_intrins_call() {
   %1 = call double @llvm.pow.f64(double 2.0, double 2.0)
   ret double %1
 }