[Attributor][FIX] Do not replace a value with a non-dominating instru…

…ction We have to be careful when we replace values to not use a non-dominating instruction. It makes sense that simplification offers those as "simplified values" but we can't manifest them in the IR without PHI nodes. In the future we should consider potentially adding those PHI nodes.
llvm · Jul 10, 2021 · dbb3a65 · dbb3a65
1 parent a647040
commit dbb3a65
Show file tree

Hide file tree

Showing 12 changed files with 504 additions and 207 deletions.
diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h b/llvm/include/llvm/Transforms/IPO/Attributor.h
@@ -142,6 +142,12 @@ namespace AA {
 /// instruction/argument of \p Scope.
 bool isValidInScope(const Value &V, const Function *Scope);
 
+/// Return true if \p V is a valid value at position \p CtxI, that is a
+/// constant, an argument of the same function as \p CtxI, or an instruction in
+/// that function that dominates \p CtxI.
+bool isValidAtPosition(const Value &V, const Instruction &CtxI,
+                       InformationCache &InfoCache);
+
 /// Try to convert \p V to type \p Ty without introducing new instructions. If
 /// this is not possible return `nullptr`. Note: this function basically knows
 /// how to cast various constants.

diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
@@ -176,6 +176,22 @@ bool AA::isValidInScope(const Value &V, const Function *Scope) {
   return false;
 }
 
+bool AA::isValidAtPosition(const Value &V, const Instruction &CtxI,
+                           InformationCache &InfoCache) {
+  if (isa<Constant>(V))
+    return true;
+  const Function *Scope = CtxI.getFunction();
+  if (auto *A = dyn_cast<Argument>(&V))
+    return A->getParent() == Scope;
+  if (auto *I = dyn_cast<Instruction>(&V))
+    if (I->getFunction() == Scope) {
+      const DominatorTree *DT =
+          InfoCache.getAnalysisResultForFunction<DominatorTreeAnalysis>(*Scope);
+      return DT && DT->dominates(I, &CtxI);
+    }
+  return false;
+}
+
 Value *AA::getWithType(Value &V, Type &Ty) {
   if (V.getType() == &Ty)
     return &V;

diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -4385,16 +4385,20 @@ struct AAValueSimplifyImpl : AAValueSimplify {
 
   /// Return a value we can use as replacement for the associated one, or
   /// nullptr if we don't have one that makes sense.
-  virtual Value *getReplacementValue() const {
+  Value *getReplacementValue(Attributor &A) const {
     Value *NewV;
     NewV = SimplifiedAssociatedValue.hasValue()
                ? SimplifiedAssociatedValue.getValue()
                : UndefValue::get(getAssociatedType());
     if (!NewV)
       return nullptr;
     NewV = AA::getWithType(*NewV, *getAssociatedType());
-    if (!NewV || NewV == &getAssociatedValue() ||
-        !AA::isValidInScope(*NewV, getAnchorScope()))
+    if (!NewV || NewV == &getAssociatedValue())
+      return nullptr;
+    const Instruction *CtxI = getCtxI();
+    if (CtxI && !AA::isValidAtPosition(*NewV, *CtxI, A.getInfoCache()))
+      return nullptr;
+    if (!CtxI && !AA::isValidInScope(*NewV, getAnchorScope()))
       return nullptr;
     return NewV;
   }
@@ -4449,7 +4453,7 @@ struct AAValueSimplifyImpl : AAValueSimplify {
     if (getAssociatedValue().user_empty())
       return Changed;
 
-    if (auto *NewV = getReplacementValue()) {
+    if (auto *NewV = getReplacementValue(A)) {
       LLVM_DEBUG(dbgs() << "[ValueSimplify] " << getAssociatedValue() << " -> "
                         << *NewV << " :: " << *this << "\n");
       if (A.changeValueAfterManifest(getAssociatedValue(), *NewV))
@@ -4596,7 +4600,7 @@ struct AAValueSimplifyReturned : AAValueSimplifyImpl {
   ChangeStatus manifest(Attributor &A) override {
     ChangeStatus Changed = ChangeStatus::UNCHANGED;
 
-    if (auto *NewV = getReplacementValue()) {
+    if (auto *NewV = getReplacementValue(A)) {
       auto PredForReturned =
           [&](Value &, const SmallSetVector<ReturnInst *, 4> &RetInsts) {
             for (ReturnInst *RI : RetInsts) {
@@ -4821,7 +4825,7 @@ struct AAValueSimplifyCallSiteArgument : AAValueSimplifyFloating {
   ChangeStatus manifest(Attributor &A) override {
     ChangeStatus Changed = ChangeStatus::UNCHANGED;
 
-    if (auto *NewV = getReplacementValue()) {
+    if (auto *NewV = getReplacementValue(A)) {
       Use &U = cast<CallBase>(&getAnchorValue())
                    ->getArgOperandUse(getCallSiteArgNo());
       if (A.changeUseAfterManifest(U, *NewV))

diff --git a/llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll b/llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll
@@ -32,21 +32,39 @@ entry:
 
 define i64 @fn2b(i32 %arg) {
 ;
-; IS__TUNIT____: Function Attrs: nofree nosync nounwind readnone willreturn
-; IS__TUNIT____-LABEL: define {{[^@]+}}@fn2b
-; IS__TUNIT____-SAME: (i32 [[ARG:%.*]]) #[[ATTR0]] {
-; IS__TUNIT____-NEXT:  entry:
-; IS__TUNIT____-NEXT:    [[CONV:%.*]] = sext i32 [[ARG]] to i64
-; IS__TUNIT____-NEXT:    [[DIV:%.*]] = sdiv i64 8, [[CONV]]
-; IS__TUNIT____-NEXT:    ret i64 [[DIV]]
+; IS__TUNIT_OPM: Function Attrs: nofree nosync nounwind readnone willreturn
+; IS__TUNIT_OPM-LABEL: define {{[^@]+}}@fn2b
+; IS__TUNIT_OPM-SAME: (i32 [[ARG:%.*]]) #[[ATTR0]] {
+; IS__TUNIT_OPM-NEXT:  entry:
+; IS__TUNIT_OPM-NEXT:    [[CONV:%.*]] = sext i32 [[ARG]] to i64
+; IS__TUNIT_OPM-NEXT:    [[DIV:%.*]] = sdiv i64 8, [[CONV]]
+; IS__TUNIT_OPM-NEXT:    [[CALL2:%.*]] = call i64 @fn1(i64 [[DIV]]) #[[ATTR0]]
+; IS__TUNIT_OPM-NEXT:    ret i64 [[CALL2]]
 ;
-; IS__CGSCC____: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
-; IS__CGSCC____-LABEL: define {{[^@]+}}@fn2b
-; IS__CGSCC____-SAME: (i32 [[ARG:%.*]]) #[[ATTR0]] {
-; IS__CGSCC____-NEXT:  entry:
-; IS__CGSCC____-NEXT:    [[CONV:%.*]] = sext i32 [[ARG]] to i64
-; IS__CGSCC____-NEXT:    [[DIV:%.*]] = sdiv i64 8, [[CONV]]
-; IS__CGSCC____-NEXT:    ret i64 [[DIV]]
+; IS__TUNIT_NPM: Function Attrs: nofree nosync nounwind readnone willreturn
+; IS__TUNIT_NPM-LABEL: define {{[^@]+}}@fn2b
+; IS__TUNIT_NPM-SAME: (i32 [[ARG:%.*]]) #[[ATTR0]] {
+; IS__TUNIT_NPM-NEXT:  entry:
+; IS__TUNIT_NPM-NEXT:    [[CONV:%.*]] = sext i32 [[ARG]] to i64
+; IS__TUNIT_NPM-NEXT:    [[DIV:%.*]] = sdiv i64 8, [[CONV]]
+; IS__TUNIT_NPM-NEXT:    ret i64 [[DIV]]
+;
+; IS__CGSCC_OPM: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
+; IS__CGSCC_OPM-LABEL: define {{[^@]+}}@fn2b
+; IS__CGSCC_OPM-SAME: (i32 [[ARG:%.*]]) #[[ATTR0]] {
+; IS__CGSCC_OPM-NEXT:  entry:
+; IS__CGSCC_OPM-NEXT:    [[CONV:%.*]] = sext i32 [[ARG]] to i64
+; IS__CGSCC_OPM-NEXT:    [[DIV:%.*]] = sdiv i64 8, [[CONV]]
+; IS__CGSCC_OPM-NEXT:    [[CALL2:%.*]] = call i64 @fn1(i64 [[DIV]]) #[[ATTR1:[0-9]+]]
+; IS__CGSCC_OPM-NEXT:    ret i64 [[CALL2]]
+;
+; IS__CGSCC_NPM: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
+; IS__CGSCC_NPM-LABEL: define {{[^@]+}}@fn2b
+; IS__CGSCC_NPM-SAME: (i32 [[ARG:%.*]]) #[[ATTR0]] {
+; IS__CGSCC_NPM-NEXT:  entry:
+; IS__CGSCC_NPM-NEXT:    [[CONV:%.*]] = sext i32 [[ARG]] to i64
+; IS__CGSCC_NPM-NEXT:    [[DIV:%.*]] = sdiv i64 8, [[CONV]]
+; IS__CGSCC_NPM-NEXT:    ret i64 [[DIV]]
 ;
 entry:
   %conv = sext i32 %arg to i64
@@ -83,11 +101,23 @@ entry:
 }
 
 define internal i64 @fn1(i64 %p1) {
-; IS__CGSCC____: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
-; IS__CGSCC____-LABEL: define {{[^@]+}}@fn1
-; IS__CGSCC____-SAME: (i64 [[P1:%.*]]) #[[ATTR0]] {
-; IS__CGSCC____-NEXT:  entry:
-; IS__CGSCC____-NEXT:    ret i64 undef
+; IS__TUNIT_OPM: Function Attrs: nofree nosync nounwind readnone willreturn
+; IS__TUNIT_OPM-LABEL: define {{[^@]+}}@fn1
+; IS__TUNIT_OPM-SAME: (i64 returned [[P1:%.*]]) #[[ATTR0]] {
+; IS__TUNIT_OPM-NEXT:  entry:
+; IS__TUNIT_OPM-NEXT:    ret i64 [[P1]]
+;
+; IS__CGSCC_OPM: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
+; IS__CGSCC_OPM-LABEL: define {{[^@]+}}@fn1
+; IS__CGSCC_OPM-SAME: (i64 returned [[P1:%.*]]) #[[ATTR0]] {
+; IS__CGSCC_OPM-NEXT:  entry:
+; IS__CGSCC_OPM-NEXT:    ret i64 [[P1]]
+;
+; IS__CGSCC_NPM: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
+; IS__CGSCC_NPM-LABEL: define {{[^@]+}}@fn1
+; IS__CGSCC_NPM-SAME: (i64 [[P1:%.*]]) #[[ATTR0]] {
+; IS__CGSCC_NPM-NEXT:  entry:
+; IS__CGSCC_NPM-NEXT:    ret i64 undef
 ;
 entry:
   %tobool = icmp ne i64 %p1, 0
@@ -97,5 +127,8 @@ entry:
 ;.
 ; IS__TUNIT____: attributes #[[ATTR0]] = { nofree nosync nounwind readnone willreturn }
 ;.
-; IS__CGSCC____: attributes #[[ATTR0]] = { nofree norecurse nosync nounwind readnone willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR0]] = { nofree norecurse nosync nounwind readnone willreturn }
+; IS__CGSCC_OPM: attributes #[[ATTR1]] = { readnone willreturn }
+;.
+; IS__CGSCC_NPM: attributes #[[ATTR0]] = { nofree norecurse nosync nounwind readnone willreturn }
 ;.