[PATCH] D109437: [PowerPC] FP compare and test XL compat builtins.

[Lei Huang via Phabricator via llvm-commits]

lei requested changes to this revision.
lei added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/include/clang/Sema/Sema.h:12680
   bool CheckPPCMMAType(QualType Type, SourceLocation TypeLoc);
+  bool CheckPPCTestDataClassType(CallExpr *TheCall);
 
----------------
need to remove.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16039-16040
+  case PPC::BI__builtin_ppc_test_data_class:
+    Value *ArgValue = EmitScalarExpr(E->getArg(0));
+    llvm::Type *ArgType = ArgValue->getType();
+    unsigned Int;
----------------
`ArgValue` is only used one so not needed. 
```
    llvm::Type *ArgType = EmitScalarExpr(E->getArg(0))->getType();
```



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16041
+    llvm::Type *ArgType = ArgValue->getType();
+    unsigned Int;
+    if (ArgType->isDoubleTy()) {
----------------
variables should be discriptive of what they represent.  This is no diff then a single char variable 🙂
```
unsigned IntrinsicID;
```


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16042
+    unsigned Int;
+    if (ArgType->isDoubleTy()) {
+      Int = Intrinsic::ppc_test_data_class_d;
----------------
braces here are redundant.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:16050
+    Function *F = CGM.getIntrinsic(Int);
+    return Builder.CreateCall(F, Ops, "test_data_class");
   }
----------------
Try to refrain from def one-time use variables.
```
return Builder.CreateCall(CGM.getIntrinsic(Int), Ops, "test_data_class");
```


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3492
+      Diag(TheCall->getBeginLoc(), diag::err_ppc_invalid_test_data_class_type);
+      ArgTypeIsInvalid = true;
+    }
----------------
I'm not sure this is needed... can't we just `return true` here since this is a `S` error?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10382-10383
+    switch (IntrinsicID) {
+    default:
+      llvm_unreachable("Unknown Intrinsic");
+    case Intrinsic::ppc_compare_exp_lt:
----------------
I dont' think this is needed since you will only be here if he IntrinsicID matches the lines listed prior to this block.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10397-10398
+    }
+    SDValue Op1 = Op.getOperand(1);
+    SDValue Op2 = Op.getOperand(2);
+    SDValue Ops[]{
----------------
one time used variables can be removed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10408-10417
+    switch (IntrinsicID) {
+    default:
+      llvm_unreachable("Unknown Intrinsic");
+    case Intrinsic::ppc_test_data_class_d:
+      CmprOpc = PPC::XSTSTDCDP;
+      break;
+    case Intrinsic::ppc_test_data_class_f:
----------------
this can just be an if/else since you won't be in this block unless the IntrisicID are `ppc_test_data_class_[d|f]`
```
unsigned CmprOpc = PPC::XSTSTDCDP;
if (IntrinsicID == Intrinsic::ppc_test_data_class_f)
  CmprOpc = PPC::XSTSTDCSP;
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10419
+    SDValue Op1 = Op.getOperand(2);
+    SDValue Op2 = Op.getOperand(1);
+    SDValue Ops[]{
----------------
one-time use variable.  Can be merged into the call below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109437