PartialExecuter: Dominators optimization (draft)#357
Conversation
This is a draft PR to mainly confirm if what I implemented is along the right lines, or if I misunderstood the idea. Right now it seems that this optimization is actually slower than what is currently in master, so if I did not misunderstood the idea, it would not be good to merge this.
andmadri
force-pushed
the
PartialExecuter_Optimizations
branch
from
a873e14 to
7bc3a88
Compare
|
I removed the recursion, we now check the dominator once when starting a block and use the bit to track the state. I was thinking of using the flag for BBProgress as we set it to true in the same cases but for now I did not wanted to mix them and still not quite sure about this. It makes sense not to use an extra map/set but I am still unsure how to do it without it. An alternative is looping over the instructions of a dominator and checking if the instruction is in the knownValues map and it is not marked as everskipped. This would be of course even slower. We have these progress flags for the basic blocks and SCC that indicate somehow the state but I don’t think there is a way without a map to save these states and query them later for the dominator. I am looping one extra time over the operands as before. The iteration happens originally when we check whether an operand is computed. I thought it would be logical to put the check there rather than when we check the knownValues map. This is because we only check the knownValues map when we register a value for an instruction, and we only do so after we confirm that all strong bits are computed. |
alexp-sssup
left a comment
alexp-sssup
left a comment
There was a problem hiding this comment.
I think we are going in the right direction. Is this still worse from the performance standpoint?
| } | ||
| for (auto& op : I.operands()) | ||
| { | ||
| if (!hasKnownValues && (!isa<llvm::Constant>(op) && !isa<llvm::Argument>(op) && !isa<BasicBlock>(op))) |
There was a problem hiding this comment.
You can check for isa directly here
| } | ||
| } | ||
|
|
||
| llvm::BasicBlock* PartialInterpreter::visitBasicBlock(FunctionData& data, BasicBlockGroupNode& BBGN, llvm::BasicBlock& BB, bool& BBProgress) |
There was a problem hiding this comment.
Avoid this confusing code movement
| : F(F), moduleData(moduleData), currentEE(nullptr), functionInstructionCounter(0) | ||
| { | ||
| if (!F.isDeclaration()) | ||
| DT.recalculate(F); |
There was a problem hiding this comment.
I think there might be a better way by declaring to LLVM that this pass depends on DT and DT will make the analysis available. @yuri91 does this make sense?
| executionContext.CurBB = &BB; | ||
| executionContext.CurInst = executionContext.CurBB->begin(); | ||
|
|
||
| bool hasKnownValues = checkDominator(data, &BB); |
There was a problem hiding this comment.
I see that the are some similarities to the BBProgress flag, but it's only superficial I think. Knowing an instruction is progress, but not all types of progress imply knowing an instruction
2 participants
This is a draft PR to mainly confirm if what I implemented is along the right lines, or if I misunderstood the idea.
Right now it seems that this optimization is actually slower than what is currently in master, so if I did not misunderstood the idea, it would not be good to merge this.