[PATCH] D63349: [EarlyCSE] Fix hashing of self-compares

[Joseph Tremoulet via Phabricator via llvm-commits]

JosephTremoulet marked an inline comment as done.
JosephTremoulet added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:185
+    CmpInst::Predicate SwappedPred = CI->getSwappedPredicate();
+    if (std::tie(LHS, Pred) > std::tie(RHS, SwappedPred)) {
       std::swap(LHS, RHS);
----------------
JosephTremoulet wrote:
> nikic wrote:
> > JosephTremoulet wrote:
> > > nikic wrote:
> > > > Would it be possible to make the condition just `Pred > SwappedPred`, without ordering by LHS/RHS at all?
> > > No, some predicates (eq, ne) are their own swap duals, so we need to check the operands to handle e.g. `icmp eq A, B` vs `icmp eq B, A`.
> > Right you are! In that case, I'm wondering if we're missing the operand swap for equality/inequality for the select case below? We canonicalize by inverse predicate, but I don't think there's anything handling swapping of equality comparison arguments.
> The swap in the select case is swapping the second and third operands on the select and using the inverse predicate (not the swapped predicate), to handle e.g. `A == B ? X : Y` vs `A != B ? Y : X`.  Predicates are never their own inverse, so I think it's ok to ignore X and Y there.
> 
> Of course, there could also be a swap on the compare feeding the select, so e.g. `A == B ? X : Y` is also the same as `B != A ? Y : X`.  We're not handling that case in the hashing, but we're not handling it in isEqual either, so it's a missed optimization opportunity rather than nondeterminism/assertions.  E.g.:
> 
> ```
> declare void @use(i32, i32)
> 
> define void @test(i32 %a, i32 %b, i32 %x, i32 %y) {
>   %c1 = icmp eq i32 %a, %b
>   %t1 = select i1 %c1, i32 %x, i32 %y
>   %c2 = icmp ne i32 %b, %a
>   %t2 = select i1 %c2, i32 %y, i32 %x
>   call void @use(i32 %t1, i32 %t2)
>   ret void
> }
> ```
> 
> doesn't get changed by EarlyCSE (but neither does it assert, even with -earlycse-debug-hash).
> 
> I'm not familiar enough with the composition of the optimization pipeline to have an informed decision whether or not we'd want to add the complexity to handle that to EarlyCSE or not...
Sigh, meant "to have an informed opinion", not "to have an informed decision".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63349