From d58c2f282dbe6fc1a93a13bace0d9f8ed5721daf Mon Sep 17 00:00:00 2001
From: Sanjay Patel <spatel@rotateright.com>
Date: Sun, 30 Aug 2020 15:32:45 -0400
Subject: [PATCH] [IR][GVN] allow intrinsics in Instruction's isCommutative
 query

As discussed in D86798 / rG09652721 , we were potentially
returning a different result for whether an Instruction
is commutable depending on if we call the base class or
derived class method.

This requires relaxing an assert in GVN, but that pass
seems to be working otherwise.

NewGVN requires more work because it uses different
code paths for numbering binops and calls.
---
 include/llvm/IR/Instruction.h    | 2 +-
 lib/IR/Instruction.cpp           | 7 +++++++
 lib/Transforms/Scalar/GVN.cpp    | 4 +++-
 lib/Transforms/Scalar/NewGVN.cpp | 1 +
 test/Transforms/GVN/commute.ll   | 3 +--
 5 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/llvm/IR/Instruction.h b/include/llvm/IR/Instruction.h
index a03eac0ad40..ceec001dccf 100644
--- a/include/llvm/IR/Instruction.h
+++ b/include/llvm/IR/Instruction.h
@@ -532,7 +532,7 @@ public:
   /// In LLVM, these are the commutative operators, plus SetEQ and SetNE, when
   /// applied to any type.
   ///
-  bool isCommutative() const { return isCommutative(getOpcode()); }
+  bool isCommutative() const LLVM_READONLY;
   static bool isCommutative(unsigned Opcode) {
     switch (Opcode) {
     case Add: case FAdd:
diff --git a/lib/IR/Instruction.cpp b/lib/IR/Instruction.cpp
index 3a67185bef3..794a73ed28e 100644
--- a/lib/IR/Instruction.cpp
+++ b/lib/IR/Instruction.cpp
@@ -673,6 +673,13 @@ bool Instruction::isAssociative() const {
   }
 }
 
+bool Instruction::isCommutative() const {
+  if (auto *II = dyn_cast<IntrinsicInst>(this))
+    return II->isCommutative();
+  // TODO: Should allow icmp/fcmp?
+  return isCommutative(getOpcode());
+}
+
 unsigned Instruction::getNumSuccessors() const {
   switch (getOpcode()) {
 #define HANDLE_TERM_INST(N, OPC, CLASS)                                        \
diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp
index f5159652556..c9e63b6f33d 100644
--- a/lib/Transforms/Scalar/GVN.cpp
+++ b/lib/Transforms/Scalar/GVN.cpp
@@ -293,7 +293,9 @@ GVN::Expression GVN::ValueTable::createExpr(Instruction *I) {
     // of their operands get the same value number by sorting the operand value
     // numbers.  Since all commutative instructions have two operands it is more
     // efficient to sort by hand rather than using, say, std::sort.
-    assert(I->getNumOperands() == 2 && "Unsupported commutative instruction!");
+    assert(((isa<BinaryOperator>(I) && I->getNumOperands() == 2) ||
+            (isa<IntrinsicInst>(I) && I->getNumOperands() == 3))
+            && "Unsupported commutative instruction!");
     if (e.varargs[0] > e.varargs[1])
       std::swap(e.varargs[0], e.varargs[1]);
     e.commutative = true;
diff --git a/lib/Transforms/Scalar/NewGVN.cpp b/lib/Transforms/Scalar/NewGVN.cpp
index e7fd201d70e..f422d1b51b9 100644
--- a/lib/Transforms/Scalar/NewGVN.cpp
+++ b/lib/Transforms/Scalar/NewGVN.cpp
@@ -1254,6 +1254,7 @@ const UnknownExpression *NewGVN::createUnknownExpression(Instruction *I) const {
 const CallExpression *
 NewGVN::createCallExpression(CallInst *CI, const MemoryAccess *MA) const {
   // FIXME: Add operand bundles for calls.
+  // FIXME: Allow commutative matching for intrinsics.
   auto *E =
       new (ExpressionAllocator) CallExpression(CI->getNumOperands(), CI, MA);
   setBasicExpressionInfo(CI, E);
diff --git a/test/Transforms/GVN/commute.ll b/test/Transforms/GVN/commute.ll
index 4fb4f99a27f..f84bc81d143 100644
--- a/test/Transforms/GVN/commute.ll
+++ b/test/Transforms/GVN/commute.ll
@@ -32,8 +32,7 @@ define void @cmp(i32 %x, i32 %y) {
 define void @intrinsic(i32 %x, i32 %y) {
 ; CHECK-LABEL: @intrinsic(
 ; CHECK-NEXT:    [[M1:%.*]] = call i32 @llvm.umax.i32(i32 [[X:%.*]], i32 [[Y:%.*]])
-; CHECK-NEXT:    [[M2:%.*]] = call i32 @llvm.umax.i32(i32 [[Y]], i32 [[X]])
-; CHECK-NEXT:    call void @use(i32 [[M1]], i32 [[M2]])
+; CHECK-NEXT:    call void @use(i32 [[M1]], i32 [[M1]])
 ; CHECK-NEXT:    ret void
 ;
   %m1 = call i32 @llvm.umax.i32(i32 %x, i32 %y)