[PATCH] D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y)

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D85533#2203926 <https://reviews.llvm.org/D85533#2203926>, @spatel wrote:

> Is there a reason to do this in InstCombine rather than InstSimplify? We are not creating any new instructions.

Since this transformation takes the number of uses of freeze into consideration, InstCombine is a better place to handle it IMO.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2540
+  // The freeze should be only used by this select. Otherwise, remaining uses of
+  // the freeze can observe a contradictory value.
+  //   c = freeze(x == y)   ; Let's assume that y = poison & x = 42; c is 0 or 1
----------------
nikic wrote:
> I agree the one-use check is necessary, but weirdly alive doesn't seem to require it: https://alive2.llvm.org/ce/z/hzwzM_
Yes, actually there is a bug regarding this pattern: https://github.com/AliveToolkit/alive2/issues/450
I'll have a look at this.


================
Comment at: llvm/test/Transforms/InstCombine/select.ll:2588
+  call void @use_i1_i32(i1 %c.fr, i32 %v)
+  ret void
+}
----------------
efriedma wrote:
> If I'm understanding correctly, the following transform would be legal:
> 
> ```
> define void @select_freeze_icmp_multuses(i32 %x, i32 %y) {
>   %x.fr = freeze i32 %x
>   %y.fr = freeze i32 %y
>   %c = icmp ne i32 %x.fr, %y.fr
>   call void @use_i1_i32(i1 %c, i32 %x.fr)
>   ret void
> }
> ```
> 
> Is this something we should be looking into?
Yes it is legal, but it did not appear on my codebase.
If it is beneficial to place comparison just before specific operations, we can add it in SelDag.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85533/new/

https://reviews.llvm.org/D85533