[PATCH] D83139: [InstCombine] Always try to invert non-canonical predicate of an icmp

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LG from my side, but would be great if @spatel can chime in as well. We generally avoid folds that inspect users of an instruction, but I think there's reasonable motivation here. It would be great if you can provide your original test case that shows the missed CSE opportunity, as a sanity check that this can't be solved in some other way.

> fcmp is a can of worms i'm not inclined to touch here..

Did you run into some particular issue on that front?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineInternal.h:219
 ///
 /// See also: isFreeToInvert()
 static inline bool canFreelyInvertAllUsersOf(Value *V, Value *IgnoredUser) {
----------------
Add a comment to keep this synced with canonicalizeICmpPredicate()?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineInternal.h:230
+      if (U.getOperandNo() != 0) // Only if the value is used as select cond.
+        return false;
+      break;
----------------
Nice catch!


================
Comment at: llvm/test/Transforms/LoopUnroll/runtime-loop-multiple-exits.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -loop-unroll -unroll-runtime=true -unroll-runtime-epilog=true -unroll-runtime-multi-exit=true -verify-loop-lcssa -verify-dom-info -verify-loop-info -S | FileCheck %s -check-prefix=EPILOG-NO-IC
----------------
I don't see what has changed here. Can you either precommit, or only adjust the changed part? (Not sure if whoever wrote this appreciates the large generated output.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83139