- User Since
- Oct 23 2013, 6:32 PM (294 w, 6 d)
Mon, Jun 17
So, just to say this back to you, this would be treating an addrspacecast more like a bitcast when the source is known to be an alloca?
On hold given previous review comments.
No longer actively pursuing. The idea is reasonable, and only partly overlaps with LFTR, but all of the non-reduced motivating cases are covered by multiple exit LFTR.
No opinion GC wise, but mostly because I don't understand the intent of this patch. What is the desired outcome here?
Thu, Jun 13
Wed, Jun 12
Wow, we have a huge amount of boilerplate when adding an attribute.
Absolutely. That's exactly what I'm asking for.
rebase on requested tests
Sorry for the late reply. Replying selectively to key points.
LGTM w/a caveat. I fully expect that landing this would expose lots of existing breakage and would break the build bots. In practice, I don't think you should land this until fixing any issues you've already found. Once that's done, land, but expect to revert and fix found issues for a few cycles.
LGTM w/two required changes.
LGTM w/two required changes. (If you want to just make the changes, you can submit without further review. )
Incorporate Nikita's suggestion about IVs which are already post-increment, and just need their formed changed.
Tue, Jun 11
A suggestion on how to make progress here. Introduce the wrap flags to the API, but have *all* callers pass AnyWrap. That must be NFC.
Rebase on landed prep patches. Ready for actual review, though won't land until after https://reviews.llvm.org/D62939.
Actually, think 'S' being poisoned is almost fully handled or maybe even fully handled. If 'S' is poison, then IV must be poison on at least the first iteration. Given that, if we find a side effect which must execute on the path to our exit block, then we've proven we must have executed UB. As such, the program is undefined and we have no further obligations.
Planning a change in direction after offline discussion w/Artur. Instead of framing this as hoisting/widening, I'm going to reframe as a separate loop transform which lifts a trivial guard (by splitting the condition) into the loop preheader where possible. We think this will be easier to test, and may have positive effects beyond guard widening,
Mon, Jun 10
Rebase over tweaked tests and address comments from Nikita.
First iteration on a combined patch. The tests need updated to reflect the poison/undef split. I'll do that, then rebase on top.
The issues raised in this and D62939 turned out to be inseparable. Given the meaningful review discussion happened on the other review, I'm abandoning this patch.
Minor rebase. As pointed out by previous comments both from myself and Nikita, the entire approach here is broken. This doesn't actually appear to be separable from D62936. Given all the meaningful review has happened here, I'm going to repurpose this review for the bug fix and close that one.
LGTM for the AtomicExpand parts. I'm willing to assume you know what you're doing for the AMDGPU bit. :)
Rebase on top of newly added tests.
Also LGTM. Nice cleanup.
Thu, Jun 6
Remove the buggy phi code Nikita pointed out and otherwise address last round of comments from Nikita.
Remove the two cases Nikita noted.
Blocked on discussion in D62939 and landing thereof.
Flags on SCEVs are valid for the current set of users that exist in the loop. If you add *new* users then those flags may no longer be valid. We recently ran into this in LFTR, and this patch seems likely to have the same risk. I have not audited the patch or its output to be sure, just throwing out a possibility.
Revise comments per Nikita's suggestions and some further word smithing of my own.
Address a few of Nikita's comment. I ended up heavily revising the structure, so the line locations got a bit confusing.
Cleanup code and fix a couple of bugs in the process.
Wed, Jun 5
Tue, Jun 4
The NFC portion of this turned out to be a bit more involved than expected. It's been separated into D62880. Once that lands, I'll rebase for the small remaining functional bit. I'm debating just going to all loop exits instead of just the latch exit. On further reflection, I'm not sure it's really going to matter impact wise, and fleshing out problems faster might be worthwhile.