Attributor & Function-attrs mark function as noundef incorrectly due to return value not in range

[nunoplopes]

This example returns a constant value out of range:

define range(i32 0, 42) i32 @range_attribute(<4 x i32> range(i32 -1, 42) %a) {
  ret i32 43
}

Running opt -S -p='attributor' gives an incorrect result:

define noundef range(i32 0, 42) i32 @fn() {
  ret i32 43
}

opt -S -p='function-attrs' has the same bug.

[nikic]

Nice find! This is not specific to the new range attribute, we can do the same thing with nonnull for example: https://llvm.godbolt.org/z/4hz8PEoav

define nonnull ptr @test() {
  ret ptr null
}
; RUN: opt -S -passes=function-attrs
define noalias noundef nonnull ptr @test() #0 {
  ret ptr null
}

Looks like we entirely failed to account for the possibility that a poison value gets introduced by a return attribute in this inference!

[nunoplopes]

Another one with nocapture from @regehr:

define ptr @src(ptr nocapture dereferenceable(2) %0) {
  ret ptr %0
}

define noundef ptr @tgt(ptr nocapture dereferenceable(2) %0) {
  ret ptr %0
}

[antoniofrighetto]

Untested, but maybe something like this should work:

--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -749,6 +749,22 @@ determinePointerAccessAttrs(Argument *A,
     return Attribute::ReadNone;
 }
 
+static bool findRetArg(ReturnInst *Ret, Argument *&RetArg) {
+  // Note that stripPointerCasts should look through functions with
+  // returned arguments.
+  auto *RetVal = dyn_cast<Argument>(Ret->getReturnValue()->stripPointerCasts());
+  if (!RetVal ||
+      RetVal->getType() != Ret->getParent()->getParent()->getReturnType())
+    return false;
+
+  if (!RetArg) {
+    RetArg = RetVal;
+    return true;
+  }
+
+  return false;
+}
+
 /// Deduce returned attributes for the SCC.
 static void addArgumentReturnedAttrs(const SCCNodeSet &SCCNodes,
                                      SmallSet<Function *, 8> &Changed) {
@@ -767,27 +783,12 @@ static void addArgumentReturnedAttrs(const SCCNodeSet &SCCNodes,
     if (F->getAttributes().hasAttrSomewhere(Attribute::Returned))
       continue;
 
-    auto FindRetArg = [&]() -> Argument * {
-      Argument *RetArg = nullptr;
-      for (BasicBlock &BB : *F)
-        if (auto *Ret = dyn_cast<ReturnInst>(BB.getTerminator())) {
-          // Note that stripPointerCasts should look through functions with
-          // returned arguments.
-          auto *RetVal =
-              dyn_cast<Argument>(Ret->getReturnValue()->stripPointerCasts());
-          if (!RetVal || RetVal->getType() != F->getReturnType())
-            return nullptr;
-
-          if (!RetArg)
-            RetArg = RetVal;
-          else if (RetArg != RetVal)
-            return nullptr;
-        }
-
-      return RetArg;
-    };
-
-    if (Argument *RetArg = FindRetArg()) {
+    Argument *RetArg = nullptr;
+    if (all_of(*F, [&](BasicBlock &BB) {
+          if (auto *Ret = dyn_cast<ReturnInst>(BB.getTerminator()))
+            return findRetArg(Ret, RetArg);
+          return false;
+        })) {
       RetArg->addAttr(Attribute::Returned);
       ++NumReturned;
       Changed.insert(F);
@@ -1335,6 +1336,10 @@ static void addNoUndefAttrs(const SCCNodeSet &SCCNodes,
                 !Attr.getRange().contains(
                     computeConstantRange(RetVal, /*ForSigned=*/false)))
               return false;
+
+            Argument *RetArg = nullptr;
+            if (findRetArg(Ret, RetArg) && RetArg->hasNoCaptureAttr())
+              return false;
           }
           return true;
         })) {

[nikic]

@nunoplopes Not sure I get this one -- I would expect nocapture to be UB-implying, not poison generating. I don't get how it can be poison-generating, given that nocapture is generally used by the caller, not the callee.

[nunoplopes]

@nunoplopes Not sure I get this one -- I would expect nocapture to be UB-implying, not poison generating. I don't get how it can be poison-generating, given that nocapture is generally used by the caller, not the callee.

I was debating here with myself.
But let's assume the caller:

1. passes a pointer to a nocapture argument. It assumes that after the call that memory is not stored to by anyone else as the pointer did not escape.
2. the function uses the return value, which is poison because the callee violated the nocapture constraint.

If 1 & 2 happen, then we get UB if the usage is a load/store. Any other operation like icmp that uses the pointer are poison. If the caller doesn't use the returned pointer, then it's irrelevant whether it's poison or UB as the assumption in 1) holds.
If inlining happens, the return may be replaced with the argument. That's ok as we are then refining poison into a concrete value.

In summary, yielding poison when nocapture is violated seems sufficient.