[PATCH] D105031: [SLP]Fix non-determinism in PHI sorting.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev created this revision.
ABataev added reviewers: RKSimon, spatel, vdmitrie, dtemirbulatov, anton-afanasyev.
Herald added subscribers: mgrang, hiraditya.
ABataev requested review of this revision.
Herald added a project: LLVM.

Compare type IDs and DFS numbering for basic block instead of addresses
to fix non-determinism.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105031

Files:
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
  llvm/test/Transforms/SLPVectorizer/X86/remark_unsupported.ll


Index: llvm/test/Transforms/SLPVectorizer/X86/remark_unsupported.ll
===================================================================
--- llvm/test/Transforms/SLPVectorizer/X86/remark_unsupported.ll
+++ llvm/test/Transforms/SLPVectorizer/X86/remark_unsupported.ll
@@ -3,7 +3,7 @@
 ; RUN: FileCheck --input-file=%t --check-prefix=YAML %s
 
 ; This type is not supported by SLP
-define void @test(x86_fp80* %i1, x86_fp80* %i2, x86_fp80* %o) {
+define i1 @test(x86_fp80* %i1, x86_fp80* %i2) {
 ; CHECK-LABEL: @test(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[I1_0:%.*]] = load x86_fp80, x86_fp80* [[I1:%.*]], align 16
@@ -19,10 +19,8 @@
 ; CHECK:       end:
 ; CHECK-NEXT:    [[PHI0:%.*]] = phi x86_fp80 [ [[I1_0]], [[ENTRY:%.*]] ], [ [[I2_0]], [[THEN]] ]
 ; CHECK-NEXT:    [[PHI1:%.*]] = phi x86_fp80 [ [[I1_1]], [[ENTRY]] ], [ [[I2_1]], [[THEN]] ]
-; CHECK-NEXT:    store x86_fp80 [[PHI0]], x86_fp80* [[O:%.*]], align 16
-; CHECK-NEXT:    [[O_GEP1:%.*]] = getelementptr inbounds x86_fp80, x86_fp80* [[O]], i64 1
-; CHECK-NEXT:    store x86_fp80 [[PHI1]], x86_fp80* [[O_GEP1]], align 16
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    [[RES:%.*]] = fcmp oeq x86_fp80 [[PHI0]], [[PHI1]]
+; CHECK-NEXT:    ret i1 [[RES]]
 ;
 entry:
   %i1.0 = load x86_fp80, x86_fp80* %i1, align 16
@@ -37,11 +35,8 @@
   br label %end
 end:
   %phi0 = phi x86_fp80 [ %i1.0, %entry ], [ %i2.0, %then ]
-
   %phi1 = phi x86_fp80 [ %i1.1, %entry ], [ %i2.1, %then ]
-  store x86_fp80 %phi0, x86_fp80* %o, align 16
-  %o.gep1 = getelementptr inbounds x86_fp80, x86_fp80* %o, i64 1
-  store x86_fp80 %phi1, x86_fp80* %o.gep1, align 16
+  %res = fcmp oeq x86_fp80 %phi0, %phi1
   ; YAML:      Pass:            slp-vectorizer
   ; YAML-NEXT: Name:            UnsupportedType
   ; YAML-NEXT: Function:        test
@@ -49,5 +44,5 @@
   ; YAML-NEXT:   - String:          'Cannot SLP vectorize list: type '
   ; YAML-NEXT:   - String:          x86_fp80 is unsupported by vectorizer
 
-  ret void
+  ret i1 %res
 }
Index: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
===================================================================
--- llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -8308,7 +8308,7 @@
       // No need to analyze deleted, vectorized and non-vectorizable
       // instructions.
       if (!VisitedInstrs.count(P) && !R.isDeleted(P) &&
-          !P->getType()->isVectorTy())
+          isValidElementType(P->getType()))
         Incoming.push_back(P);
     }
 
@@ -8336,10 +8336,15 @@
     }
 
     // Sort by type, parent, operands.
-    stable_sort(Incoming, [&PHIToOpcodes](Value *V1, Value *V2) {
-      if (V1->getType() < V2->getType())
+    stable_sort(Incoming, [this, &PHIToOpcodes](Value *V1, Value *V2) {
+      assert(isValidElementType(V1->getType()) &&
+             isValidElementType(V2->getType) &&
+             "Expected vectorizable types only.");
+      // It is fine to compare type IDs here, since we expect only vectorizable
+      // types, like ints, floats and pointers, we don't care about other type.
+      if (V1->getType()->getTypeID() < V2->getType()->getTypeID())
         return true;
-      if (V1->getType() > V2->getType())
+      if (V1->getType()->getTypeID() > V2->getType()->getTypeID())
         return false;
       ArrayRef<Value *> Opcodes1 = PHIToOpcodes[V1];
       ArrayRef<Value *> Opcodes2 = PHIToOpcodes[V2];
@@ -8353,10 +8358,15 @@
           continue;
         if (auto *I1 = dyn_cast<Instruction>(Opcodes1[I]))
           if (auto *I2 = dyn_cast<Instruction>(Opcodes2[I])) {
-            if (I1->getParent() < I2->getParent())
-              return true;
-            if (I1->getParent() > I2->getParent())
-              return false;
+            auto *NodeI1 = DT->getNode(I1->getParent());
+            auto *NodeI2 = DT->getNode(I2->getParent());
+            assert(NodeI1 && "Should only process reachable instructions");
+            assert(NodeI2 && "Should only process reachable instructions");
+            assert((NodeI1 == NodeI2) ==
+                       (NodeI1->getDFSNumIn() == NodeI2->getDFSNumIn()) &&
+                   "Different nodes should have different DFS numbers");
+            if (NodeI1 != NodeI2)
+              return NodeI1->getDFSNumIn() < NodeI2->getDFSNumIn();
             InstructionsState S = getSameOpcode({I1, I2});
             if (S.getOpcode())
               continue;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105031.354896.patch
Type: text/x-patch
Size: 4420 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210628/6f2e9629/attachment.bin>