[VPlan] Assign custom opcodes to recipes not mapping to IR opcodes. #162267
Conversation
We can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe (llvm#162110). To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes.
| | ||
| auto C = getOpcodeOrIntrinsicID(Def); |
There was a problem hiding this comment.
| auto C = getOpcodeOrIntrinsicID(Def); | |
| auto C = getOpcodeOrIntrinsicID(Def); | |
There was a problem hiding this comment.
Should be adjusted, thanks
| @llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesWe can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes. Full diff: https://github.com/llvm/llvm-project/pull/162267.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h index fb696bea671af..8ca3bedfaa259 100644 --- a/llvm/lib/Transforms/Vectorize/VPlan.h +++ b/llvm/lib/Transforms/Vectorize/VPlan.h @@ -1064,6 +1064,7 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags, ResumeForEpilogue, /// Returns the value for vscale. VScale, + OpsEnd = VScale, }; /// Returns true if this VPInstruction generates scalar values for all lanes. diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp index c8a2d84a535d3..8d0870b69121f 100644 --- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp +++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp @@ -1982,6 +1982,13 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> { .Case<VPWidenIntrinsicRecipe>([](auto *I) { return std::make_pair(true, I->getVectorIntrinsicID()); }) + .Case<VPVectorPointerRecipe>([](auto *I) { + // For recipes that do not directly map to LLVM IR instructions, + // assign opcodes after the last VPInstruction opcode (which is also + // after the last IR Instruction opcode), based on the VPDefID. + return std::make_pair(false, + VPInstruction::OpsEnd + 1 + I->getVPDefID()); + }) .Default([](auto *) { return std::nullopt; }); } @@ -2005,11 +2012,8 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> { static bool canHandle(const VPSingleDefRecipe *Def) { // We can extend the list of handled recipes in the future, // provided we account for the data embedded in them while checking for - // equality or hashing. We assign VPVectorEndPointerRecipe the GEP opcode, - // as it is essentially a GEP with different semantics. - auto C = isa<VPVectorPointerRecipe>(Def) - ? std::make_pair(false, Instruction::GetElementPtr) - : getOpcodeOrIntrinsicID(Def); + // equality or hashing. + auto C = getOpcodeOrIntrinsicID(Def); // The issue with (Insert|Extract)Value is that the index of the // insert/extract is not a proper operand in LLVM IR, and hence also not in @@ -2048,6 +2052,8 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> { vputils::isSingleScalar(L) != vputils::isSingleScalar(R) || !equal(L->operands(), R->operands())) return false; + assert(getOpcodeOrIntrinsicID(L) && getOpcodeOrIntrinsicID(R) && + "must have valid opcode info for both recipes"); if (auto *LFlags = dyn_cast<VPRecipeWithIRFlags>(L)) if (LFlags->hasPredicate() && LFlags->getPredicate() != |
| @llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesWe can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes. Full diff: https://github.com/llvm/llvm-project/pull/162267.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h index fb696bea671af..8ca3bedfaa259 100644 --- a/llvm/lib/Transforms/Vectorize/VPlan.h +++ b/llvm/lib/Transforms/Vectorize/VPlan.h @@ -1064,6 +1064,7 @@ class LLVM_ABI_FOR_TEST VPInstruction : public VPRecipeWithIRFlags, ResumeForEpilogue, /// Returns the value for vscale. VScale, + OpsEnd = VScale, }; /// Returns true if this VPInstruction generates scalar values for all lanes. diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp index c8a2d84a535d3..8d0870b69121f 100644 --- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp +++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp @@ -1982,6 +1982,13 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> { .Case<VPWidenIntrinsicRecipe>([](auto *I) { return std::make_pair(true, I->getVectorIntrinsicID()); }) + .Case<VPVectorPointerRecipe>([](auto *I) { + // For recipes that do not directly map to LLVM IR instructions, + // assign opcodes after the last VPInstruction opcode (which is also + // after the last IR Instruction opcode), based on the VPDefID. + return std::make_pair(false, + VPInstruction::OpsEnd + 1 + I->getVPDefID()); + }) .Default([](auto *) { return std::nullopt; }); } @@ -2005,11 +2012,8 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> { static bool canHandle(const VPSingleDefRecipe *Def) { // We can extend the list of handled recipes in the future, // provided we account for the data embedded in them while checking for - // equality or hashing. We assign VPVectorEndPointerRecipe the GEP opcode, - // as it is essentially a GEP with different semantics. - auto C = isa<VPVectorPointerRecipe>(Def) - ? std::make_pair(false, Instruction::GetElementPtr) - : getOpcodeOrIntrinsicID(Def); + // equality or hashing. + auto C = getOpcodeOrIntrinsicID(Def); // The issue with (Insert|Extract)Value is that the index of the // insert/extract is not a proper operand in LLVM IR, and hence also not in @@ -2048,6 +2052,8 @@ struct VPCSEDenseMapInfo : public DenseMapInfo<VPSingleDefRecipe *> { vputils::isSingleScalar(L) != vputils::isSingleScalar(R) || !equal(L->operands(), R->operands())) return false; + assert(getOpcodeOrIntrinsicID(L) && getOpcodeOrIntrinsicID(R) && + "must have valid opcode info for both recipes"); if (auto *LFlags = dyn_cast<VPRecipeWithIRFlags>(L)) if (LFlags->hasPredicate() && LFlags->getPredicate() != |
…lvm#162267) We can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe (llvm#162110). To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes. PR: llvm#162267
… opcodes. (#162267) We can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe (llvm/llvm-project#162110). To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes. PR: llvm/llvm-project#162267
| static bool canHandle(const VPSingleDefRecipe *Def) { | ||
| // We can extend the list of handled recipes in the future, | ||
| // provided we account for the data embedded in them while checking for | ||
| // equality or hashing. We assign VPVectorEndPointerRecipe the GEP opcode, |
There was a problem hiding this comment.
So this potentially changes behavior rather than purely NFC, as VPVectorEndPointerRecipe and GEP are now distinct?
There was a problem hiding this comment.
We check the subclass ID in addition to the opcode while performing CSE, so this change would be non-functional.
There was a problem hiding this comment.
Is this checking of subclass ID redundant now, and can be replaced by an assert that the subclass ID's are the same if their opcodes are?
There was a problem hiding this comment.
It is still needed now, as this prevents replacing for example VPWidenRecipe BinOps with VPReplicateRecipe BinOps. This would be taken care of if we include VPDefID for all opcodes, but we would need some kind of total order for VPDefIDs and their start and end opcodes, for all recipes.
There was a problem hiding this comment.
Understood. Making getOpcode/OrIntrinsicID unique across recipes could also be taken care of by consolidating conflicting recipes, so that information about widening vs. replication (as in the example) is encoded elsewhere, e.g., in the type of the operands, if not in the opcode itself.
| // assign opcodes after the last VPInstruction opcode (which is also | ||
| // after the last IR Instruction opcode), based on the VPDefID. | ||
| return std::make_pair(false, | ||
| VPInstruction::OpsEnd + 1 + I->getVPDefID()); |
There was a problem hiding this comment.
Alternatively, an opcode can be added for VPVectorPointerRecipe, similar to various VPInstructions. OTOH, this offers a systematic way of providing opcodes to all recipes, aiming to also support VPPredInstPHI. Having a universal opcode across all recipes would require addressing multiple recipes with potentially common underlying opcodes.
There was a problem hiding this comment.
I initially added the GEP opcode to VPVectorPointerRecipe, but @fhahn said that it other users could potentially conflate a VectorPointer with a plain GEP, causing confusion. I think adding opcodes to the remaining recipes as OpsEnd + 1 + getVPDefID() could be interesting.
…lvm#162267) We can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe (llvm#162110). To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes. PR: llvm#162267
4 participants
We can perform CSE on recipes that do not directly map to Instruction opcodes. One example is VPVectorPointerRecipe. Currently this is handled by supporting them in ::canHandle, but currently that means that we return std::nullopt from getOpcodeOrIntrinsicID() for it. This currently only works, because the only case we return std::nullopt and perform CSE is VPVectorPointerRecipe. But that does not work if we support more such recipes, like VPPredInstPHIRecipe
(#162110).
To fix this, return a custom opcode from getOpcodeOrIntrinsicID for recipes like VPVectorPointerRecipe, using the VPDefID after all regular instruction opcodes.