[PATCH] D103986: [PowerPC] Floating Point Builtins for XL Compat.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-fp.c:5
+// RUN: %clang_cc1 -triple powerpc64le-unknown-unknown \
+// RUN:   -emit-llvm %s -o - -target-cpu pwr8 | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-unknown-aix \
----------------
AFAICT, nothing changes with Power8 so you might as well just have run lines for Power7 (AIX) and Power8 (LE).


================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:1529
+                     Intrinsic<[llvm_double_ty], [llvm_double_ty, 
+                     llvm_double_ty, llvm_double_ty], [IntrNoMem]>;
+  def int_ppc_fsels : GCCBuiltin<"__builtin_ppc_fsels">,
----------------
amyk wrote:
> Minor indentation nit.
I don't think this indentation is desired. This makes it look like the first type on line 1529 belongs in the first list rather than the second list it belongs to. I suggest the following:
```
def int_ppc_fsel :
  GCCBuiltin<"__builtin_ppc_fsel">,
  Intrinsic<[llvm_double_ty], [llvm_double_ty,
                               llvm_double_ty, llvm_double_ty], [IntrNoMem]>;
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4990
   case ISD::INTRINSIC_WO_CHAIN: {
+
+    if (N->getConstantOperandVal(0) == Intrinsic::ppc_fsels) {
----------------
NeHuang wrote:
> you can delete blank line and better add comments for the operation below.
I agree that this warrants a comment. However I don't think this is really what Victor had in mind. In general if your comment boils down to "this is what we are doing" and simply describes the code, it is not very useful. What your comment should address is "why are we doing this here and this way".

To me as a reader, it is not at all clear why we manually select this to `PPC::FSELS` here. All the other intrinsics are matched with patterns in the `.td` file but this one is matched specifically here. Why?


================
Comment at: llvm/test/CodeGen/builtins-ppc-xlcompat-fp.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-unknown \
+; RUN:   -mcpu=pwr7 < %s | FileCheck %s --check-prefix=CHECK-PWR7
----------------
Again, this is excessive. There should be run lines for Power8, Power8-32-bit, Power8-no-vsx, Power7. We don't really need a cross-product of triples and CPUs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103986