[IRSim] Make sure that commutative intrinsics are treated as function calls without commutativity

[AndrewLitteken]

Summary

Created to fix: https://github.com/llvm/llvm-project/issues/53537

Some intrinsics functions are considered commutative since they are performing operations like addition or multiplication. Some of these have extra parameters to provide extra information that are not part of the operation itself and are not commutative. This makes sure that if an instruction that is an intrinsic takes the non commutative path to handle this case.

[paquette]

I think this fix is fine for the purposes of getting the crash out of the way. But I think it's possible to fix this and also support commutative intrinsics at the same time.

In IntrinsicInst.h:

/// Return true if swapping the first two arguments to the intrinsic produces
/// the same result.
bool isCommutative() const {
   ...

We know that the first two arguments are the only ones that matter. So, maybe we can teach the similarity identifier to understand that? (The commutative case for intrinsics was added in D86798)

[AndrewLitteken]

I think we should be able to do that, right now it's an either or for the commutative path vs the non commutative path, but I think we should be give the similarity identifier a sublist of values that are commutative and a sublist that are noncommutative and run both functions.

[paquette]

Do you think you could add a TODO for commutative intrinsic support?

llvm/lib/Analysis/IRSimilarityIdentifier.cpp

for (unsigned Loc = A.getStartIdx(); Loc < SectionLength;
     std::tie(ValueMappingIt, WasInserted) = ValueNumberMappingB.insert(
         std::make_pair(InstValB, DenseSet<unsigned>({InstValA})));
     if (!WasInserted && !ValueMappingIt->second.contains(InstValA))
       return false;
 
     // We have different paths for commutative instructions and non-commutative
     // instructions since commutative instructions could allow multiple mappings
     // to certain values.
-    if (IA->isCommutative() && !isa<FPMathOperator>(IA)) {
+    if (IA->isCommutative() && !isa<FPMathOperator>(IA) &&
+        !isa<IntrinsicInst>(IA)) {
       if (!compareCommutativeOperandMapping(
               {A, OperValsA, ValueNumberMappingA},
               {B, OperValsB, ValueNumberMappingB}))
         return false;
       continue;
     }
 
     // Handle the non-commutative cases.

llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp

for (std::vector<IRSimilarityCandidate> &Cands : SimilarityCandidates) {
     unsigned InstIdx = 0;
     for (IRSimilarityCandidate &Cand : Cands) {
       ASSERT_TRUE(Cand.getStartIdx() == InstIdx);
       InstIdx += 3;
     }
   }
 }
 
+// This test makes sure that intrinsic functions that are marked commutative
+// are still treated as non-commutative since they are function calls.
+TEST(IRSimilarityIdentifier, InstrinsicCommutative) {
+  // If treated as commutative, we will fail to find a valid mapping, causing
+  // an assertion error.
+  StringRef ModuleString = R"(
+  define void @foo() {
+    entry:
+      %0 = call i16 @llvm.smul.fix.i16(i16 16384, i16 16384, i32 15)
+      store i16 %0, i16* undef, align 1
+      %1 = icmp eq i16 undef, 8192
+      call void @bar()
+      %2 = call i16 @llvm.smul.fix.i16(i16 -16384, i16 16384, i32 15)
+      store i16 %2, i16* undef, align 1
+      %3 = icmp eq i16 undef, -8192
+      call void @bar()
+      %4 = call i16 @llvm.smul.fix.i16(i16 -16384, i16 -16384, i32 15)
+      ret void
+  }
+
+  declare void @bar()
+
+  ; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+  declare i16 @llvm.smul.fix.i16(i16, i16, i32 immarg))";
+  LLVMContext Context;
+  std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleString);
+
+  std::vector<std::vector<IRSimilarityCandidate>> SimilarityCandidates;
+  getSimilarities(*M, SimilarityCandidates);
+
+  ASSERT_TRUE(SimilarityCandidates.size() == 0);
+}
+
 // This test checks to see whether we can detect different structure in
 // commutative instructions.  In this case, the second operand in the second
 // add is different.
 TEST(IRSimilarityIdentifier, NoCommutativeSimilarity) {
   StringRef ModuleString = R"(
                           define i32 @f(i32 %a, i32 %b) {
                           bb0:
                              %0 = add i32 %a, %b