[PATCH] D50480: [LV] Vectorizing loops of arbitrary trip count without remainder under opt for size

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

> Reverted to use the original ICmpULE extended opcode instead of detached ICmpInst. This can be revised quite easily once VPInstructions acquire any other form of modeling compares.

Since the VPCmpInst code is ready (https://reviews.llvm.org/D50823) and this is a clear use case where we need to model a new compare (including its predicate) that is not in the input IR, I'd appreciate if we could discuss a bit more about using the VPCmpInst approach. At least, I'd like to understand what are the concerns about the VPCmpInst approach and what other people think.

I do have concerns regarding modeling ICmpULE as an opcode only for compare instructions newly created during a VPlan-to-VPlan transformation. For example:

1. Inconsistent modeling of compare instructions in the VPlan native path. Compare instructions in the input IR will be modeled as VPInstructions with a `Instruction::ICmpInst`/`Instruction::FCmpInst` opcode. New compare instructions will be modeled as VPInstructions with predicates as opcodes (`VPInstruction::ICmpULE`, for now). We'd have to compare the opcode against `Instruction::ICmpInst`, `Instruction::ICmpInst`, `VPInstruction::ICmpULE` and any future predicate opcode to know that a VPInstruction is a comparison. Similar inconsistency to get information about the compare predicate.

2. Adding ICmpULE as an opcode is paving the way to adding more predicates as opcodes in VPInstruction in the short term. Where would the limit be? Do we want to model the around 30 predicates currently in LLVM CmpInst as opcodes?

3. The ICmpULE approach may also be detrimental for the Instruction/VPInstruction templatization that we planned to explore.

If these points and the fact that VPCmpInst code is ready to go don't convince you, there isn't much else I can do :). I know this compare representation may sound insignificant but I'm well aware of how painful things can turn when things are built on top of "insignificant decisions" that have to be changed later on. If the problem with VPCmpInst is to rebase this patch on top of https://reviews.llvm.org/D50823, I'm perfectly fine with introducing https://reviews.llvm.org/D50823 after this patch goes in. However, if there are any other concerns regarding the VPCmpInst sub-class, it would be better to know them now. I'd prefer not to keep the ICmpULE opcode representation for a long time.

Thanks,
Diego


https://reviews.llvm.org/D50480