[PATCH] D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8144
 
+static SDValue getMayStrictNode(unsigned Opc, const SDLoc &dl, EVT VT, SDValue Op,
+                                SDValue Chain, SelectionDAG &DAG, bool Strict) {
----------------
The parameter SDLoc is not needed. And can we change the function like: getFpNode() or something else ?  You don't need to have the strict in the function name as it is already one of the function parameter.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8148
+    return DAG.getNode(Opc, dl, VT, Op);
+  assert((!Strict || Chain) && "Missing chain for creating strict nodes");
+
----------------
!Strict is not needed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8150
+
+  unsigned NewOpc;
+  switch (Opc) {
----------------
Please give the default initializer to avoid uninitialized local variable if llvm_unreachable is off.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8172
+  // For strict nodes, source is the second operand.
+  SDValue Src = Op.getOperand((int)Strict);
+  SDValue FPChain;
----------------
This is not good code practice. Please use: Strict ? 1 : 0 or seeking some API in the SDValue.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8196
     break;
   }
 
----------------
The logic between LowerFP_TO_INTForReuse and LowerFP_TO_INTDirectMove is nearly the same between line 8168 ~ 8196. And that is expected as the difference between the two is how to move the data from FPR to GPR.  So, can we add another function to do the convert ? Something like:
```
LowerFP_TO_INTDirectMove:
  V = convertToFp()
  MFVSR V
```

```
LowerFP_TO_INTForReuse:
 V = convertToFp()
 Store V
 Load V
```



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8278
   // FP to INT conversions are legal for f128.
-  if (EnableQuadPrecision && (Op->getOperand(0).getValueType() == MVT::f128))
+  if (EnableQuadPrecision && (Src.getValueType() == MVT::f128))
     return Op;
----------------
This could be something that we can improve later. We should mark it as legal instead of checking it here if I understand the intention correctly.  


================
Comment at: llvm/test/CodeGen/PowerPC/fp-strict-conv.ll:8
+; RUN:   < %s -mtriple=powerpc64le-unknown-linux -mcpu=pwr8 -mattr=-vsx | \
+; RUN:   FileCheck %s -check-prefix=NOVSX
+
----------------
Add run for SPE target.


================
Comment at: llvm/test/CodeGen/PowerPC/fp-strict-conv.ll:10
+
+declare i32 @llvm.experimental.constrained.fptosi.i32.f64(double, metadata)
+declare i64 @llvm.experimental.constrained.fptosi.i64.f64(double, metadata)
----------------
A a test for fp128


================
Comment at: llvm/test/CodeGen/PowerPC/fp-strict-conv.ll:171
+
+attributes #0 = { strictfp }
----------------
Does this attribute need ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81537