- User Since
- Jan 23 2017, 8:11 PM (208 w, 1 d)
Added test for combination of these options.
This is awesome. And scary. This is scarily awesome. :)
Wed, Jan 13
Tue, Jan 12
Dec 14 2020
Dec 8 2020
Dec 7 2020
Dec 4 2020
Please reduce the test or give an elaborate explanation of the problem. So far it's not clear to me what was the problem and why it fixed this way.
Dec 3 2020
That could go without review. LGTM.
Dec 1 2020
Nov 29 2020
Nov 27 2020
I added tests for signed and for ne predicate, but currently they fail even before they reach this code. Can be enabled in follow-ups.
Nov 26 2020
Previous version was better, but still not good enough. Tried another version that removes two instances of isKnownViaNonRecuriveReasoning from the path.
Maybe it's because isKnownPredicateAt makes an extra call to isKnownPredicate which we don't really need. Let's try more lightweight version with isBasicBlockEntryGuardedByCond. This should not introduce extra overhead.
Already present in isBasicBlockEntryGuardedByCond.
Strange. I'll revert it for now and investigate.
Rebased, fixed '.wide name.
Nov 25 2020
My understanding of this situation is that, whenever we delete an instruction that may be referenced by SCEV, we should call forgetValue for it. SCEV that contains references on deleted instructions is invalid. Faling to forget deleted values is a bug. I don't understand why llvm.dbg.value is somehow special about it. This patch tries to hide the consequences of the bug instead of fixing it.
Actually, it only means that isLoopBackedgeGuardedByCond should directly call isKnownPredicateAt(Latch->getTerminator()) as the last resort. I will follow-up with this improvement adding relevant test.
Eeek. I found a test where isKnownPredicateAt is more powerful. Need to use both then.
Do we know at which point LSR deletes the code? To me it looks like we just forgot to call forgetLoop or smth like this somewhere.
Nov 24 2020
Yes, I ran 4077 fuzz tests checking this assumption, and it seems better in every case there. There can theoretically be cases where isLoopBackedgeGuardedBy is weaker, but it's only the reason to improve it.
Please move the test out of MIPS-specific into general test directory if it's possible. Otherwise, LGTM.
The idea of having SCEV pointing to deleted instructions scares me. Imagine what if we had SCEVAddRecs referencing the loops we've already deleted. There is a vast field for nasty bugs caused by UB and memory corruption. I'd rather expect that we fail some assertion if we try to optimize with SCEV in that state.
The change looks very reasonable, though I don't know what motivation was behind the current ordering. There might have been some (arcane) reasons behind it. Fine by me, but please have someone else to take a look.
This was completely illegal. Need to find a way to revert it now.
Nov 23 2020
Roll back to older code concept (without generatization into DomTree). It seems unreasonably difficult to check in code there.
Let's go with 3. The idea was to share this code if someone might need it. I'm totally fine if it's written every time in different places whenever someone needs this logic.
Fixed check (as expected, dominates fails for two Phis from the same block).
Added expensive asserts;
I see the deep API problem here now. Current dominates(Value *, Instruction *) should not be called like that (at least because it returns false for dominates(X, X)). It has a different semantics, and should be called something like "canBeUserOf".
Nov 22 2020
You can find the planned user in dependencies stack: https://reviews.llvm.org/D90456