[PATCH] D158992: [VPlan] Manage compare predicates in VPRecipeWithIRFlags.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for following up on this! Adding various nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:124
   }
 
+  VPValue *createICmp(CmpInst::Predicate Pred, VPValue *A, VPValue *B,
----------------
Add documentation? TODO add createFCmp() when needed?

Perhaps better place below, close to createNot/And/Or/Select, after the more general createNaryOp().
Have it `returned createInstruction()` like them, with a new createInstruction() that calls the new VPInstruction  constructor?

BTW, the createNaryOp() with Inst seems a bit odd?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:817
   enum class OperationType : unsigned char {
+    Cmp,
     OverflowingBinOp,
----------------
nit: place after FPMathOp, to be consistent with the union order below?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:859
     FastMathFlagsTy FMFs;
     unsigned char AllFlags;
+    CmpInst::Predicate CmpPredicate;
----------------
nit: keep AllFlags last? An unsigned char still suffices, right?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:877
+      CmpPredicate = Op->getPredicate();
+    } else if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
       OpType = OperationType::OverflowingBinOp;
----------------
nit: place below after handling FPMathOperator, for consistency.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:120
+    case Instruction::ICmp:
+    case Instruction::FCmp:
     case VPInstruction::Not:
----------------
Is FCmp case dead, or checked by any test?

OTOH, would be ideal to generalize by checking if Instruction may have side effects for all opcodes up to Instruction::OtherOpsEnd, but w/o having an Instruction instance...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:631
+    O << " " << CmpInst::getPredicateName(getPredicate());
+    break;
   case OperationType::PossiblyExactOp:
----------------
nit: place below for consistency.


================
Comment at: llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll:909
 ; CHECK-NEXT:     REPLICATE ir<%l2> = trunc ir<%l1>
-; CHECK-NEXT:     REPLICATE ir<%cmp> = icmp ir<%l1>, ir<0>
+; CHECK-NEXT:     REPLICATE ir<%cmp> = icmp eq ir<%l1>, ir<0>
 ; CHECK-NEXT:     REPLICATE ir<%sel> = select ir<%cmp>, ir<5>, ir<%l2>
----------------
nice catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158992