- User Since
- Jan 23 2017, 8:11 PM (167 w, 19 h)
Sun, Mar 29
Fri, Mar 27
Wed, Mar 18
As a suggestion: since this helper doesn't create new code, it doesn't seem to be important whether it is FIFO or LIFO. If that is so, I'd suggest using SmallVector and LIFO algorithm instead. It should have better memory footprint. OK if it does as follow-up.
Fri, Mar 13
Looks good. Sorry for long delay.
Thu, Mar 12
Updated the patch in accordance with comments.
@lebedev.ri I am not sure what your logic implied here; maybe instead of removal of assert this check should be a part of if for profitability reasons.
Feb 24 2020
LGTM. Hope it doesn't end up exploding. :)
Feb 18 2020
Not sure what are cost implications of that, but fine.
Feb 10 2020
Ok, thanks for clarification. Let them shoot their legs then. :) LGTM
Ok, with assert it makes sense. LGTM
Feb 4 2020
I'm not sure about how we should interpret lack of TTI. Maybe we should interpret it like "if there is no model, we don't care about the high costs, and everything is low cost enough", or opposite (like in your patch). Neither of this looks NFC to me (given that we can pass null TTI). Maybe we should merge it *after* we make use of TTI inside the methods?
There is no possible situation when it matters, but fine. Making this check first sounds reasonable. LGTM.
Feb 3 2020
LGTM (pls make sure that clang-format is fixed).
Jan 30 2020
What was the motivation to make it a function pass? Conceptually it's a loop pass.
Do you have a simple test demonstrating a claimed speedup?
I don't understand how come that getOrCreateAddExpr does not do the job instead of your code. Can you please explain what is missing there that is present in your change?
I am a bit worried about that. As far as I remember RightValue needs to be in preheader because it is used in checks that decide whether or not we should go to pre/postloops. Does your case generate them? If yes, please add checks to make it explicit. If not, please construct a similar test with pre/postloop and make sure it passes with the fix.
Jan 28 2020
In the commit message:
I see to possibilities to mitigate that.
to -> two
Just nits from me, I don't know this part well enough to approve.
Jan 23 2020
LGTM with nit (see inline).
Oct 7 2019
Feb 21 2019
Feb 20 2019
Feb 19 2019
Ok. Then I suggest to hold it off unless we see a case where unswitch doesn't do its job and this does.
The main motivation is unification - I don't want to leave a single thing that is supported for intrinsic guards and not supported for control flow guards. Unswitching has known problems related to exponential code size growth, so I don't want our ability to eliminate guards in loops to depend on its cost model.