[PATCH] D49219: [Sparc] Custom bitcast between f64 and v2i32

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: lib/Target/Sparc/SparcISelLowering.cpp:3060
 
+SDValue SparcTargetLowering::LowerBITCAST(SDValue Op, SelectionDAG &DAG) const {
+  SDLoc dl(Op);
----------------
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.


================
Comment at: lib/Target/Sparc/SparcISelLowering.cpp:3070
+  if (LoadSDNode *LD = dyn_cast<LoadSDNode>(Src))
+    return DAG.getLoad(VT, dl, LD->getChain(), LD->getBasePtr(),
+                       LD->getMemOperand());
----------------
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.



================
Comment at: lib/Target/Sparc/SparcISelLowering.cpp:3073
+
+  if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Src)) {
+    APInt V = C->getValueAPF().bitcastToAPInt();
----------------
Is it guaranteed here that VT is only v2i32? Maybe just add that to the if.


================
Comment at: lib/Target/Sparc/SparcISelLowering.cpp:3075
+    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);
----------------
This code works right in both endiannesses, right? (I forgot how build-vector endianness goes).


================
Comment at: test/CodeGen/SPARC/float-constants.ll:32-33
 ; CHECK-LABEL: test_intrins_call
-; TODO-CHECK: sethi 1049856, %o0
-; TODO-CHECK: sethi 0, %o1
+; CHECK: sethi %hi(.LCPI2_0), %i0
+; CHECK: ldd [%i0+%lo(.LCPI2_0)], %o0
 declare double @llvm.pow.f64(double, double)
----------------
Leave a note that there is still non-optimal codegen in this case?


https://reviews.llvm.org/D49219