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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:124
   }
 
+  VPValue *createICmp(CmpInst::Predicate Pred, VPValue *A, VPValue *B,
----------------
Ayal wrote:
> 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?
Added TODO and documentation and moved.

Using `createInstruction` would require adding a new variant of that function I think. Maybe it would be better to just add a helper to insert the created instruction.

> BTW, the createNaryOp() with Inst seems a bit odd?
 Used for VPlan-native path I think. Should be able to remove it once we add an in-place IR recipe.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:854
 
   union {
     WrapFlagsTy WrapFlags;
----------------
tschuett wrote:
> No std::variant?
Hmm, can be changed in the future. It looks like there are very few uses of std::variant in the `llvm` subdirectory so it doesn't seem used a lot.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:859
     FastMathFlagsTy FMFs;
     unsigned char AllFlags;
+    CmpInst::Predicate CmpPredicate;
----------------
Ayal wrote:
> nit: keep AllFlags last? An unsigned char still suffices, right?
No, updated the `unsigned` thanks! (Although all predicates should fit into a char...)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:877
+      CmpPredicate = Op->getPredicate();
+    } else if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
       OpType = OperationType::OverflowingBinOp;
----------------
Ayal wrote:
> nit: place below after handling FPMathOperator, for consistency.
Updated to have `Cmp` at top of the enum and union.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1077
   VPInstruction *clone() const {
     SmallVector<VPValue *, 2> Operands(operands());
     return new VPInstruction(Opcode, Operands, DL, Name);
----------------
Ayal wrote:
> Also have clone() replicate the IR flags? (Independent of this patch, calls for testing?)
Seemed unused, will remove separately.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:120
+    case Instruction::ICmp:
+    case Instruction::FCmp:
     case VPInstruction::Not:
----------------
Ayal wrote:
> 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...
Dead code for now, removed!


> 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...

Agreed, that would require refactoring the corresponding implementation in `Instruction` to just work on opcodes.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:631
+    O << " " << CmpInst::getPredicateName(getPredicate());
+    break;
   case OperationType::PossiblyExactOp:
----------------
Ayal wrote:
> nit: place below for consistency.
Reordered code to have Cmp at front everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158992