[PATCH] D50823: [VPlan] Introduce VPCmpInst sub-class in the instruction-level representation

[Robin Kruppe via Phabricator via llvm-commits]

rkruppe added a comment.

The implementation looks good to me. The interface chosen here (directly mirroring `CmpInst` in the `VPValue` hierarchy) also seems like the right direction to me. Besides avoiding the problematic concept of "underlying `Instruction`s" altogether, it also gives a convenient place to put any helper functionality that the vectorizer code might want when generating and manipulating such comparisons.

That said, this is currently the only subclass of its kind and it would be weird if it remained that way. In other words, I would expect that `VPInstruction` would gain more such subclasses (e.g., `VPBinaryOperator`, `VPSelectInst`, etc.) as the vectorizer starts to create and manipulate these instructions. Does this make sense to y'all?



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:157
+    assert(CI && "CI can't be null!");
+    assert(LeftOp && RightOp && "VPCmpInst's operands can't be null!");
+    VPCmpInst *VPCI = createCmpInst(LeftOp, RightOp, CI->getPredicate());
----------------
This duplicates the assertion in the other overload that's called in the next line, right? I don't mind that, it's good to be a bit more defensive, just want to make sure I didn't miss anything and that this is intentional.


Repository:
  rL LLVM

https://reviews.llvm.org/D50823