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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

> Reordered code to have Cmp at front everywhere.

Fine, let's have it at front everywhere.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:49
+  /// Insert \p VPI in BB at InsertPt if BB is set.
+  VPInstruction *insertInstruction(VPInstruction *VPI) {
+    if (BB)
----------------
nit: tryInsertInstruction?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:904
 
+  template <typename IterT>
+  VPRecipeWithIRFlags(const unsigned char SC, IterT Operands,
----------------
nit: place this first as well


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:993
 
+  CmpInst::Predicate getPredicate() const {
+    assert(OpType == OperationType::Cmp &&
----------------
nit: let Cmp appear first consistently


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1071
 
+  VPInstruction(unsigned Opcode, CmpInst::Predicate Pred, VPValue *A,
+                VPValue *B, DebugLoc DL = {}, const Twine &Name = "");
----------------
nit: ditto


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:258
 
+VPInstruction::VPInstruction(unsigned Opcode, CmpInst::Predicate Pred,
+                             VPValue *A, VPValue *B, DebugLoc DL,
----------------
nit: ditto


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:283
   }
-  case VPInstruction::ICmpULE: {
+  case Instruction::FCmp:
+  case Instruction::ICmp: {
----------------
nit: FCmp is currently dead?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:120
+    case Instruction::ICmp:
+    case Instruction::FCmp:
     case VPInstruction::Not:
----------------
fhahn wrote:
> 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.
Perhaps worth leaving behind a TODO.
Perhaps native may find this useful already?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158992