[llvm] 25597f7 - [IR][GVN] allow intrinsics in Instruction's isCommutative query

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2020-08-30T16:49:22-04:00
New Revision: 25597f7783e7038b8a2ee88bb49ac605b211b564

URL: https://github.com/llvm/llvm-project/commit/25597f7783e7038b8a2ee88bb49ac605b211b564
DIFF: https://github.com/llvm/llvm-project/commit/25597f7783e7038b8a2ee88bb49ac605b211b564.diff

LOG: [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.

Added: 
    

Modified: 
    llvm/include/llvm/IR/Instruction.h
    llvm/lib/IR/Instruction.cpp
    llvm/lib/Transforms/Scalar/GVN.cpp
    llvm/lib/Transforms/Scalar/NewGVN.cpp
    llvm/test/Transforms/GVN/commute.ll

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index a03eac0ad40d..ceec001dccf4 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -532,7 +532,7 @@ class Instruction : public User,
   /// 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/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 3a67185bef31..794a73ed28e9 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/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/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index f51596525569..c9e63b6f33df 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/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/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index e7fd201d70e4..f422d1b51b99 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/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/llvm/test/Transforms/GVN/commute.ll b/llvm/test/Transforms/GVN/commute.ll
index 4fb4f99a27fc..f84bc81d143a 100644
--- a/llvm/test/Transforms/GVN/commute.ll
+++ b/llvm/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)