- User Since
- Oct 23 2013, 6:32 PM (278 w, 2 d)
Thu, Feb 21
Wed, Feb 20
D58490 is posted for review.
I'm fine with this landing, since the patch is an obvious improvement. However, I have to admit I don't understand the interaction between LCSSA and SCEV here. Why does SCEV need to care that a phi might be a LCSSA phi? They seem orthogonal. Maybe *some particular caller* might need to care, but that seems like a different issue?
I was planning on sending a note after it lands. Happy to do so now if folks things it's warranted.
Does the existing code successfully hoist the widenable_condition call itself out of the loop? If not, splitting out specifically that part of the transform would be well called for and necessary for unswitch to make progress. If it does, then the patch as written includes redundant handling which should be removed.
JFYI, I disagree w/the sentiment expressed that we shouldn't do obvious memory optimizations in InstCombine. InstCombine does local peephole optimizations *including memory optimizations*. Even if I accept the stated goal that it *shouldn't*, today it *does* and there's no reason to reject this patch.
Tue, Feb 19
Fri, Feb 15
This looks to be a form of unswitching. Can you explain why this needs to be done in LICM rather than in unswitching?
Thu, Feb 14
Address review comments
Wed, Feb 13
Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.
No longer WIP, ready for real review.
Tue, Feb 12
Mon, Feb 11
LGTM w/two required follow ups. Can be before or after landing.
Fri, Feb 8
Wed, Feb 6
Tue, Feb 5
Mon, Feb 4
Marking just remove from review queue since discussion appears to have stalled.
Sorry for just getting around to reviewing this now. Should be a bit more response now.