- User Since
- Jan 23 2017, 8:11 PM (185 w, 1 d)
Fri, Aug 7
Mon, Aug 3
Sun, Aug 2
Looks good. Is it still not merged? :)
LGTM. Good catch! Need to think about API to make this more generic.
Seems that SCEV simplification is unable to exploit this fact when computing minimum. I think the right place to fix this is inside getMinExpr.
Fri, Jul 31
LGTM. Please wait for Roman if he has any objections.
Thu, Jul 30
Wed, Jul 29
Tue, Jul 28
Looks good (with nits).
Let's give it a try. :) LGTM with some nits inline.
Fine by me, but not sure if @nikic has any questions.
LGTM. Please add a TODO that the whole block can be collapsed.
Thu, Jul 23
Wed, Jul 22
Though I don't see obvious problems in the new algorithm, I cannot wrap my mind around the old one and can't say if they are doing the same thing. I'd suggest the following reliable way to make sure it is NFC:
- Instead of changing logic of IsValueFullyAvailableInBlock, introduce a new version of it and use it.
- In debug mode, also call the old one and assert that the results are the same.
- Then, after it has been in for a long enough while and we are certain those algorithms do the same thing, remove the old one.
I've updated commit message basing on our discussion. We want to be at a point when "is pointer" notion doesn't get lost. We are clearly not at this point now. We need to assess the amount of work we should do to get there, and construct more tests exercising different scenarios of this.
I think we have some cases (related to mul) where "is pointer" notion can be lost. Maybe we can fix it. Let me try to construct some more examples of those.
Tue, Jul 21
Sun, Jul 19
I'd suggest splitting this in two parts:
- NFC patch with new options & test updates, but preserving current behavior;
- One-liner patch to switch default values.
This is needed to make revert easier in case if it exposes some problems.
Fri, Jul 17
Not sure why it is necessary to rename it in the first place, but looks fine.
@kpolushin please follow-up with the test when you know how to create it.
Thu, Jul 16
Exactly. Approved per comment above, I will merge it now since Kirill has no committer access.
Can we check in this fix and merge the test later? The bug here is pretty obvious. WDYT?
Please add unit test.
Could you please add some .ll tests showing how this change impacts the code being generated (if it is possible)?
Follow-up fix for one of clang tests: https://reviews.llvm.org/D83936
SimplifyCFG's attempts to perform PR are outrageously stupid. It re-inserts (worse version of) removed Phi in order to perform threading, but does not actually thread. I'll fix it separately, it's not blocking this one (it inserts phi anyway, just now it's worse).
One of clang tests shown that we are still missing some case...
Wed, Jul 15
Looks like some clang tests need update too...
Jul 12 2020
Jul 10 2020
Fixed var naming in test that went astray after rebase.
Implemented deduplication (used SetVector to ensure that we have deterministic fold order).
Jul 9 2020
Jul 7 2020
Ideally we could consider all dominator chain, but this is too expensive. This is attempt to reduce the impact.
Jul 6 2020
At least 2 another transforms for selects are missing that we need to avoid regressions. I'm depreoritizing this as these efforts don't pay off.
Still missing cases found for selects... It's getting not worth it.
Jul 3 2020
Err, you're right @nikic. Will do.
Jul 1 2020
Rebased on top of underlying fixes.
Jun 29 2020
commit f01d9e6fc3e291a2faed8c9b7dcbabf760f32bd6 Author: Max Kazantsev <email@example.com> Date: Tue Jun 30 12:38:15 2020 +0700
Jun 24 2020
commit 1eeb7147878edb7c0c0fbf54bc3dffd43db271b8 Author: Max Kazantsev <firstname.lastname@example.org> Date: Thu Jun 25 10:42:16 2020 +0700
Jun 22 2020
Reworked using DoPHITranslation.