- User Since
- Jan 23 2017, 8:11 PM (77 w, 2 d)
Mon, Jul 9
Sun, Jul 8
Thu, Jul 5
I've reverted the original problematic change by rL336410. I think we need to collect tests that degrade from it to make this change properly.
I failed to construct pure SCEV test easily, will try unit test if needed.
Wed, Jul 4
The context is following: the patch rL336172 has broken this test, and I was asked to fix it. Basically I just restored the behavior that we used to have before this sinking. I would prefer to not introduce any new transforms to InstCombine without dire need because it is fragile, and I am not familiar with it well enough (in particular, I don't understand ordering of particular transforms in many places). So I've chosen a conservative approach (to restore old behavior).
Mon, Jul 2
Rebased on top of pre-merged test, fixed comments.
Sun, Jul 1
Added some comments, removed one unnecessary lambda.
Fri, Jun 29
Thu, Jun 28
LGTM once a TODO comment is added.
Frankly, I don't understand why we want it to be an instruction at all. Can these methods (i.e. hoistIVInc and isNormalAddRecExprPHI) be taught to work with values? For example, if it is not an instruction then we just consider it already hoisted.
Wed, Jun 27
Moved fill to private API, added lazy calculations, overall it is now more user-friendly.
Tue, Jun 26
Rebased. We need to reanimate this.
Sinking it to the end of method did not introduce any new changes. Also added a comment.
Mon, Jun 25
Wed, Jun 20
Tue, Jun 19
Jun 18 2018
Jun 14 2018
Thanks for fixing that!
Jun 13 2018
Jun 12 2018
Jun 8 2018
Not sure how to construct a test that will fail an assert, however the general idea is that, for example, a block that goes to method exit from the innermost look is also an exiting block for all its containing loops, up to the topmost parent. It means that this exiting block may be considered for iter count calculation for all those loops. From this point of view, there is no practical difference between the immediate parent and any other top loop.
Jun 7 2018
Jun 6 2018
Jun 5 2018
Jun 4 2018
I will do it as a follow-up then.
I'm not sure if it gives benefits, but the idea is interesting. Do you mind if it goes as a follow-up?
Jun 3 2018
Jun 1 2018
May 31 2018
May 23 2018
May 21 2018
May 19 2018
May 14 2018
May 6 2018
LGTM once comments are addressed.
May 4 2018
May 3 2018
Never mind, in this case the latch of outer loop changes, so we must clear cache.
Actually particularly with case of unswitching, I grow hesitant about one of our asserts that fails here. Maybe it is over-conservative. See comment https://reviews.llvm.org/D44676#1086144
May 2 2018
This one appeared to be a real bug, at least we have a test that fails with assert for trivial unswitching. Thanks @uabelho for providing the test case.
I am not sure that we actually break the invariant which was preserved before. Note that all recent failures we have seen were on assert from the patch D44676. We might as well just break this invariant previously, but without this assert we did not know about it.