User Details
- User Since
- Jan 17 2017, 8:49 PM (279 w, 3 d)
Tue, May 24
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.
Wed, May 18
LGTM
Tue, May 17
Make test show diff, add a description about why @and_noopt is not optimized yet, update comments in ProcessImpliedCondition
Wed, May 11
Mon, May 2
Hi, sorry for my delay. The old assertion seems problematic.
Sun, May 1
Sat, Apr 30
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! :)
Dec 13 2021
Nov 25 2021
Nov 12 2021
Hello all,
I couldn't find this mail because it was somehow buried in other mails.
Nov 5 2021
Nov 3 2021
Nov 1 2021
I am not familiar with inline assembly, but it seems the output variable (%0) is not updated because it does not appear in the code.
Oct 28 2021
Oct 18 2021
I will revert this patch, since its fix needs some time for me to investigate.
I have a colleague who has access to an AArch64 server, so I can give it a try by myself.
Oct 17 2021
It seems the original code has a use of an uninitialized variable.
Line 4420 at seek-preproc.c (function ff_seek_frame_binary):
int64_t pos_min=pos_min, pos_max=pos_max, pos, pos_limit; // pos_min and pos_max are self-assigned. ... if (sti->index_entries) { ... } // pos_min and pos_max are used as arguments below pos = ff_gen_search(s, stream_index, target_ts, pos_min, pos_max, pos_limit, ts_min, ts_max, flags, &ts, avif->read_timestamp);
I can see that @ff_seek_frame_binary is the only affected function.
It introduces llvm.assume as well as !nonnull at a few places and folds null pointer checks.
Still investigating..
Oct 16 2021
Oct 15 2021
There has been no concern in a week, so I'd like to land this patch and D108453 in this weekend.
I'll carefully watch the buildbots and address failures if any.
@eugenis could you accept this patch and D108453 please, if you don't mind?
Oct 12 2021
I thought about the validity of this transformation.
To be pedantic, the validity is unclear when ptr is
(1) an out-of-bounds pointer such that (intptr_t)ptr is 0.
But, I remember people wanted to regard free(ptr) as equivalent to free(null) in that case, so it would be fine.
(2) a partially undefined pointer s.t. ptr = undef | 1
But... I couldn't see any such expression appear in practice, and I don't think LLVM is strictly correct with respect to partially undef pointers
The change looks great to me.
Oct 7 2021
Thank you!
Sep 29 2021
Sep 28 2021
LGTM
Sep 27 2021
@spatel Thanks!
@hyeongyukim Do you mind if we add the new CreateInsertElements and use them in this patch?
Sep 26 2021
Explicitly state that undef can be given as a shuffle mask and its meaning is equivalent to that of the poison mask
Resurrect mistakenly removed test statements
Rebase
Sep 25 2021
I left comments on a few changes - I think it is okay to leave ones that are clearly valid if you prefer.
Sep 20 2021
Aug 22 2021
Hello all, I'm a messenger of @hyeongyukim's two patches that allow clang to turn on noundef analysis by default.
The motivation and background of these patches are described at https://reviews.llvm.org/D105169#2959464.
Hello all,
Rebase, rename disable-noundef-args to disable-noundef-analysis, remove redundant diffs
Rename disable-noundef-args to disable-noundef-analysis
Aug 21 2021
In terms of correctness, I think the patch looks okay.
Aug 20 2021
Aug 19 2021
I became to wonder what happens if a pointer is freed in the function.
For example:
// assume that %p is dereferenceable(8) call void @g(%p); %q = inttoptr (ptrtoint %p); use(%q); // can we replace this with %p?
Aug 9 2021
LGTM, thank you for writing this patch.
Since the poison values in the insertelement and shufflevector are not visible from shufflevector's users, I believe this change is harmless.
Aug 8 2021
Aug 3 2021
I think so? it sounds great.
Could you try inserting freeze to icmp only when one of its operands is constant?
ex) freeze (icmp op0, 1) -> icmp (freeze(op0), 1)
ex2) freeze (icmp op0, op1) -/-> icmp (freeze(op0), freeze(op1)) <= this splits one freeze into two; its benefit is not clear.
Aug 2 2021
I left a few nitpicks that is probably worth addressed
LGTM.
Jul 30 2021
If loop unswitch introduced the freeze, what about adding an extra InstCombine pass between the loop unswitch and SimplifyCFG?
As suggested, finding a common dominator and sinking the instruction to the location seems reasonable and worth trying.
It will help reduce the diff at line 3866~, I hope.
If DT is null, fallback to the old one use check?
Just found that this old patch is still open - I'd like to revisit this later since freeze can be used to resurrect this transformation. This will require investigation into the impact of freeze.
LGTM too.
I have a question about addrspacecast: is ptrtoint(inttoptr(ptrtoint pty1 p) to pty2) equivalent to ptrtoint(addrspacecast pty1 p to pty2)?
Does anyone have idea about this?
Jul 27 2021
Since there is a second opinion about using TBAA, will close this for now.