[PATCH] D130910: [NewGVN][PHIOFOPS] Bail-out if an operand is in OpSafeForPHIOfOps but it is not safe for the current basic block.

[Konstantina Mitropoulou via Phabricator via llvm-commits]

kmitropoulou created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
kmitropoulou requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Fix for 53807.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130910

Files:
  llvm/lib/Transforms/Scalar/NewGVN.cpp
  llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll


Index: llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
===================================================================
--- llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
+++ llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
@@ -186,3 +186,60 @@
 }
 
 declare void @f()
+
+define void @issfeoperand([3 x [2 x [1 x i8]]]* nocapture readonly %array, i1 %cond1, i1 %cond2, i16* nocapture readonly %p1, i32* nocapture writeonly %p2, i32* nocapture writeonly %p3) local_unnamed_addr #0 {
+; CHECK-LABEL: @issfeoperand(
+; CHECK-NEXT:  for.body:
+; CHECK-NEXT:    br i1 [[COND1:%.*]], label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+; CHECK:       cond.true:
+; CHECK-NEXT:    [[ARRAYIDX31:%.*]] = getelementptr inbounds [3 x [2 x [1 x i8]]], [3 x [2 x [1 x i8]]]* [[ARRAY:%.*]], i64 109, i64 0, i64 0, i64 undef
+; CHECK-NEXT:    [[LD1:%.*]] = load i8, i8* [[ARRAYIDX31]], align 1
+; CHECK-NEXT:    br label [[COND_FALSE]]
+; CHECK:       cond.false:
+; CHECK-NEXT:    [[PHI1:%.*]] = phi i8 [ [[LD1]], [[COND_TRUE]] ], [ 0, [[FOR_BODY:%.*]] ]
+; CHECK-NEXT:    [[ARRAYIDX42:%.*]] = getelementptr inbounds [3 x [2 x [1 x i8]]], [3 x [2 x [1 x i8]]]* [[ARRAY]], i64 109, i64 0, i64 0, i64 undef
+; CHECK-NEXT:    [[LD2:%.*]] = load i8, i8* [[ARRAYIDX42]], align 1
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp ult i8 [[LD2]], [[PHI1]]
+; CHECK-NEXT:    [[ZEXT:%.*]] = zext i1 [[CMP1]] to i32
+; CHECK-NEXT:    store i32 [[ZEXT]], i32* [[P2:%.*]], align 4
+; CHECK-NEXT:    br i1 [[COND2:%.*]], label [[COND_END:%.*]], label [[EXIT:%.*]]
+; CHECK:       cond.end:
+; CHECK-NEXT:    [[LD3:%.*]] = load i16, i16* [[P1:%.*]], align 2
+; CHECK-NEXT:    [[SEXT:%.*]] = sext i16 [[LD3]] to i32
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       exit:
+; CHECK-NEXT:    [[PHI2:%.*]] = phi i32 [ [[SEXT]], [[COND_END]] ], [ 0, [[COND_FALSE]] ]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i8 [[LD2]], 0
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[CMP2]], i32 [[PHI2]], i32 0
+; CHECK-NEXT:    store i32 [[SEL]], i32* [[P3:%.*]], align 4
+; CHECK-NEXT:    ret void
+;
+for.body:
+  br i1 %cond1, label %cond.true, label %cond.false
+
+cond.true:                                        ; preds = %for.body
+  %arrayidx31 = getelementptr inbounds [3 x [2 x [1 x i8]]], [3 x [2 x [1 x i8]]]* %array, i64 109, i64 0, i64 0, i64 undef
+  %ld1 = load i8, i8* %arrayidx31, align 1
+  br label %cond.false
+
+cond.false:                                       ; preds = %cond.true, %for.body
+  %phi1 = phi i8 [ %ld1, %cond.true ], [ 0, %for.body ]
+  %arrayidx42 = getelementptr inbounds [3 x [2 x [1 x i8]]], [3 x [2 x [1 x i8]]]* %array, i64 109, i64 0, i64 0, i64 undef
+  %ld2 = load i8, i8* %arrayidx42, align 1
+  %cmp1 = icmp ult i8 %ld2, %phi1
+  %zext = zext i1 %cmp1 to i32
+  store i32 %zext, i32* %p2, align 4
+  br i1 %cond2, label %cond.end, label %exit
+
+cond.end:                                         ; preds = %cond.false
+  %ld3 = load i16, i16* %p1, align 2
+  %sext = sext i16 %ld3 to i32
+  br label %exit
+
+exit:                                             ; preds = %cond.end, %cond.false
+  %phi2 = phi i32 [ %sext, %cond.end ], [ 0, %cond.false ]
+  %cmp2 = icmp eq i8 %ld2, 0
+  %sel = select i1 %cmp2, i32 %phi2, i32 0
+  store i32 %sel, i32* %p3, align 4
+  ret void
+}
Index: llvm/lib/Transforms/Scalar/NewGVN.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -2582,8 +2582,19 @@
   if (!isa<Instruction>(V))
     return true;
   auto OISIt = OpSafeForPHIOfOps.find(V);
-  if (OISIt != OpSafeForPHIOfOps.end())
-    return OISIt->second;
+  if (OISIt != OpSafeForPHIOfOps.end()) {
+    // Let's assume that we have the following scenario: an operand is not safe
+    // for the basic block where it is defined and it is safe for one of its
+    // successors. In this case, it will be added in OpSafeForPHIOfOps as a safe
+    // operand. In the second run of NewGVN, it will be considered a safe
+    // operand even for the first case. To avoid this problem, we meed to
+    // bail-out here.
+    const Instruction *I = cast<Instruction>(OISIt->first);
+    const BasicBlock *BBI = I->getParent();
+    return BBI == PHIBlock && (I->mayReadFromMemory() || isa<PHINode>(I))
+               ? false
+               : OISIt->second;
+  }
 
   // Keep walking until we either dominate the phi block, or hit a phi, or run
   // out of things to check.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D130910.449031.patch
Type: text/x-patch
Size: 4462 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220801/2eb8b479/attachment.bin>