- User Since
- Jan 23 2017, 8:11 PM (260 w, 2 d)
Tue, Jan 18
Sun, Jan 9
Wed, Dec 22
Dec 19 2021
Please rewrite commit message here to make it clear what's going on.
Dec 3 2021
Because incomplete SCCs draw a lot of questions and suspicions, despite I think everything should still work correctly for them, these cases are unimportant for any practical purpose.
Dec 2 2021
Actually the definition of "almost addrec" doesn't even cover the motivation for this patch, where we have 2 phis from same block that just swap values. I don't think we can have any good framing here.
Thanks for the suggesting, I need to wright a prototype to see how it works and if it covers all I need.
Nov 28 2021
Do you need any approval for that? :)
Ok, thanks for clarification.
One more point is that the name wouldInstructionBeTriviallyDeadOnPathsWithoutUse is not entirely clear. Does true here mean that at least one path without uses exists, or it is something that we need to check separately?
I think most of this logic really belongs to isGuaranteedToTransferExecutionToSuccessor. I also think that wouldInstructionBeTriviallyDeadOnPathsWithoutUse should simply be isGuaranteedToTransferExecutionToSuccessor && !mayHaveSideEffects (maybe with filtering out terminators, ehpads and stuff like this), unless you have a counter-test.
Nov 23 2021
Nov 21 2021
Can we replace it with assert for the first time, and then delete it when we make sure it never fails?
Nov 10 2021
I'd rather move the assert inside of applyLoopGuards. If it breaks, it's a subject for improving our reasoning. I can imagine it could be breaking because of issues with caching (e.g. if addrecs are involved), though. So, unless the failures are mass and disruptive, I'd prefer it to stay and catch us opportunities.
Nov 9 2021
Ok, I don't care enough.
Nov 8 2021
This is strange. From what I'm seeing, your IV starts from 1 and is just trivially positive, so there should not be any troubles with using zext. Could you please post a separate review with the unsigned version & its generated checks for this test?
Nov 7 2021
I don't understand the old code. Why does it look at one user? I thought the order of uses is undefined, and whatever checks are done for just one user and not done for the others may be a source of non-determinism.
Can we split out refactoring + verifier changes (that expose some failues) and fixes for those failures? I'd like to see the tests and situations where they fail to make sure these functional fixes really address the problems. If those failures aren't too numerous or disrupting, please do.
Nov 2 2021
Wow, that's impressive number of noop updates. :) Change in pr27133.ll is OK and widen-loop-comp.ll has improvements so LGTM. Passing there TTI totally makes sense.
Nov 1 2021
Should be fine!
If I understand your proposal correctly, you propose to replace scheme "take signedness from 1st user which is non-deterministically selected" with scheme "if at least one user of widest type is signed cast, widen as signed; otherwize widen as unsigned"?
Deletion of loops before they are vectorized absolutely makes sense to me. Please give it couple more days just in case if someone has concerns regarding the regressions, but making vectorizer run on empty doesn't sound. Please give it 2-3 days to hear others' concerns.
(Please don't mind -1 for me as it was just for sorting out review list purposes)
Oct 31 2021
This one is now ready for review. Please feel free to take a look.
Some more detailed description: imagine each block had a unique call signaling that we entered it. In the initial test, path bb->bb1->bb5 did exist and was legal. But if after bb we've decided to go to bb2, then there is no possible way we can ever get to bb5. But with this transform, we sure can (bb->bb2->bb1->bb5), for no explainable reason. Guard widening doesn't explain it. It's a bug.
Philip, the bug was indeed revealed on attempt to change the frontend, but there was no bug in initial IR. You are right that widenable condition does not require anything specific be down the false path. But guard widening does. All deopts are mutually replacable, but you can't replace a deopt with a random branch just because there is a widenable condition elsewhere. This makes SimplifyCFG somewhat mad.
Oct 29 2021
Addressed Nikita's non-determinism concern.
Well, uniqueness is to reduce overhead for the users. If that concerns you, I'll rewrite it into returning vector of unique elements in deterministic order.
Oct 28 2021
Oct 27 2021
Oct 26 2021
Upon that, I'm going to merge it tomorrow if there will be no objections.
Oct 25 2021
I have no idea what you mean by this. Expand?
Looks fine for me, but let's give other reviewers time to raise their concerns. And please run some extensive fuzzer testing for it.
Oct 24 2021
Not 100% sure it should be UniqueSCEVs to iterate on; besides, we don't verify obsolete dangling users so far (at least not before we start dropping them). Ideas for further improvement welcome.
Oct 22 2021
All splittable parts are split out.
Needs rebase. Withdrawing from review for now.
LGTM, but I still think that "folded" is not a proper term here.