[PATCH] D122478: [PowerPC] Add max/min intrinsics to Clang and PPC backend

[Ting Wang via Phabricator via cfe-commits]

tingwang added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:192
+                  [llvm_float_ty, llvm_float_ty, llvm_float_ty, llvm_vararg_ty],
+                  [IntrNoMem]>;
 }
----------------
qiucf wrote:
> Will we support `llvm_f128_ty`?
I'm afraid not at this moment. Document mentions only three types: float, double, or long double.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10598-10602
+    for (--I; Cnt != 0; --Cnt, I = (--I == 0 ? (Op.getNumOperands() - 1) : I)) {
+      Res = LowerSELECT_CC(
+          DAG.getSelectCC(dl, Res, Op.getOperand(I), Res, Op.getOperand(I), CC),
+          DAG);
+    }
----------------
qiucf wrote:
> I don't think we need to manually call `LowerSELECT_CC` here. SelectionDAG knows `ppc_fp128` should not be custom lowered.
> 
> This also makes the case pass. Thus D122462 is not needed.
Thank you for pointing out this. Verified LowerSELECT_CC is not required here. This was added due to misconception.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11266
+    case Intrinsic::ppc_maxfe:
+    case Intrinsic::ppc_minfe:
     case Intrinsic::ppc_fnmsub:
----------------
qiucf wrote:
> Why only two `fe`?
This is only for ppcf128 during type legalization: DAGTypeLegalizer::ExpandFloatResult -> CustomLowerNode ->  PPCTargetLowering::ReplaceNodeResults. The other cases seem not hitting here. I will double check the code path to verify.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122478