[PATCH] D61961: [PowerPC] Cust lower fpext v2f32 to v2f64 from extract_subvector v4f32

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

A few minor nits and a functional problem that needs to be addressed (exiting if the input is not a `v4f32`).



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9632
+  case ISD::EXTRACT_SUBVECTOR: {
+    assert((Op0.getNumOperands()==2 || isa<ConstantSDNode>(Op0->getOperand(1)))
+     && "Node should have 2 operands with second one being a constant!");
----------------
Nit: spaces around the `==`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9635
+
+    // Custom lower is only done for high or low word.
+    int Idx = cast<ConstantSDNode>(Op0.getOperand(1))->getZExtValue();
----------------
I think you mean doubleword here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9637
+    int Idx = cast<ConstantSDNode>(Op0.getOperand(1))->getZExtValue();
+    if (Idx % 2 != 0) return SDValue();
+    int Word = Idx ? 1:0;
----------------
Nit: the `return SDValue()` should be on the line under the `if`.
We also need a check here that the input to the `EXTRACT_SUBVECTOR` is actually of type `v4f32`, otherwise this doesn't work.
If you add a test case much like the one you added here, but change all instances of
`<4 x float>` to `<16 x float>`. This will not do the right thing.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9638
+    if (Idx % 2 != 0) return SDValue();
+    int Word = Idx ? 1:0;
+
----------------
Nit: spaces around `:`. Also, this is really just `Idx >> 1` isn't it?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9641
+    // High and low word positions are different on little endian.
+    if (Subtarget.isLittleEndian()) Word = !Word;
+
----------------
Same nit about the code being on the line under the `if`. And also, not word, but doubleword.


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

https://reviews.llvm.org/D61961