User Details
- User Since
- Jan 17 2017, 8:49 PM (322 w, 4 d)
Fri, Mar 10
+1 for landing this patch if there is no significant input from the discourse thread by next Monday (Mar. 13).
Wed, Mar 8
H, is D104790 superseded by this patch? I wonder what is the status of this patch as well.
D143129 will do what this patch was supposed to do.
Tue, Mar 7
Regarding the updated experimental results, I think people might have thoughts about them. I will leave a comment there.
Thanks..! :)
Hi, I am currently occupied with something else. I will be back by today and continue reviewing this.
Sat, Mar 4
ping
Tue, Feb 28
LGTM, thanks..!
Mon, Feb 27
Thanks! Yes, indeed there are still many files having diffs. But the number shrunk to ~1/5, which seems nice!
Sun, Feb 26
We could carve out some exceptions to this bailout. For example, the transform is actually ok with udiv/urem and a common divisor because poison in the dividend would not cause immediate UB - the divisor has to be zero, and that would be UB in the original code too. That doesn't work with signed div/rem because we can choose the poison dividend to be MinSignedValue, and that overflows when divided by -1 causing UB that doesn't exist in the original code.
Allow transformation or types that are to be promoted
Feb 24 2023
Feb 23 2023
Feb 22 2023
Closing this as D142388 added a function that can be used instead
Feb 20 2023
Feb 18 2023
Reflect comments
Feb 16 2023
Use isMinSignedConstant
Feb 15 2023
Feb 10 2023
The one case I'm probably worried about most are checks of the form %p == null, where replacing %p with null is not generally legal because it has nullary provenance. Replacing two arbitrary pointers likely isn't super important, but replacement with null might be.
Feb 1 2023
Looks good to me in high level, but I suggest waiting for one more accept since I haven't been working with the LLVM codebase for a while.
Dec 12 2022
I checked the updated instructions' LLVM LangRef documentation as well as cppreference.com description and they looked good to me
Dec 5 2022
The clarification looks good to me as well, thanks.
Since LLVM IR does not have a notion of unspecified value (AFAIK), defining these as returning a nondeterministic, well-defined value seems like a right choice because it satisfies refinement.
Making them return an undefined value or poison value breaks refinement.
Oct 25 2022
The updates are analogous to how GNUNullExprClass is processed because UnspecifiedValueExprClass and it is similar (have no operand).
Aug 10 2022
Aug 4 2022
Aug 3 2022
Define undef_or_freeze_undef and use it for the new pattern
Jul 29 2022
Jul 27 2022
The logic looks good to me, but it would be great if the patch gets approvals from people familiar with SelectionDag.
Jul 22 2022
Jun 27 2022
PR31524 (https://github.com/llvm/llvm-project/issues/31524) discusses about the lowering of such intrinsics.
According to the PR, it seems users consider undefined intrinsics as returning unknown but consistent bits.
Jun 19 2022
It seems llvm/lib/Target/X86/X86ISelLowering.cpp's LowerAVXCONCAT_VECTORS is relevant to efficient lowering of shufflevector %x, freeze(poison), mask.
Jun 17 2022
Thank you for confirming it!
Jun 16 2022
I suspect that this also obsoletes the CanonicalizeFreezeInLoops pass, and we can probably drop it.
Jun 6 2022
May 24 2022
Per LangRef statements of the definition of dereferenceable(<n>) attribute:
The pointer should be well defined, otherwise it is undefined behavior. This means dereferenceable(<n>) implies noundef.
May 18 2022
LGTM
May 17 2022
Make test show diff, add a description about why @and_noopt is not optimized yet, update comments in ProcessImpliedCondition
May 11 2022
May 2 2022
Hi, sorry for my delay. The old assertion seems problematic.
May 1 2022
Apr 30 2022
This has been observed as a real-world miscompile with rustc.
I agree that introducing ptrtoint + inttoptr here doesn't sound like a good idea because both it is bad for alias analysis and its correctness is not clear.
There was a similar miscompilation report (which was introduced by my patch) and a bug fix due to the existence of select cond, true, false in the past: https://reviews.llvm.org/rG5bb38e84d3d0#986154 and https://reviews.llvm.org/rG6b4b1dc6ec6f0bf0a1bb414fbe751ccab99d41a0
Also a follow-up patch to enhance an assertion condition: https://reviews.llvm.org/rG431a40e1e28f181e87dd247b91a5e6872dd64ab4
Apr 27 2022
Yep, a test added
Apr 25 2022
Apr 23 2022
haveSameProvenanceIfEqual seems like a better name, certainly.
I began to think that we might not need to make replacement of pointers 'directional' because it is too complex.
What about renaming canReplacePointersIfEqual in Loads.h to haveSameProvenance and only checking the equivalence of two pointers, which will make things simpler (including this patch)?
It sounds great - then after the removal of the old pass I will make a patch that removes https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L2508 as well.
A quick question: should LoopUnswitch.cpp be fixed as well? It seems the pass is not maintained though. Will SimpleLoopUnswitch supercede the LoopUnswitch pass?
Apr 22 2022
Thank you for your work..! The diff looks fine.
Apr 21 2022
Apr 19 2022
I confirmed that there are a few other architectures assuming the same semantics:
- AArch64: https://llvm.godbolt.org/z/b5M5j6r9h
- RISCV32: https://llvm.godbolt.org/z/1ax4s5jPo
- RISCV64: https://llvm.godbolt.org/z/W59oazebx
- X86 (32-bit): https://llvm.godbolt.org/z/s3PTznrxa
Mar 1 2022
Also, could you leave a link to Alive2 in the patch description showing that pushing freeze into icmp ops is safe when there are multiple users?
Jan 26 2022
Jan 7 2022
Since x == 0 ? x : umin(x, y) cannot be represented using the current SCEV operations (at least using the ops in SCEVTypes). I believe the new ops in this patch are necessary.
Another approach to support such expressions would be adding a ternary operator and comparisons to SCEV, but it would require bigger changes, I guess?
Jan 5 2022
Dec 15 2021
This sounds like a great fix to me! :)