Recusiverly lower instructions #655
Conversation
| if (auto *BinOp = dyn_cast<BinaryOperator>(V)) { | ||
| if (auto *EquivalentType = getEquivalentType(BinOp->getType())) { | ||
| IRBuilder<> B(BinOp); | ||
| if (isa<Argument>(V)) { |
There was a problem hiding this comment.
InstructionMap is looking like a bit of a misnomer now. Maybe ValueMap or RemappedValues?
There was a problem hiding this comment.
You're right, I'll rename it.
FWIW, it only contains Instruction and Constant objects and there's already a check for this in LongVectorLoweringPass::cleanDeadInstructions. Yet, it could be extended to include other kind of objects in the future.
lib/LongVectorLoweringPass.cpp
Outdated
| return V; | ||
| } | ||
|
|
||
| bool LongVectorLoweringPass::handlingRequired(Instruction &I) { |
There was a problem hiding this comment.
This might be better served taking a User as the parameter for reuse with constants. At some point I assume the lowering will have to break down aggregates that contain long vectors (e.g. arrays and structs) and I think this will be useful at that point.
There was a problem hiding this comment.
That's a good idea, I'll make this change.
| return UndefValue::get(EquivalentTy); | ||
| } | ||
|
|
||
| if (auto *Vector = dyn_cast<ConstantDataVector>(&Cst)) { |
There was a problem hiding this comment.
I think you are missing constant null vectors. They should be handled now.
Separately, I assume it is left to future changes to handle constant expressions?
There was a problem hiding this comment.
I think you are missing constant null vectors. They should be handled now.
Good catch, I'll add support for this with the relevant tests.
Separately, I assume it is left to future changes to handle constant expressions?
Yes, but more generally the prototype doesn't include support for all features. The check below catches such cases, and features can be easily added once the core features are in place.
There are features I don't cover (mainly because we don't have a clean use-case to test them against at the moment) and that you rightfully point out during these reviews. I don't want to dismiss those comments as they are obviously valuable, yet I can't implement them as we go or that would delay the core functionality too much. How do you propose we keep track of missing features? Should I add in the description of #613 a list of known missing features we discover while peer-reviewing this work? Or should we track this in a follow-up issue?
There was a problem hiding this comment.
I'm happy to track missing functionality in #613. To be clear, I'm totally ok with this coming together in small steps. These questions are more to help to me scope out the extent of a given PR.
There was a problem hiding this comment.
FYI, I've updated the description in #613.
| @@ -426,7 +634,8 @@ bool LongVectorLoweringPass::runOnFunction(Function &F) { | |||
|
|
|||
| bool Modified = (FunctionToVisit != &F); | |||
| for (Instruction &I : instructions(FunctionToVisit)) { | |||
| Modified |= (visit(&I) != nullptr); | |||
| // Use the Value overload of visit to ensure cache is used. | |||
| Modified |= (visit(static_cast<Value *>(&I)) != nullptr); | |||
There was a problem hiding this comment.
Does LLVM's cast not suffice here? If it does it is preferable to static_cast.
There was a problem hiding this comment.
While it would likely work, llvm::cast is usually not a no-op as it checks at runtime for types. Here, we know at compile time I is an Instruction and therefore a Value, so we can leverage the language type system to do a no-op cast at compile time -- or get a compile-time error if the underlying assumption was incorrect.
| return UndefValue::get(EquivalentTy); | ||
| } | ||
|
|
||
| if (auto *Vector = dyn_cast<ConstantDataVector>(&Cst)) { |
There was a problem hiding this comment.
FYI, I've updated the description in #613.
Improve the design by relying on InstVisitor to lower instructions. This is a first step to ensure the pass will remain modular when adding support for additional instructions. Signed-off-by: Marco Antognini <marco.antognini@arm.com>
Lower InsertElementInst, ExtractElementInst, ShuffleVectorInst, UndefValue, ConstantAggregateZero, ConstantDataVector and pointer types. With support for these instructions, it is now possible to use recursion to implement support for BinaryOperator with the existing tests. Strengthen checks in getEquivalentTypeImpl. Now that this function is recursive it cannot be const anymore. Enable the disabled llvm_unreachable statements to catch bugs now that enough functionalities are implemented. Add test to cover the newly supported constants and instructions. Signed-off-by: Marco Antognini <marco.antognini@arm.com>
|
Ha, yes. Should be fixed now. |
The first commit is almost a NFC. It refactors the code to use InstVisitor to make the pass more modular. In future commits, support for a new instruction will be added with a new method overriding InstVisitor's default behaviour. The only change is that the pass will no longer print "Value not handled" (or any other message) for instruction that do not need handling, as determined by
handlingRequired.The second commit recursively visits instructions and lowers their operands. It adds support for a few additional kind of instructions and constants, with tests covering these, that, together with the existing support for BinaryOperator, allow turning on assertion/llvm_unreachable to alert users about features not yet supported.