[PATCH] D141473: [PowerPC] Simplify fp-to-int store optimization

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added a comment.

In D141473#4260035 <https://reviews.llvm.org/D141473#4260035>, @shchenz wrote:

> If this patch can optimize the redundant `xscvdpsxws` away for case: https://godbolt.org/z/zWsaae4j4 ?

Yes, one less `xscvdpsxws`.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8075
+  MVT DestTy = Op.getSimpleValueType();
+  if ((DestTy == MVT::i8 || DestTy == MVT::i16) && Subtarget.hasP9Vector())
+    DestTy = MVT::i32;
----------------
shchenz wrote:
> This change is not enough I think. You extend this function to be used before type legalizer and make this function depend on the caller that will not pass other illegal types except `i8` and `i16`. How about simple types like `i2` and `i4`?
> 
> We need a guard for the type in this function.
> 
`combineStoreFPToInt` already rejected illegal types. And codegen for `i2` `i4` are good:

```
xscvdpsxws 0, 1
mffprwz	3, 0
clrlwi	3, 3, 28
stb 3, 0(4)
stb 3, 0(5)
stb 3, 0(6)
blr
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8089
   }
+  EVT ConvTy = Src.getValueType() == MVT::f128 ? MVT::f128 : MVT::f64;
+  SDValue Conv;
----------------
shchenz wrote:
> If we plan to support type `MVT::f128`, we may need to change the PPCISD node names to generic ones. These names are 1:1 map to PPC instructions and these instructions assume the types are f64.
Thanks for reminding about this. I think that deserves another patch. I propose renaming them into `CONV_FP_(I|U)(32|64)`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14878
-                    dl, ResVT == MVT::f128 ? MVT::f128 : MVT::f64, Val);
-  DCI.AddToWorklist(Val.getNode());
 
----------------
shchenz wrote:
> We may need to following this logic and consider about adding the users back to the work list.
Is that necessary? DAG combiner will add the users into worklist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141473