[PATCH] D54583: PowerPC: Optimize SPE double parameter calling setup

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:393
     setOperationAction(ISD::BITCAST, MVT::f64, Expand);
+    if (Subtarget.hasSPE()) {
+      setOperationAction(ISD::BITCAST, MVT::i64, Custom);
----------------
No need for braces when there is a single statement in the if/else.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7833
+
+  if (!Subtarget.hasSPE())
+    return SDValue();
----------------
The early exit should be first.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7837
+  if (!(Op.getValueType() == MVT::i32 &&
+      Op0.getOpcode() == ISD::BITCAST))
+    return SDValue();
----------------
The indentation is off. Maybe run `clang-format` on this function. I don't know which editor you use but if you use `Vim`, you can run `:7827,7845 ! clang-format` if you have `clang-format` in your `$PATH`.
Also, it seems like you might want to check the input type as well - maybe with an assert if it can't be anything other than `f64`.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7840
+
+  assert(Op0.getNumOperands() > 0 && "WTF?");
+  if (Op->getConstantOperandVal(1) == 0)
----------------
A more descriptive assert message is probably in order.
Also, perhaps it would be clearer if this was rewritten to:
- Assert that the constant operand value is less than 2
- Use a ternary operator to select the opcode (or just have one opcode - see above)


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7842
+  if (Op->getConstantOperandVal(1) == 0)
+    return DAG.getNode(PPCISD::EXTRACT_SPE_LO,  dl, MVT::i32, Op0.getOperand(0));
+
----------------
Line too long?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.h:203
+      /// Extract high SPE register component
+      EXTRACT_SPE_HI,
+
----------------
Why not just have `EXTRACT_SPE_OP` and have it take a constant operand that determines Hi/Lo? Also, for both build and extract, it would be good to add a comment that these correspond almost exactly to `BUILD_PAIR` and `EXTRACT_VECTOR_ELT` nodes except that the input types are floating point since `i64` isn't a legal type for the target.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54583/new/

https://reviews.llvm.org/D54583