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

[Qiu Chaofan via Phabricator via cfe-commits]

qiucf added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9899
+def err_ppc_unsupported_argument_type : Error<
+  "unsupported argument type %0 for target %1">;
 def err_x86_builtin_invalid_rounding : Error<
----------------
Can we re-use `err_typecheck_convert_incompatible`?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16290-16310
+  case PPC::BI__builtin_ppc_maxfe:
+  case PPC::BI__builtin_ppc_maxfl:
+  case PPC::BI__builtin_ppc_maxfs:
+  case PPC::BI__builtin_ppc_minfe:
+  case PPC::BI__builtin_ppc_minfl:
+  case PPC::BI__builtin_ppc_minfs: {
+    if (BuiltinID == PPC::BI__builtin_ppc_maxfe)
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:3913
+  case PPC::BI__builtin_ppc_minfs: {
+    // FIXME: remove below check once -mlong-double-128 is supported on AIX.
+    if (Context.getTargetInfo().getTriple().isOSAIX() &&
----------------
I think we don't need this fixme.


================
Comment at: clang/test/CodeGen/PowerPC/builtins-ppc.c:65
+  // CHECK: call double (double, double, double, ...) @llvm.ppc.maxfl(double %0,
+  // double %1, double %2, double %3)
+  res = __builtin_ppc_maxfl(a, b, c, d);
----------------
Don't break CHECK lines.


================
Comment at: clang/test/Sema/builtins-ppc.c:13-17
+// RUN: %clang_cc1 -triple powerpc64le-unknown-unknown -DTEST_MAXMIN -fsyntax-only \
+// RUN: -verify %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -DTEST_MAXMINFE_AIX -fsyntax-only \
+// RUN: -verify %s
----------------



================
Comment at: clang/test/Sema/builtins-ppc.c:63-83
+#ifdef TEST_MAXMIN
+void test_maxmin() {
+  long double fe;
+  double fl;
+  float fs;
+  __builtin_ppc_maxfe(fl, fl, fl, fl); // expected-error-re {{requires argument of {{.*}} type (passed in {{.*}})}}
+  __builtin_ppc_minfe(fl, fl, fl, fl); // expected-error-re {{requires argument of {{.*}} type (passed in {{.*}})}}
----------------
I don't know if it's convention in tests, but looks simpler.


================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:192
+                  [llvm_float_ty, llvm_float_ty, llvm_float_ty, llvm_vararg_ty],
+                  [IntrNoMem]>;
 }
----------------
Will we support `llvm_f128_ty`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10583-10587
+    for (unsigned i = 4, e = Op.getNumOperands(); i < e; ++i) {
+      if (Op.getOperand(i).getValueType() != Op.getValueType())
+        report_fatal_error("Intrinsic::ppc_[max|min]f[e|l|s] must have uniform "
+                           "type arguments");
+    }
----------------
We can make it even simpler.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10594
+    // Below selection order follows XLC behavior: start from the last but one
+    // operand, move towards the first operand, end with the last operand.
+    unsigned I, Cnt;
----------------
We don't need to mention old behavior.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10595-10596
+    // operand, move towards the first operand, end with the last operand.
+    unsigned I, Cnt;
+    I = Cnt = Op.getNumOperands() - 2;
+    SDValue Res = Op.getOperand(I);
----------------



================
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);
+    }
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:11266
+    case Intrinsic::ppc_maxfe:
+    case Intrinsic::ppc_minfe:
     case Intrinsic::ppc_fnmsub:
----------------
Why only two `fe`?


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-maxmin.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mcpu=pwr9 -mtriple=powerpc64le-unknown-linux < %s | FileCheck %s
+
----------------
Can we add `pwr8` run line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122478