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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
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);
----------------
qiucf wrote:
> These are not guarded by any predicate, why move them?
We have other ATOMIC op later, I move them together. For BITREVERSE, I move them together with BITCAST. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:330
 
-  // PowerPC does not have ROTR
-  setOperationAction(ISD::ROTR, MVT::i32   , Expand);
-  setOperationAction(ISD::ROTR, MVT::i64   , Expand);
+  setOperationAction(ISD::STRICT_FP_ROUND,   MVT::f32, Legal);
 
----------------
Do we miss the case for setOperationAction(ISD::STRICT_FP_ROUND,   MVT::f64, Legal); ?


================
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);
----------------
qiucf wrote:
> 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?
For now, usually we have:
```
if (A) {
   if (B) {
     setLegal(X);
   } else {
      ...
   }
}
```
I am trying to flat them and move the same operation together to make it more clear. Regarding to your example, I have no good answer and that's something we can discuss. Personally, I think it makes sense as it means, by default, it is custom, if feature A enabled, it is legal. And we will have case like :
```
if (A) { setLegal(X); } else { setCustom(X); }
if (B) { setExpand(X);} else { setCustom(X); }
```
Now it is:
```
setCustom(X)
if (A)
   setLegal(X)
if (B)
   setExpand(X);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94211