[PATCH] D94211: [NFC][PowerPC] Format and clean the logic for setOperationActions to make it easier to maintain

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added a comment.

Maybe better to add some comments before each section? Like D78132 <https://reviews.llvm.org/D78132> or X86ISelLowering.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:162
-  // Match BITREVERSE to customized fast code sequence in the td file.
-  setOperationAction(ISD::BITREVERSE, MVT::i32, Legal);
-  setOperationAction(ISD::BITREVERSE, MVT::i64, Legal);
----------------
These are not guarded by any predicate, why move them?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:302
 
-  // If we're enabling GP optimizations, use hardware square root
-  if (!Subtarget.hasFSQRT() &&
-      !(TM.Options.UnsafeFPMath && Subtarget.hasFRSQRTE() &&
-        Subtarget.hasFRE()))
-    setOperationAction(ISD::FSQRT, MVT::f64, Expand);
-
-  if (!Subtarget.hasFSQRT() &&
-      !(TM.Options.UnsafeFPMath && Subtarget.hasFRSQRTES() &&
-        Subtarget.hasFRES()))
-    setOperationAction(ISD::FSQRT, MVT::f32, Expand);
-
-  if (Subtarget.hasFCPSGN()) {
-    setOperationAction(ISD::FCOPYSIGN, MVT::f64, Legal);
-    setOperationAction(ISD::FCOPYSIGN, MVT::f32, Legal);
-  } else {
-    setOperationAction(ISD::FCOPYSIGN, MVT::f64, Expand);
-    setOperationAction(ISD::FCOPYSIGN, MVT::f32, Expand);
-  }
+  setOperationAction(ISD::INTRINSIC_VOID, MVT::i8,    Custom);
+  setOperationAction(ISD::INTRINSIC_VOID, MVT::i16,   Custom);
----------------
Better to move them down around `init_trampoline` since here others are related to float?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:343
+    setOperationAction(ISD::UINT_TO_FP,        MVT::i32, Custom);
+    setOperationAction(ISD::STRICT_FP_TO_SINT, MVT::i32, Custom);
+    setOperationAction(ISD::STRICT_FP_TO_UINT, MVT::i32, Custom);
----------------
So, this patch does 'overrides' like:

```
// Before
if (A) { setLegal(X); } else { setCustom(X); }

// After
setCustom(X); if (A) { setLegal(X); }
```

Looks correct. But is this good practice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94211