[PATCH] D49507: [Power9] Add __float128 support in the backend for bitcast to a i128

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

I think the changes that are needed are clear enough that this doesn't require another review cycle. Approving this with the assumption that the required change will be made on the commit.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14325
+    if (ConstNode && ConstNode->getZExtValue() == 64) {
+      // Flip the constant bit.
+      EltToExtract = EltToExtract ? 0 : 1;
----------------
This seems kind of misleading - makes the reader wonder what this "constant bit" is. I think something like `// Switch the element number to extract.` is a lot more descriptive.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14329
+      Op0 = Op0.getOperand(0);
+    }
+  }
----------------
There needs to be an `else` here that will `return SDValue()` since it is not OK to keep going if we have an `SRL` with a non-constant shift amount or a shift amount other than 64.
However, rather than an `else` I think it would be nicer to flip the condition and exit early...
```
// The right shift has to be by 64 bits.
if (!ConstNode || ConstNode->getZextValue() != 64)
  return SDValue();
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14336
+      Op0.getOperand(0).getValueType() == MVT::f128) {
+    // The correct bitcast is present.
+    SDValue Bitcast = DCI.DAG.getBitcast(MVT::v2i64, Op0.getOperand(0));
----------------
The comment is redundant - basically just says that we're inside the if block.


https://reviews.llvm.org/D49507