User Details
- User Since
- Oct 23 2013, 6:32 PM (492 w, 2 d)
Yesterday
This is all good news, thanks!
@dmgreen I'd really appreciate any input you can share on the performance swing you observed.
I've posted a review with a second attempt here: https://reviews.llvm.org/D147336. This contains the functional fix, but not yet anything on the performance side. Let's continue the discussion on the perf issue there.
Thu, Mar 30
Use wording suggested by @asb
Wed, Mar 29
Two levels of comments here.
LGTM
Tue, Mar 28
Mon, Mar 27
LGTM
LGTM once Craig's style comment addressed.
Fri, Mar 24
LGTM w/comment
LGTM
Looking at some code which benefits from this change, I noticed an opportunity for a follow up.
LGTM w/minor comments.
Thu, Mar 23
LGTM - once all the earlier patches have been approved.
LGTM - though can you split this into two patches when you land? The SLP bit deserves a slightly different description focus.
LGTM
I think you're going one step too far with this patch. Not in the sense that this patch isn't justified - only in the sense you should do a simpler case first.
Wed, Mar 22
Address review comment
Address review comment.
p.s. The previous case might be interesting to explore later. It should involve a stride store, but separating that into it's own patch series makes complete sense.
LGTM
Tue, Mar 21
Ok, at this point all the known soundness problems are fixed in tree. I was simply fixing ones obvious on inspection, and have not done any testing of this mechanism beyond the LIT tests themselves.
I landed a fix for this in 00fdd2cb6c55744ae21f18c377854e9359273454. This patch can be abandoned.
LGTM
So, you'd be correct here, except that the IV in question is nsw, and we branch on the result. For N=0, the starting value of the post-inc IV for the loop is -1 and we subtract 1 on every iteration. To reach zero, we have to pass through the negative overflow case, and thus have poison and (from the branch) UB. So, we can discount this case when reasoning about the expansion.
lsr-term-fold is still wildly unsound. I have patches out to address this, but we should wait until that is done discussing enabling it (by default or by target).
Mon, Mar 20
Another simplification.
Fri, Mar 17
LGTM
LGTM
LGTM