- User Since
- Jan 17 2017, 8:49 PM (220 w, 4 d)
Thu, Apr 8
I made https://reviews.llvm.org/D100122 to show how the patch would look like if m_Undef is fixed instead.
Wed, Apr 7
Thank you all! Incremental change makes sense to me as well. It will help smooth landing without buildbot failures.
I'll start writing patches soon.
Mon, Apr 5
This tracks down to a commit in 2010: https://github.com/llvm/llvm-project/commit/d9df6eaa9ce20
Interestingly the motivation of introducing ConstantPreference was to properly deal with indirectbr. :)
I have no strong preference either.
Sun, Apr 4
This turns out to be a bug in InstCombine. I reported it as https://llvm.org/pr49832 .
Sat, Apr 3
Thu, Apr 1
rebase, correctify the test file name
It indeed seems tricky to find out the root cause for this; this optimization enabled vectorization which led to the error for some reason.
I'll have more time to investigate this tomorrow and weekends.
Wed, Mar 31
Tue, Mar 30
I made a fix to keep the main branch in a good state, but not sure this is a real solution because there might be a hidden problem. I'm investigating it.
Mon, Mar 29
I'll make a fix for this, thanks.
Is there a plan to enable -enable-noundef-analysis by default? It seems this patch is already addressing a lot of test cases.
Another good news is that DeadArgElim is also fixed by D98899 to correctly drop UB-raising attributes such as noundef when replacing an unused argument with undef. This was an issue which is known to be problematic when the flag was activated.
I can make a patch based on this work instead if people want.
Sat, Mar 27
Fri, Mar 26
Wed, Mar 24
The core notions here are: 1) separate dereferenceability and object lifetime (in the marker intrinsic sense), and 2) unify handling for all memory object types.
Tue, Mar 23
Mon, Mar 22
Closed in favor of D99027
Closed in favor of D99027
Should be fixed via https://reviews.llvm.org/rGb00209ed100c
Sun, Mar 21
Sat, Mar 20
It is impressive that leaving the few patterns was enough to cover all existing unit tests.
If more patterns are needed, we can factor out the non-refined foldings from SimplifyXXInst as a static function and call it here.
Fri, Mar 19
Wed, Mar 17
In terms of maintenance: if reviewers think the complexity this patch is introducing is big to maintain, I'll turn this patch to using canCreatePoison check since it will fix the bug anyway.
Does this direction look okay? @nikic
One benefit of this approach is that it detects unsafe replacements that have non-poison-creating instructions as well.
For example, this transformation is illegal:
%v = select (x == 0), 0, (x & y) => %v = x & y
If x = 0 and y was poison this makes the result more undefined. However, the current check relying on canCreatePoison cannot detect this.
D98585 fixes this by making the relevant transformation respect Q.AllowRefinement.
Tue, Mar 16
the fact that we now have "multiple kinds of arguments" is confusing.
Address comments, minor update to the grammar
Mon, Mar 15
Thank you all!
check if the argument has noundef
Sun, Mar 14
To be safe, what do you think about marking nosync to ops that can be represented as a series of loads/stores or scalar ops only? For example, I believe memset is nosync because it is equivalent to a series of nonatomic stores.
For side-effecting operations, such as printf, I'm not 100% sure whether it is nosync. printf interacts with cout to properly flush buffers, which might do some interactions.
Make computePointerICmp slightly more conservative if Q.AllowRefinement is false
Sat, Mar 13
Add AllowRefinement field
TBH, I don't feel comfortable with extending the canCreatePoison() check. :( Even if it requires inspection of foldings in InstSimplify, I think a safer way is to add a flag that disallows foldings returning refined values.
Let me update this patch and write following patches for updating them.
Mar 11 2021
Mar 10 2021
Confirmed that this resolves the regression (https://reviews.llvm.org/D95026#2610334) from two-stage build!
Well, I found a way to get around this issue without changing these APIs. :)
Fix deoptimized case