[LICM]: support hosting ref_element_addr even if they are not guaranteed to be executed #18931
Conversation
|
@atrick , @eeckstein can either one review this small change please? :) |
|
@swift-ci Please test |
| LLVM_DEBUG(llvm::dbgs() << "Hoisted BeginAccess " << *BI); | ||
| for (auto *instSink : Ends) { | ||
| if (!sinkInstruction(DT, LoopSummary, instSink, LI)) { | ||
| llvm_unreachable("LICM: Could not perform must-sink instruction"); |
There was a problem hiding this comment.
That looks pretty scary. Can you prove that whenever hoistInstruction succeeds also sinkInstruction will succeed?
The logic in both functions is very different. Just a simple example: what about an infinite loop? Wouldn't hoisting succeed but sinking not?
Is there a way to bail if on of hoisting/sinking fails? Just to be on the safe side?
There was a problem hiding this comment.
There's no way to bail, but, all the appropriate checks where done when analyzing the begin access instruction. as long as the loop is contains at least one exiting block this should succeed. sinkInstruction can only fail in the non-special-instructions case. just re-using the code.
There was a problem hiding this comment.
ok, as long as you are sure about it :-)
| LLVM_DEBUG(llvm::errs() << " Successfully hosited and sank pair\n"); | ||
| } else { | ||
| auto *REA = dyn_cast<RefElementAddrInst>(Inst); | ||
| assert(REA && "Only RefElementAddr and BeginAccess are supported"); |
There was a problem hiding this comment.
You can replace dyn_cast<> + assert with a static cast<>
| @@ -615,6 +619,13 @@ void LoopTreeOptimization::analyzeCurrentLoop( | |||
| checkSideEffects(Inst, MayWrites); | |||
| break; | |||
| } | |||
| case SILInstructionKind::RefElementAddrInst: { | |||
| auto *REA = dyn_cast<RefElementAddrInst>(&Inst); | |||
| assert(REA && "ref_element_addr"); | |||
| auto *REA = dyn_cast<RefElementAddrInst>(&Inst); | ||
| assert(REA && "ref_element_addr"); | ||
| SpecialHoist.insert(REA); | ||
| checkSideEffects(Inst, MayWrites); |
There was a problem hiding this comment.
That's not needed. ref_element_addr cannot have any side effects
…eed to be executed In some instances, some instructions, like ref_element_addr, can be hoisted outside of loops even if they are not guaranteed to be executed. We currently don’t support that / bail. We only try to do so / do further analysis only for begin_access because they are extremely heavy. However, we need to support hosting of ref_element_addr in that case, if it does not have a loop dependent operand, in order to be able to hoist begin_access instructions in some benchmarks. Initial local testing shows that this PR, when we enable exclusivity, improves the performance of a certain internal benchmark by over 40% See rdar://problem/43623829
|
@swift-ci Please test and merge |
In some instances, some instructions, like ref_element_addr, can be hoisted outside of loops even if they are not guaranteed to be executed.
We currently don’t support that / bail. We only try to do so / do further analysis only for begin_access because they are extremely heavy.
However, we need to support hosting of ref_element_addr in that case, if it does not have a loop dependent operand, in order to be able to hoist begin_access instructions in some benchmarks.
Initial local testing shows that this PR, when we enable exclusivity, improves the performance of a certain internal benchmark by over 40%
See rdar://problem/43623829