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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14192
+SDValue PPCTargetLowering::combineTRUNCATE(SDNode *N,
+                                            DAGCombinerInfo &DCI) const {
+  // If we are using CRBits then try that first.
----------------
Nit: indentation is off.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14195
+  if (Subtarget.useCRBits()) {
+    SDValue CRTruncValue = DAGCombineTruncBoolExt(N, DCI);
+
----------------
Perhaps for brevity and readability, combine this into the if:
```
if (SDValue CRTruncValue = DAGCombineTruncBoolExt(N, DCI))
  return CRTruncValue;
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14203
+  SDLoc dl(N);
+  const SDValue & Op0 = N->getOperand(0);
+  if (Op0.getValueType() == MVT::i128 &&
----------------
Nit: the more common way to declare references puts the ampersand immediately before the variable name.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:14209
+    // BITCAST feeding a TRUNCATE
+    if (Op0.getNode()->getOpcode() == ISD::BITCAST &&
+        Op0.getNode()->getOperand(0).getValueType() == MVT::f128) {
----------------
Why not just `Op0.getOpcode()` and `Op0.getOperand(0)`? Here as well as below.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:237
+                             SDTypeProfile<1, 2,
+                               [SDTCisInt<0>, SDTCisFP<1>, SDTCisPtrTy<2>]>,
+                             []>;
----------------
I don't think it is OK to allow arbitrary integer types for the result nor arbitrary floating point types for operand 1. This really seems like it should use `SDTCisVT`. I realize that `PPCbuild_fp128` has the same issue and we missed it, but I think we should favour fixing that one (in a separate patch) rather than making this one match.


================
Comment at: test/CodeGen/PowerPC/f128-bitcast.ll:1
+; RUN: llc -mcpu=pwr9 -mtriple=powerpc64le-unknown-unknown \
+; RUN:   -enable-ppc-quad-precision -verify-machineinstrs \
----------------
I think once you add the BE portion to this patch, we should have another RUN to test that.


https://reviews.llvm.org/D49507