[PM][FunctionAttrs] add NoUnwind attribute inference to PostOrderFunctionAttrs pass

[fedor.sergeev]

Summary

This was motivated by absence of PrunEH functionality in new PM.
It was decided that a proper way to do PruneEH is to add NoUnwind inference
into PostOrderFunctionAttrs and then perform normal SimplifyCFG on top.

This change generalizes attribute handling implemented for (a removal of)
Convergent attribute, by introducing a generic builder-like class

AttributeInferer

It registers all the attribute inference requests, storing per-attribute
predicates into a vector, and then goes through an SCC Node, scanning all
the instructions for not breaking attribute assumptions.

The main idea is that as soon all the instructions from all the functions
of SCC Node conform to attribute assumptions then we are free to infer
the attribute as set for all the functions of SCC Node.

It handles two distinct cases of attributes:

• those that might break due to derefinement of the function code
  
  for these attributes we are allowed to apply inference only if all the functions are "exact definitions". Example - NoUnwind.
• those that do not care about derefinement
  
  for these attributes we are allowed to apply inference as soon as we see any function definition. Example - removal of Convergent attribute.

Also in this commit:

• Converted all the FunctionAttrs tests to use FileCheck and added new-PM invocations to them

• FunctionAttrs/convergent.ll test demonstrates a difference in behavior between new and old PM implementations. Marked with FIXME.

• PruneEH tests were converted to new-PM as well, using function-attrs+simplify-cfg combo as intended

• some of "other" tests were updated since function-attrs now infers 'nounwind' even for old PM pipeline

• -disable-nounwind-inference hidden option added as a possible workaround for a supposedly rare case when nounwind being inferred by default presents a problem

[fedor.sergeev]

cleaning up some tests, still a couple FIXMEs in tests

[fedor.sergeev]

all the tests updated, one FIXME left on convergent.ll test as intended

[jlebar]

Hi, thanks for the patch.

I like the overall approach of making this pass generic. But I have some high-level comments on the patch:

• Is it possible to explain in the code itself everything a user needs to know to understand the code? For example, I could not find where we define "derefinable", or derefinement. More broadly, it is not easy for me to follow the overall structure of this code just by reading the code.

• It's a nit, but I would prefer to write all comments in proper English, so starting with capital letters, ending with periods (if they are complete sentences), using apostrophes in contractions (e.g. cant -> can't), etc. I think this will make it easier for others to read.

• I'm somewhat confused by our notion of some instructions being "invalid". Perhaps that should have a different name.

• I'm a bit concerned about the overhead of calling multiple virtual functions (std::function) for every instruction in the module, especially since our loop structure (which is the correct one!) does not have one easily-predicted virtual function call target, but instead iterates between call targets. I know we go through a lot of pain elsewhere in LLVM to avoid this. Since the list of functors is static, I wonder if we shouldn't metaprogram this. Using llvm::integer_sequence would make it not *too* bad.
  
  But maybe that's a premature optimization; I definitely don't want to do it if there's no benefit. Perhaps @chandlerc can comment on this.

[chandlerc]

Sorry for delay sending these commetns -- i had them typed up and didn't mash send. =[[[

Also, to address (briefly) what Justin mentioned: I think getting cache locality of single visit of instructions is likely to be more valuable than the overhead of the indirect calls. Could be wrong, but we can also look to see if there is a problem.

lib/Transforms/IPO/FunctionAttrs.cpp

1048–1049

This should be a (more expansive I suspect) doxygen comment on the type.

1050–1055

The tuple here should almost certainly be a little struct to simplify the code. At that point, I don't know that you need the type aliases.

1060–1066

I think this should be named more along the lines of registering something as it doesn't actually do inference here.

Currently, the doxygen comment doesn't add much value. Instead, I would suggest that the doxygen comment should explain what the semantics of these predicates are, how they are used, and mention what is different about the derefinement.

1081

This routine is only going to be run once over an SCC. I think that suggests a much simpler implementation.

As we process the SCC nodes and the instructions within each, we can use a remove_if (or better erase_if) pattern so that as predicates fail, we simple remove the struct w/ the callbacks for that attribute.

Then at the end, we run all the callbacks that remain.

This should allow you to not have index based walks or the bit vectors.

[jlebar]

Also, to address (briefly) what Justin mentioned: I think getting cache locality of single visit of instructions is likely to be more valuable than the overhead of the indirect calls. Could be wrong, but we can also look to see if there is a problem.

Totally agree, but I think I wasn't clear, that's a different comparison from the one I was trying to make. The question I was trying ask is, given the current loop structure (which I think you're right, is the right structure), is the overhead of indirect calls vs direct (possibly inlined) calls (generated via metaprogramming) significant?

[chandlerc]

In D44415#1036911, @jlebar wrote:

Also, to address (briefly) what Justin mentioned: I think getting cache locality of single visit of instructions is likely to be more valuable than the overhead of the indirect calls. Could be wrong, but we can also look to see if there is a problem.

Totally agree, but I think I wasn't clear, that's a different comparison from the one I was trying to make. The question I was trying ask is, given the current loop structure (which I think you're right, is the right structure), is the overhead of indirect calls vs direct (possibly inlined) calls (generated via metaprogramming) significant?

Probably not? But maybe?

That said, generating direct calls here seems really, really hard... Maybe you have an idea I don't see...

The only way I see to make them not indirect, we would need to have heterogeneous collection of callbacks for the various attributes and iterate across it heterogeneously.... Essentially, we'd need to take each collection of callbacks in the constructor so we can deduce all of their types and the number of them in one go, build a tuple of them, and then have the 'run' thing actually use tuple-iteration (maybe w/ a variadic function template) instead of a loop. The complexity of that code just didn't seem worth it w/o a benchmark...

[jlebar]

generating direct calls here seems really, really hard... Maybe you have an idea I don't see...

It's...not awful. https://godbolt.org/g/D3KMQH

#include <array>
#include <vector>
#include <utility>

struct Instruction;

struct FooDescriptor {
  static bool IsGood(Instruction* I) { return true; }
};

struct BarDescriptor {
  static bool IsGood(Instruction* I) { return false; }
};

template <typename... Descriptors>
struct AttrInferer {
    void Foo(std::vector<Instruction*> Instrs) {
        for (Instruction* Instr : Instrs) {
            bool IsGood[] = {
                [&] { return Descriptors::IsGood(Instr); }()...
            };
            for (int i = 0; i < sizeof...(Descriptors); ++i) {
                
            }
        }
    }
};

void Test() {
    AttrInferer<FooDescriptor, BarDescriptor> a;
    a.Foo({});
}

Anyway if you think the virtual functions aren't a big deal, that's good enough for me.

[fedor.sergeev]

Thanks everybody for good comments, will see into them.
Will put virtual functions issue at the very end of the queue.

In D44415#1036829, @jlebar wrote:

• Is it possible to explain in the code itself everything a user needs to know to understand the code? For example, I could not find where we define "derefinable", or derefinement.

I'm not sure about proper usage of this term either. It came from my discussions with Chandler, but if it sounds confusing ....
Anyway, I tried to explain the term in my commit message.

More broadly, it is not easy for me to follow the overall structure of this code just by reading the code.

You mean overall structure of AttributeInferer::run?

• It's a nit, but I would prefer to write all comments in proper English, so starting with capital letters, ending with periods

Yeah, it is my biggest common mistake with the style :-/

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1081

I can use remove/erase for the purpose of "Valid" attributes tracking.
However I still need a couple of deducible boolean facts separately tracked for each attribute -"ScanHere" and "Ready".
I can try adding those into Predicates structure and clean/set them accordingly.

Lets see how it looks like in implementation suggested by Justin (which, btw, does have an index walk ;) ).

[fedor.sergeev]

fixing comments, adding struct instead of tuple.
Addressed pretty much everything except of bitvectors and virtual functions,
just to have some ready base for comparison with other solutions.

[jlebar]

I like it.

I have a suggestions on the comments, but that's...pretty normal for me. :)

I think that if we don't go with the variadic templates thing -- which is probably a premature optimization, I trust Chandler here -- then going with Chandler's suggestion and getting rid of most or all of the bitsets would be a good simplification.

lib/Transforms/IPO/FunctionAttrs.cpp

1056

Can we expand upon what we mean by an "exact" definition?

1059

Should we say this function can be null (and if so, all functions need to be scanned)?

1069

Perhaps a different name for this vector, now that the struct has a name.

1116

This is kind of confusing right above the auto &AP = line, because it's not really describing that line...

Perhaps we could have a comment on the loop explaining at a higher level what we're doing?

1120

s/Perhaps,/Perhaps/

1122

This comment is probably obvious from the line above now.

1123

else if (...) {
}

?

1126

can't

1195

We should indicate somewhere (on the struct definition?) that if a function is skipped, it's skipped from both scanning *and* from having the attribute set on it. That was surprising to me.

1200

Perhaps instead of introducing this new "invalid" terminology, we should call it something related to one of the names in InferenceDescriptor?

1210

Perhaps this function would be clearer as:

if (I.mayThrow()) return false;
if (...) {
  if (SCCNodes.count(Callee)) {
     // I is a may-throw call to a function inside our SCC.  This doesn't invalidate our working assumption that the SCC is no-throw; we have to scan that other function.
    return false;
  }
}
// This instruction may throw, and it's not a call to another function in our SCC.  It therefore invalidates our non-throwing assumption.
return true;

1241

Suggest /*RequiresExactDefinition=*/false this way it's not ambiguous whether we're saying that false means it does require an exact definition, or that it doesn't.

1256

s/Note, that/Note that/

1271

Please re-clang-format

[fedor.sergeev]

addressing most of Justin's comments on comments ;)

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1056

I'm not sure how can I do that w/o duplicating a huge comment for GlobalValue::isDefinitionExact.

1059

SkipFunc is not intended to be null. Comment added below to clarify the intended use.

1271

Err... clang-format behaves badly on this :(
It formats two calls of registerAttrInference differently.
So I took one format as is and hand-edited another invocation correspondingly.
Will repeat the same before integration as well.

[fedor.sergeev]

Btw, I tried Justin's version of "simplified" meta-programmed callbacks... and fully functional version of it
(one that performs early bailouts on ScanAttrHere/ValidAttrs) starts approaching Chandler's suggested tuple/tuple-iteration by complexity.
The problem with bailouts is that you cant just fill the whole array with calculated values of all the predicates,
or else you need to extend all your predicates with shortcut-ting semantics.

[fedor.sergeev]

getting rid of bitvectors

[fedor.sergeev]

making InstrBreaksNonThrowing a bit more readable, introducing -disable-nounwind-inference option just in case

[fedor.sergeev]

just to clarify - I believe this is ready to go :)

[jlebar]

lib/Transforms/IPO/FunctionAttrs.cpp

1056

Could we just have a pointer to GlobalValue::isDefinitionExact? As in

If true, only "exact" definitions can be used to infer this attribute.  (See GlobalValue::isDefinitionExact.)

1072

If we're going to mix both "properties" of the attribute and "state" of the algorithm in the same class, can we at least have a comment indicating that this is what we're doing, and all variables below this point are mutable state used by the algorithm?

1073

Capital

1073

s/still hold valid/are still valid/

1080

Capital

1082

I don't know how Chandler feels about it, but personally I'm not wild about the approach of sticking state into this struct. It uses the descriptor class for two quite different purposes, and perhaps worse, it's really easy to have a bug where we forget to reset ScanCurrent at the top of the loop...

I *think* Chandler's suggestion was, instead of tracking the state with a bitset, track it by removing elements from a list (or adding them to a list of "attributes to use when scanning this function").

It looks like the difficulty you face here is that the list you want to remove elements from is also the list you're iterating over? I'd be OK with pretty much any of the obvious solutions to that -- remove and adjust the loop index, use an std::list and remove in place, remove after the loop completes...

Dunno if Chandler has feels about this too.

1271

So I took one format as is and hand-edited another invocation correspondingly.

That's fine with me, so long as we don't expect that future modifications to this part of the file will preserve the format. People should be able to clang-format their changes and expect it's "good enough".

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1082

Well, I dont like merging descriptor with state either.
In my original implementation I had those in separate vectors, which forced me to use indices as the way to link descriptors and states.

Chandler's suggestion indeed was to remove elements.
However it does not play well with the need to have as many early exits as possible (which was a property of implementation before my changes).
Namely, there is a single meaningful vector here, a vector of descriptors, and while I can use removal from it to signify a single binary fact (e.g. Valid), there are more facts to track. Should I introduce copies of the main vector? How would it be different from having bit-vectors?

I really do not see an ideal solution here.

I can introduce a vector< pair<descriptor,state> >, which is rather clumsy.
Or I can introduce a more complicated interface to the descriptor class, with read/write accessors as needed,
and service functions that do initialization of mutables as needed.

1271

Most recent version is clang-formatted. This particular problem was solved by reordering arguments of InferenceDescriptor constructor.

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1080

I wonder if capitalizing "true" which is a boolean value and not an English word here is fine?

[jlebar]

lib/Transforms/IPO/FunctionAttrs.cpp

1080

I think of the boolean as *being* true, which in C++ might mean it has value true, in Python might mean it has value True, in PHP might mean it has value "True" :)... That is, I think you can say "this Python variable is true" with a lower-case "t", and that's just fine.

But that's quite a nit, whatever you like is fine. :)

1082

Should I introduce copies of the main vector?

That's what I was thinking.

How would it be different from having bit-vectors?

I think the notion was that you'd iterate over the vector that's being modified, so there's less indirection in the code.

In pseudocode:

ScannedSomeFunction = false;

SCCDescriptors = InterfaceDescriptors;
for (F : SCCNodes) {
  FnDescriptors = []
  for (Desc : SCCDescriptors) {
    if (F->isDeclaration() || (ID.RequiresExactDefinition && !F->hasExactDefinition())) {
      SCCDescriptors.Remove(Desc);
    } else if (!Desc.SkipFunction(F)) {
      FnDescriptors.Append(F);
    }
  }

  for (I : instructions(F)) {
    if (FnDescriptors.empty())
      break;

    for (Desc : FnDescriptors) {
      if (Desc.InstrBreaksAttribute(I)) {
        FnDescriptors.Remove(Desc);
	SCCDescriptors.Remove(Desc);
      }
    }
  }

  for (Desc : SCCDescriptors) {
    if (Desc.SkipFunction(F))
      continue;
    Desc.SetAttribute(F)
  }
}

I don't have SuccessfullyScanned here, but I *think* it's actually not necessary, because SkipFunction is the only reason we wouldn't scan something, and if we skipped it, we also won't set the attr on it.

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1082

Ahem... that should work, indeed. Lemme try it.

[fedor.sergeev]

minor cleanup first

[fedor.sergeev]

trying with erase_if

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1082

Done more or less along the lines of your suggestion, Justin (applying Chandler's advice about using erase_if).
The only catch is that I had to add AttrKind to the descriptor to be able to match descriptors in different vectors
(to implement your : FnDescriptors.Remove + SCCDescriptors.Remove).

[jlebar]

I like the algorithm! Just comments on comments, then I think I'm happy. Dunno if @chandlerc wants to have a look or not...

lib/Transforms/IPO/FunctionAttrs.cpp

1054

Remove newline?

1057

Update comment?

1076

s/)/.)/

1103

Remove newline?

1106

... "for each of them. We'll remove attributes which aren't valid for the SCC from InferInSCC."

1116

This comment may be more confusing than not at this point.

1118

s/meeting/visiting/ (idom, sorry)

1119

has an unsuitable definition

1120

Nit, remove outer parens?

1126

Suggest moving the comment up one line and saying something like

For each attribute still in InferInSCC that doesn't skip F, check that the instructions in F are valid.

1136

Remove newline?

1140

Perhaps this can get a "because" -- it's kind of the crux of the whole algorithm, and it's quite hidden here.

1149

If you move this check to the top of the loop, then you don't need the check before the loop.

[fedor.sergeev]

addressing Justin's comments

[fedor.sergeev]

more comments cleanup

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1149

Perhaps its a bit of over-optimization, but I dont want to touch instructions(F) (enter the instructions loop) just to determine that we need to break out.
I dont feel that having these two checks is detrimental to readability.

[jlebar]

Hey, I can almost sign off on this, but two of the comments are not clear. I'm happy to provide suggestions if you like.

lib/Transforms/IPO/FunctionAttrs.cpp

1126

Sorry, I think the reworded comment is still unclear.

If you're willing to tell me what you don't like about the suggestion I made earlier, perhaps we can come up with something we both like.

1141

This comment needs to be reworded, sorry.

[fedor.sergeev]

minor comments cleanup

[fedor.sergeev]

lib/Transforms/IPO/FunctionAttrs.cpp

1126

Mostly I didnt like "instructions are valid" part, which is not what we are checking for.
If you dont like my last variant - please, suggest your own that talks about attribute, not instruction.

[jlebar]

\o/

lib/Transforms/IPO/FunctionAttrs.cpp

1126

What you came up with looks great to me.

Thank you for being patient with me -- I know I'm not an easy reviewer.

1140

lgtm, thanks again for being patient with me.

[fedor.sergeev]

Thanks for following up till the very end of it! :)
Your feeling of language is a bit over my capabilities, but I do like to learn here.

[fedor.sergeev]

minor semantical update - calculation of return value corrected for AttributeInferer::run,
returning true only when real changes are detected.
Fixes regression introduced by most recent algorithmic update.

lib/Transforms/IPO/FunctionAttrs.cpp

 STATISTIC(NumReadOnly, "Number of functions marked readonly");
 STATISTIC(NumNoCapture, "Number of arguments marked nocapture");
 STATISTIC(NumReturned, "Number of arguments marked returned");
 STATISTIC(NumReadNoneArg, "Number of arguments marked readnone");
 STATISTIC(NumReadOnlyArg, "Number of arguments marked readonly");
 STATISTIC(NumNoAlias, "Number of function returns marked noalias");
 STATISTIC(NumNonNullReturn, "Number of function returns marked nonnull");
 STATISTIC(NumNoRecurse, "Number of functions marked as norecurse");
+STATISTIC(NumNoUnwind, "Number of functions marked as nounwind");
 
 // FIXME: This is disabled by default to avoid exposing security vulnerabilities
 // in C/C++ code compiled by clang:
 // http://lists.llvm.org/pipermail/cfe-dev/2017-January/052066.html
 static cl::opt<bool> EnableNonnullArgPropagation(
     "enable-nonnull-arg-prop", cl::Hidden,
     cl::desc("Try to propagate nonnull argument attributes from callsites to "
              "caller functions."));
 
+static cl::opt<bool> DisableNoUnwindInference(
+    "disable-nounwind-inference", cl::Hidden,
+    cl::desc("Stop inferring nounwind attribute during function-attrs pass"));
+
 namespace {
 
 using SCCNodeSet = SmallSetVector<Function *, 8>;
 
 } // end anonymous namespace
 
 /// Returns the memory access attribute for function F using AAR for AA results,
 /// where SCCNodes is the current SCC.
for (Function *F : SCCNodes) {
       ++NumNonNullReturn;
       MadeChange = true;
     }
   }
 
   return MadeChange;
 }
 
-/// Remove the convergent attribute from all functions in the SCC if every
-/// callsite within the SCC is not convergent (except for calls to functions
-/// within the SCC).  Returns true if changes were made.
-static bool removeConvergentAttrs(const SCCNodeSet &SCCNodes) {
-  // For every function in SCC, ensure that either
-  //  * it is not convergent, or
-  //  * we can remove its convergent attribute.
-  bool HasConvergentFn = false;
+namespace {
+
+/// Collects a set of attribute inference requests and performs them all in one
+/// go on a single SCC Node. Inference involves scanning function bodies
+/// looking for instructions that violate attribute assumptions.
+/// As soon as all the bodies are fine we are free to set the attribute.
+/// Customization of inference for individual attributes is performed by
+/// providing a handful of predicates for each attribute.
+class AttributeInferer {
+
+public:
+  /// Describes a request for inference of a single attribute,
+  /// as well as tracks the state of inference.
+  struct InferenceDescriptor {
+
+    /// Returns true if this function does not have to be handled.
+    /// General intent for this predicate is to provide an optimization
+    /// for functions that do not need this attribute inference at all
+    /// (say, for functions that already have the attribute).
+    std::function<bool(const Function &)> SkipFunction;
+
+    /// Returns true if this instruction violates attribute assumptions.
+    std::function<bool(Instruction &)> InstrBreaksAttribute;
+
+    /// Sets the inferred attribute for this function.
+    std::function<void(Function &)> SetAttribute;
+
+    /// Attribute we derive.
+    Attribute::AttrKind AKind;
+
+    /// If true, only "exact" definitions can be used to infer this attribute.
+    /// (See GlobalValue::isDefinitionExact)
+    bool RequiresExactDefinition;
+
+    InferenceDescriptor(Attribute::AttrKind AK,
+                        std::function<bool(const Function &)> SkipFunc,
+                        std::function<bool(Instruction &)> InstrScan,
+                        std::function<void(Function &)> SetAttr,
+                        bool ReqExactDef)
+        : SkipFunction(SkipFunc), InstrBreaksAttribute(InstrScan),
+          SetAttribute(SetAttr), AKind(AK),
+          RequiresExactDefinition(ReqExactDef) {}
+  };
+
+private:
+  SmallVector<InferenceDescriptor, 4> InferenceDescriptors;
+
+public:
+  void registerAttrInference(InferenceDescriptor AttrInference) {
+    InferenceDescriptors.push_back(AttrInference);
+  }
+
+  bool run(const SCCNodeSet &SCCNodes);
+};
+
+/// Perform all the requested attribute inference actions according to the
+/// attribute predicates stored before.
+bool AttributeInferer::run(const SCCNodeSet &SCCNodes) {
+  SmallVector<InferenceDescriptor, 4> InferInSCC = InferenceDescriptors;
+  // Go through all the functions in SCC and check corresponding attribute
+  // assumptions for each of them. Attributes that are invalid for this SCC
+  // will be removed from InferInSCC.
   for (Function *F : SCCNodes) {
-    if (!F->isConvergent()) continue;
-    HasConvergentFn = true;
 
-    // Can't remove convergent from function declarations.
-    if (F->isDeclaration()) return false;
+    // No attributes whose assumptions are still valid - done.
+    if (InferInSCC.empty())
+      return false;
+
+    // Check if our attributes ever need scanning/can be scanned.
+    llvm::erase_if(InferInSCC, [F](const InferenceDescriptor &ID) {
+      if (ID.SkipFunction(*F))
+        return false;
+
+      // Remove from further inference (invalidate) when visiting a function
+      // that has no instructions to scan/has an unsuitable definition.
+      return F->isDeclaration() ||
+             (ID.RequiresExactDefinition && !F->hasExactDefinition());
+    });
+
+    // For all the attributes in InferInSCC which do not explicitly skip F
+    // we will continue inference by scanning all the instructions in
+    // this function.
+    SmallVector<InferenceDescriptor, 4> InferInThisFunc;
+    llvm::copy_if(
+        InferInSCC, std::back_inserter(InferInThisFunc),
+        [F](const InferenceDescriptor &ID) { return !ID.SkipFunction(*F); });
 
-    // Can't remove convergent if any of our functions has a convergent call to a
-    // function not in the SCC.
+    if (InferInThisFunc.empty())
+      continue;
+
+    // Start instruction scan.
     for (Instruction &I : instructions(*F)) {
-      CallSite CS(&I);
-      // Bail if CS is a convergent call to a function not in the SCC.
-      if (CS && CS.isConvergent() &&
-          SCCNodes.count(CS.getCalledFunction()) == 0)
+      llvm::erase_if(InferInThisFunc, [&](const InferenceDescriptor &ID) {
+        if (!ID.InstrBreaksAttribute(I))
           return false;
+        // Remove attribute from inference on other functions (invalidate)
+        // since we have attribute assumptions already violated.
+        llvm::erase_if(InferInSCC, [&ID](const InferenceDescriptor &D) {
+          return D.AKind == ID.AKind;
+        });
+        // Remove attribute from the rest of current scan.
+        return true;
+      });
+
+      if (InferInThisFunc.empty())
+        break;
     }
   }
 
-  // If the SCC doesn't have any convergent functions, we have nothing to do.
-  if (!HasConvergentFn) return false;
+  if (InferInSCC.empty())
+    return false;
 
-  // If we got here, all of the calls the SCC makes to functions not in the SCC
-  // are non-convergent.  Therefore all of the SCC's functions can also be made
-  // non-convergent.  We'll remove the attr from the callsites in
-  // InstCombineCalls.
-  for (Function *F : SCCNodes) {
-    if (!F->isConvergent()) continue;
+  for (Function *F : SCCNodes)
+    // At this point InferInSCC contains only functions that were either:
+    //   - explicitly skipped from scan/inference, or
+    //   - verified to have no instructions that break attribute assumptions.
+    // Hence we just go and force the attribute for all non-skipped functions.
+    for (auto &ID : InferInSCC) {
+      if (ID.SkipFunction(*F))
+        continue;
 
-    DEBUG(dbgs() << "Removing convergent attr from fn " << F->getName()
-                 << "\n");
-    F->setNotConvergent();
+      ID.SetAttribute(*F);
+    }
+  return true;
+}
+
+} // end anonymous namespace
+
+/// Helper for non-Convergent inference predicate InstrBreaksAttribute.
+static bool InstrBreaksNonConvergent(Instruction &I,
+                                     const SCCNodeSet &SCCNodes) {
+  const CallSite CS(&I);
+  // Breaks non-convergent assumption if CS is a convergent call to a function
+  // not in the SCC.
+  return CS && CS.isConvergent() && SCCNodes.count(CS.getCalledFunction()) == 0;
+}
+
+/// Helper for NoUnwind inference predicate InstrBreaksAttribute.
+static bool InstrBreaksNonThrowing(Instruction &I, const SCCNodeSet &SCCNodes) {
+  if (!I.mayThrow())
+    return false;
+  if (const auto *CI = dyn_cast<CallInst>(&I)) {
+    if (Function *Callee = CI->getCalledFunction()) {
+      // I is a may-throw call to a function inside our SCC. This doesn't
+      // invalidate our current working assumption that the SCC is no-throw; we
+      // just have to scan that other function.
+      if (SCCNodes.count(Callee) > 0)
+        return false;
+    }
   }
   return true;
 }
 
+/// Infer attributes from all functions in the SCC by scanning every
+/// instruction for compliance to the attribute assumptions. Currently it
+/// does:
+///   - removal of Convergent attribute
+///   - addition of NoUnwind attribute
+///
+/// Returns true if any changes to function attributes were made.
+static bool inferAttrsFromFunctionBodies(const SCCNodeSet &SCCNodes) {
+
+  AttributeInferer AI;
+
+  // Request to remove the convergent attribute from all functions in the SCC
+  // if every callsite within the SCC is not convergent (except for calls
+  // to functions within the SCC).
+  // Note: Removal of the attr from the callsites will happen in
+  // InstCombineCalls separately.
+  AI.registerAttrInference(AttributeInferer::InferenceDescriptor{
+      Attribute::Convergent,
+      // Skip non-convergent functions.
+      [](const Function &F) { return !F.isConvergent(); },
+      // Instructions that break non-convergent assumption.
+      [SCCNodes](Instruction &I) {
+        return InstrBreaksNonConvergent(I, SCCNodes);
+      },
+      [](Function &F) {
+        DEBUG(dbgs() << "Removing convergent attr from fn " << F.getName()
+                     << "\n");
+        F.setNotConvergent();
+      },
+      /* RequiresExactDefinition= */ false});
+
+  if (!DisableNoUnwindInference)
+    // Request to infer nounwind attribute for all the functions in the SCC if
+    // every callsite within the SCC is not throwing (except for calls to
+    // functions within the SCC). Note that nounwind attribute suffers from
+    // derefinement - results may change depending on how functions are
+    // optimized. Thus it can be inferred only from exact definitions.
+    AI.registerAttrInference(AttributeInferer::InferenceDescriptor{
+        Attribute::NoUnwind,
+        // Skip non-throwing functions.
+        [](const Function &F) { return F.doesNotThrow(); },
+        // Instructions that break non-throwing assumption.
+        [SCCNodes](Instruction &I) {
+          return InstrBreaksNonThrowing(I, SCCNodes);
+        },
+        [](Function &F) {
+          DEBUG(dbgs() << "Adding nounwind attr to fn " << F.getName() << "\n");
+          F.setDoesNotThrow();
+          ++NumNoUnwind;
+        },
+        /* RequiresExactDefinition= */ true});
+
+  // Perform all the requested attribute inference actions.
+  return AI.run(SCCNodes);
+}
+
 static bool setDoesNotRecurse(Function &F) {
   if (F.doesNotRecurse())
     return false;
   F.setDoesNotRecurse();
   ++NumNoRecurse;
   return true;
 }
 
 static bool addNoRecurseAttrs(const SCCNodeSet &SCCNodes) {
   // Try and identify functions that do not recurse.
 
   // If the SCC contains multiple nodes we know for sure there is recursion.
   if (SCCNodes.size() != 1)
     return false;
 
   Function *F = *SCCNodes.begin();
   if (!F || F->isDeclaration() || F->doesNotRecurse())
     return false;
 
   // If all of the calls in F are identifiable and are to norecurse functions, F
   // is norecurse. This check also detects self-recursion as F is not currently
   // marked norecurse, so any called from F to F will not be marked norecurse.
   for (Instruction &I : instructions(*F))
     if (auto CS = CallSite(&I)) {
       Function *Callee = CS.getCalledFunction();
       if (!Callee || Callee == F || !Callee->doesNotRecurse())
PreservedAnalyses PostOrderFunctionAttrsPass::run(LazyCallGraph::SCC &C,
   Changed |= addReadAttrs(SCCNodes, AARGetter);
   Changed |= addArgumentAttrs(SCCNodes);
 
   // If we have no external nodes participating in the SCC, we can deduce some
   // more precise attributes as well.
   if (!HasUnknownCall) {
     Changed |= addNoAliasAttrs(SCCNodes);
     Changed |= addNonNullAttrs(SCCNodes);
-    Changed |= removeConvergentAttrs(SCCNodes);
+    Changed |= inferAttrsFromFunctionBodies(SCCNodes);
     Changed |= addNoRecurseAttrs(SCCNodes);
   }
 
   return Changed ? PreservedAnalyses::none() : PreservedAnalyses::all();
 }
 
 namespace {
 
static bool runImpl(CallGraphSCC &SCC, AARGetterT AARGetter) {
   Changed |= addReadAttrs(SCCNodes, AARGetter);
   Changed |= addArgumentAttrs(SCCNodes);
 
   // If we have no external nodes participating in the SCC, we can deduce some
   // more precise attributes as well.
   if (!ExternalNode) {
     Changed |= addNoAliasAttrs(SCCNodes);
     Changed |= addNonNullAttrs(SCCNodes);
-    Changed |= removeConvergentAttrs(SCCNodes);
+    Changed |= inferAttrsFromFunctionBodies(SCCNodes);
     Changed |= addNoRecurseAttrs(SCCNodes);
   }
 
   return Changed;
 }
 
 bool PostOrderFunctionAttrsLegacyPass::runOnSCC(CallGraphSCC &SCC) {
   if (skipSCC(SCC))

test/Other/cgscc-devirt-iteration.ll

 ; The CGSCC pass manager includes an SCC iteration utility that tracks indirect
 ; calls that are turned into direct calls (devirtualization) and re-visits the
 ; SCC to expose those calls to the SCC-based IPO passes. We trigger
 ; devirtualization here with GVN which forwards a store through a load and to
 ; an indirect call.
 ;
 ; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(function-attrs,function(gvn,instcombine))' -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=BEFORE
 ; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(devirt<1>(function-attrs,function(gvn,instcombine)))' -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER --check-prefix=AFTER1
 ; RUN: opt -aa-pipeline=basic-aa -passes='cgscc(devirt<2>(function-attrs,function(gvn,instcombine)))' -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER --check-prefix=AFTER2
 ;
 ; We also verify that the real O2 pipeline catches these cases.
 ; RUN: opt -aa-pipeline=basic-aa -passes='default<O2>' -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER --check-prefix=AFTER2
 
 declare void @readnone() readnone
 ; CHECK: Function Attrs: readnone
-; CHECK: declare void @readnone()
+; CHECK-NEXT: declare void @readnone()
 
 declare void @unknown()
 ; CHECK-NOT: Function Attrs
-; CHECK: declare void @unknown()
+; CHECK-LABEL: declare void @unknown(){{ *$}}
 
 ; The @test1 function checks that when we refine an indirect call to a direct
 ; call we revisit the SCC passes to reflect the more precise information. This
 ; is the basic functionality.
 
 define void @test1() {
 ; BEFORE-NOT: Function Attrs
 ; AFTER: Function Attrs: readnone
-; CHECK: define void @test1()
+; CHECK-LABEL: define void @test1()
 entry:
   %fptr = alloca void ()*
   store void ()* @readnone, void ()** %fptr
   %f = load void ()*, void ()** %fptr
   call void %f()
   ret void
 }
 
 ; The @test2_* functions check that when we need multiple (in this case 2)
 ; repetitions to compute some state that is incrementally exposed with each
 ; one, the limit on repetitions is enforced. So we make progress with
 ; one repetition but not as much as with three.
 ;
 ; This is somewhat awkward to test because we have to contrive to have a state
 ; repetition triggered and observed with very few passes. The technique here
 ; is to have one indirect call that can only be resolved when the entire SCC is
 ; deduced as readonly, and mark that indirect call at the call site as readonly
 ; to make that possible. This forces us to first deduce readonly, then
 ; devirtualize again, and then deduce readnone.
 
 declare void @readnone_with_arg(void ()**) readnone
 ; CHECK: Function Attrs: readnone
-; CHECK: declare void @readnone_with_arg(void ()**)
+; CHECK-LABEL: declare void @readnone_with_arg(void ()**)
 
 define void @test2_a(void ()** %ignore) {
 ; BEFORE-NOT: Function Attrs
 ; AFTER1: Function Attrs: readonly
 ; AFTER2: Function Attrs: readnone
 ; BEFORE: define void @test2_a(void ()** %ignore)
 ; AFTER: define void @test2_a(void ()** readnone %ignore)
 entry:
; CHECK: call void @readnone_with_arg(void ()** %ignore)
 
   ret void
 }
 
 define void @test2_b() {
 ; BEFORE-NOT: Function Attrs
 ; AFTER1: Function Attrs: readonly
 ; AFTER2: Function Attrs: readnone
-; CHECK: define void @test2_b()
+; CHECK-LABEL: define void @test2_b()
 entry:
   %f2ptr = alloca void ()*
   store void ()* @readnone, void ()** %f2ptr
   ; Call the other function here to prevent forwarding until the SCC has had
   ; function attrs deduced.
   call void @test2_a(void ()** %f2ptr)
 
   %f2 = load void ()*, void ()** %f2ptr
   ; This is the second indirect call to be resolved, and can only be resolved
   ; after we deduce 'readonly' for the rest of the SCC. Once it is
   ; devirtualized, we can deduce readnone for the SCC.
   call void %f2() readonly
 ; BEFORE: call void %f2()
 ; AFTER: call void @readnone()
 
   ret void
 }
 
 declare i8* @memcpy(i8*, i8*, i64)
-; CHECK: declare i8* @memcpy(
+; CHECK-LABEL: declare i8* @memcpy(
 
 ; The @test3 function checks that when we refine an indirect call to an
 ; intrinsic we still revisit the SCC pass. This also covers cases where the
 ; value handle itself doesn't persist due to the nature of how instcombine
 ; creates the memcpy intrinsic call, and we rely on the count of indirect calls
 ; decreasing and the count of direct calls increasing.
-define void @test3(i8* %src, i8* %dest, i64 %size) {
-; CHECK-NOT: Function Attrs
-; BEFORE: define void @test3(i8* %src, i8* %dest, i64 %size)
-; AFTER: define void @test3(i8* nocapture readonly %src, i8* nocapture %dest, i64 %size)
+; Adding 'noinline' attribute to force attributes for improved matching.
+define void @test3(i8* %src, i8* %dest, i64 %size) noinline {
+; CHECK: Function Attrs
+; CHECK-NOT: read
+; CHECK-SAME: noinline
+; BEFORE-LABEL: define void @test3(i8* %src, i8* %dest, i64 %size)
+; AFTER-LABEL: define void @test3(i8* nocapture readonly %src, i8* nocapture %dest, i64 %size)
   %fptr = alloca i8* (i8*, i8*, i64)*
   store i8* (i8*, i8*, i64)* @memcpy, i8* (i8*, i8*, i64)** %fptr
   %f = load i8* (i8*, i8*, i64)*, i8* (i8*, i8*, i64)** %fptr
   call i8* %f(i8* %dest, i8* %src, i64 %size)
 ; CHECK: call void @llvm.memcpy
   ret void
 }
 
 ; A boring function that just keeps our declarations around.
 define void @keep(i8** %sink) {
 ; CHECK-NOT: Function Attrs
-; CHECK: define void @keep(
+; CHECK-LABEL: define void @keep(
 entry:
   store volatile i8* bitcast (void ()* @readnone to i8*), i8** %sink
   store volatile i8* bitcast (void ()* @unknown to i8*), i8** %sink
   store volatile i8* bitcast (i8* (i8*, i8*, i64)* @memcpy to i8*), i8** %sink
   call void @unknown()
   ret void
 }

test/Transforms/FunctionAttrs/2008-09-03-Mutual.ll

-; RUN: opt < %s -functionattrs -S | grep readnone
+; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NEXT: define i32 @a
 define i32 @a() {
   %tmp = call i32 @b( )   ; <i32> [#uses=1]
   ret i32 %tmp
 }
 
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NEXT: define i32 @b
 define i32 @b() {
   %tmp = call i32 @a( )   ; <i32> [#uses=1]
   ret i32 %tmp
 }

test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll

 ; RUN: opt < %s -basicaa -functionattrs -S | FileCheck %s
+; RUN: opt < %s -aa-pipeline=basic-aa -passes=function-attrs -S | FileCheck %s
+
 @x = global i32 0
 
-; CHECK: declare i32 @e() #0
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NEXT: declare i32 @e
 declare i32 @e() readnone
 
-; CHECK: define i32 @f() #0
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NEXT: define i32 @f
 define i32 @f() {
   %tmp = call i32 @e( )   ; <i32> [#uses=1]
   ret i32 %tmp
 }
 
-; CHECK: define i32 @g() #1
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NEXT: define i32 @g
 define i32 @g() readonly {
   ret i32 0
 }
 
-; CHECK: define i32 @h() #1
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NEXT: define i32 @h
 define i32 @h() readnone {
   %tmp = load i32, i32* @x    ; <i32> [#uses=1]
   ret i32 %tmp
 }
-
-; CHECK: attributes #0 = { readnone }
-; CHECK: attributes #1 = { norecurse readnone }

test/Transforms/FunctionAttrs/2008-09-03-ReadOnly.ll

 ; RUN: opt < %s -basicaa -functionattrs -S | FileCheck %s
+; RUN: opt < %s -aa-pipeline=basic-aa -passes=function-attrs -S | FileCheck %s
 
 ; CHECK: define i32 @f() #0
 define i32 @f() {
 entry:
   %tmp = call i32 @e( )
   ret i32 %tmp
 }
 
 ; CHECK: declare i32 @e() #0
 declare i32 @e() readonly
 
 ; CHECK: attributes #0 = { readonly }

test/Transforms/FunctionAttrs/2008-09-13-VolatileRead.ll

-; RUN: opt < %s -functionattrs -S | not grep read
+; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 ; PR2792
 
 @g = global i32 0   ; <i32*> [#uses=1]
 
 define i32 @f() {
   %t = load volatile i32, i32* @g    ; <i32> [#uses=1]
   ret i32 %t
 }
+
+; CHECK-NOT: attributes #{{.*}} read

test/Transforms/FunctionAttrs/2008-12-29-Constant.ll

-; RUN: opt < %s -basicaa -functionattrs -S | grep readnone
+; RUN: opt < %s -basicaa -functionattrs -S | FileCheck %s
+; RUN: opt < %s -aa-pipeline=basic-aa -passes=function-attrs -S | FileCheck %s
 
 @s = external constant i8   ; <i8*> [#uses=1]
 
+; CHECK: define i8 @f() #0
 define i8 @f() {
   %tmp = load i8, i8* @s    ; <i8> [#uses=1]
   ret i8 %tmp
 }
+
+; CHECK: attributes #0 = { {{.*}} readnone

test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 
 ; CHECK: define i32* @a(i32** nocapture readonly %p)
 define i32* @a(i32** %p) {
   %tmp = load i32*, i32** %p
   ret i32* %tmp
 }
 
 ; CHECK: define i32* @b(i32* %q)

test/Transforms/FunctionAttrs/2010-10-30-volatile.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 ; PR8279
 
 @g = constant i32 1
 
+; CHECK: Function Attrs
+; CHECK-SAME: norecurse
+; CHECK-NOT: readonly
+; CHECK-NEXT: void @foo()
 define void @foo() {
-; CHECK: void @foo() #0 {
   %tmp = load volatile i32, i32* @g
   ret void
 }
-
-; CHECK: attributes #0 = { norecurse }

test/Transforms/FunctionAttrs/assume.ll

 ; RUN: opt -S -o - -functionattrs %s | FileCheck %s
+; RUN: opt -S -o - -passes=function-attrs %s | FileCheck %s
 
 ; CHECK-NOT: readnone
 declare void @llvm.assume(i1)

test/Transforms/FunctionAttrs/atomic.ll

 ; RUN: opt -basicaa -functionattrs -S < %s | FileCheck %s
+; RUN: opt -aa-pipeline=basic-aa -passes=function-attrs -S < %s | FileCheck %s
 
 ; Atomic load/store to local doesn't affect whether a function is
 ; readnone/readonly.
 define i32 @test1(i32 %x) uwtable ssp {
 ; CHECK: define i32 @test1(i32 %x) #0 {
 entry:
   %x.addr = alloca i32, align 4
   store atomic i32 %x, i32* %x.addr seq_cst, align 4
   %r = load atomic i32, i32* %x.addr seq_cst, align 4
   ret i32 %r
 }
 
 ; A function with an Acquire load is not readonly.
 define i32 @test2(i32* %x) uwtable ssp {
 ; CHECK: define i32 @test2(i32* nocapture readonly %x) #1 {
 entry:
   %r = load atomic i32, i32* %x seq_cst, align 4
   ret i32 %r
 }
 
-; CHECK: attributes #0 = { norecurse readnone ssp uwtable }
-; CHECK: attributes #1 = { norecurse ssp uwtable }
+; CHECK: attributes #0 = { norecurse nounwind readnone ssp uwtable }
+; CHECK: attributes #1 = { norecurse nounwind ssp uwtable }

test/Transforms/FunctionAttrs/comdat-ipo.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 
 ; See PR26774
 
 ; CHECK-LABEL: define void @bar(i8* readonly) {
 define void @bar(i8* readonly) {
   call void @foo(i8* %0)
   ret void
 }
 
 
 ; CHECK-LABEL: define linkonce_odr void @foo(i8* readonly) {
 define linkonce_odr void @foo(i8* readonly) {
   call void @bar(i8* %0)
   ret void
 }

test/Transforms/FunctionAttrs/convergent.ll

-; RUN: opt -functionattrs -S < %s | FileCheck %s
+; FIXME: convert CHECK-INDIRECT into CHECK (and remove -check-prefixes) as soon
+; FIXME: as new-pass-manager's handling of indirect_non_convergent_call is fixed
+;
+; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefixes=CHECK,CHECK-INDIRECT
+; RUN: opt -passes=function-attrs -S < %s | FileCheck %s
 
 ; CHECK: Function Attrs
 ; CHECK-NOT: convergent
 ; CHECK-NEXT: define i32 @nonleaf()
 define i32 @nonleaf() convergent {
   %a = call i32 @leaf()
   ret i32 %a
 }

 define i32 @indirect_convergent_call(i32 ()* %f) convergent {
    %a = call i32 %f() convergent
    ret i32 %a
 }
 ; Give indirect_non_convergent_call the norecurse attribute so we get a
 ; "Function Attrs" comment in the output.
 ;
 ; CHECK: Function Attrs
-; CHECK-NOT: convergent
-; CHECK-NEXT: define i32 @indirect_non_convergent_call(
+; CHECK-INDIRECT-NOT: convergent
+; CHECK-INDIRECT-NEXT: define i32 @indirect_non_convergent_call(
 define i32 @indirect_non_convergent_call(i32 ()* %f) convergent norecurse {
    %a = call i32 %f()
    ret i32 %a
 }
 
 ; CHECK: Function Attrs
 ; CHECK-SAME: convergent
 ; CHECK-NEXT: declare void @llvm.nvvm.barrier0()

test/Transforms/FunctionAttrs/int_sideeffect.ll

 ; RUN: opt -S < %s -functionattrs | FileCheck %s
+; RUN: opt -S < %s -passes=function-attrs | FileCheck %s
 
+; CHECK: Function Attrs
+; CHECK-SAME: inaccessiblememonly
+; CHECK-NEXT: declare void @llvm.sideeffect()
 declare void @llvm.sideeffect()
 
 ; Don't add readnone or similar attributes when an @llvm.sideeffect() intrinsic
 ; is present.
 
-; CHECK: define void @test() {
+; CHECK: Function Attrs
+; CHECK-NOT: readnone
+; CHECK: define void @test()
 define void @test() {
     call void @llvm.sideeffect()
     ret void
 }
 
-; CHECK: define void @loop() {
+; CHECK: Function Attrs
+; CHECK-NOT: readnone
+; CHECK: define void @loop()
 define void @loop() {
     br label %loop
 
 loop:
     call void @llvm.sideeffect()
     br label %loop
 }

test/Transforms/FunctionAttrs/nocapture.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
+
 @g = global i32* null   ; <i32**> [#uses=1]
 
 ; CHECK: define i32* @c1(i32* readnone returned %q)
 define i32* @c1(i32* %q) {
   ret i32* %q
 }
 
 ; CHECK: define void @c2(i32* %q)

test/Transforms/FunctionAttrs/nonnull-global.ll

 ; RUN: opt -S -functionattrs %s | FileCheck %s
+; RUN: opt -S -passes=function-attrs %s | FileCheck %s
 
 @a = external global i8, !absolute_symbol !0
 
 ; CHECK-NOT: define nonnull
 define i8* @foo() {
   ret i8* @a
 }
 
 !0 = !{i64 0, i64 256}

test/Transforms/FunctionAttrs/nonnull.ll

 ; RUN: opt -S -functionattrs -enable-nonnull-arg-prop %s | FileCheck %s
+; RUN: opt -S -passes=function-attrs -enable-nonnull-arg-prop %s | FileCheck %s
+
 declare nonnull i8* @ret_nonnull()
 
 ; Return a pointer trivially nonnull (call return attribute)
 define i8* @test1() {
 ; CHECK: define nonnull i8* @test1
   %ret = call i8* @ret_nonnull()
   ret i8* %ret
 }

test/Transforms/FunctionAttrs/norecurse.ll

 ; RUN: opt < %s -basicaa -functionattrs -rpo-functionattrs -S | FileCheck %s
 ; RUN: opt < %s -aa-pipeline=basic-aa -passes='cgscc(function-attrs),rpo-functionattrs' -S | FileCheck %s
 
-; CHECK: define i32 @leaf() #0
+; CHECK: Function Attrs
+; CHECK-SAME: norecurse nounwind readnone
+; CHECK-NEXT: define i32 @leaf()
 define i32 @leaf() {
   ret i32 1
 }
 
-; CHECK: define i32 @self_rec() #1
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NOT: norecurse
+; CHECK-NEXT: define i32 @self_rec()
 define i32 @self_rec() {
   %a = call i32 @self_rec()
   ret i32 4
 }
 
-; CHECK: define i32 @indirect_rec() #1
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NOT: norecurse
+; CHECK-NEXT: define i32 @indirect_rec()
 define i32 @indirect_rec() {
   %a = call i32 @indirect_rec2()
   ret i32 %a
 }
-; CHECK: define i32 @indirect_rec2() #1
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NOT: norecurse
+; CHECK-NEXT: define i32 @indirect_rec2()
 define i32 @indirect_rec2() {
   %a = call i32 @indirect_rec()
   ret i32 %a
 }
 
-; CHECK: define i32 @extern() #1
+; CHECK: Function Attrs
+; CHECK-SAME: readnone
+; CHECK-NOT: norecurse
+; CHECK-NEXT: define i32 @extern()
 define i32 @extern() {
   %a = call i32 @k()
   ret i32 %a
 }
+
+; CHECK: Function Attrs
+; CHECK-NEXT: declare i32 @k()
 declare i32 @k() readnone
 
-; CHECK: define void @intrinsic(i8* nocapture %dest, i8* nocapture readonly %src, i32 %len) {
+; CHECK: Function Attrs
+; CHECK-SAME: nounwind
+; CHECK-NOT: norecurse
+; CHECK-NEXT: define void @intrinsic(i8* nocapture %dest, i8* nocapture readonly %src, i32 %len)
 define void @intrinsic(i8* %dest, i8* %src, i32 %len) {
   call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 %len, i1 false)
   ret void
 }
+
+; CHECK: Function Attrs
+; CHECK-NEXT: declare void @llvm.memcpy.p0i8.p0i8.i32
 declare void @llvm.memcpy.p0i8.p0i8.i32(i8*, i8*, i32, i1)
 
-; CHECK: define internal i32 @called_by_norecurse() #0
+; CHECK: Function Attrs
+; CHECK-SAME: norecurse readnone
+; CHECK-NEXT: define internal i32 @called_by_norecurse()
 define internal i32 @called_by_norecurse() {
   %a = call i32 @k()
   ret i32 %a
 }
+; CHECK: Function Attrs
+; CHECK-NEXT: define void @m()
 define void @m() norecurse {
   %a = call i32 @called_by_norecurse()
   ret void
 }
 
-; CHECK: define internal i32 @called_by_norecurse_indirectly() #0
+; CHECK: Function Attrs
+; CHECK-SAME: norecurse readnone
+; CHECK-NEXT: define internal i32 @called_by_norecurse_indirectly()
 define internal i32 @called_by_norecurse_indirectly() {
   %a = call i32 @k()
   ret i32 %a
 }
 define internal void @o() {
   %a = call i32 @called_by_norecurse_indirectly()
   ret void
 }
 define void @p() norecurse {
   call void @o()
   ret void
 }
-
-; CHECK: attributes #0 = { norecurse readnone }
-; CHECK: attributes #1 = { readnone }

test/Transforms/FunctionAttrs/operand-bundles-scc.ll

 ; RUN: opt -S -functionattrs < %s | FileCheck %s
+; RUN: opt -S -passes=function-attrs < %s | FileCheck %s
 
 define void @f() {
-; CHECK-LABEL:  define void @f() {
+; CHECK-LABEL:  define void @f() #0 {
  call void @g() [ "unknown"() ]
  ret void
 }
 
 define void @g() {
-; CHECK-LABEL:  define void @g() {
+; CHECK-LABEL:  define void @g() #0 {
  call void @f()
  ret void
 }
+
+
+; CHECK: attributes #0 = { nounwind }

test/Transforms/FunctionAttrs/optnone.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 
 @x = global i32 0
 
 define void @test_opt(i8* %p) {
 ; CHECK-LABEL: @test_opt
 ; CHECK: (i8* nocapture readnone %p) #0 {
   ret void
 }
 
 define void @test_optnone(i8* %p) noinline optnone {
 ; CHECK-LABEL: @test_optnone
 ; CHECK: (i8* %p) #1 {
   ret void
 }
 
 declare i8 @strlen(i8*) noinline optnone
 ; CHECK-LABEL: @strlen
 ; CHECK: (i8*) #1
 
 ; CHECK-LABEL: attributes #0
-; CHECK: = { norecurse readnone }
+; CHECK: = { norecurse nounwind readnone }
 ; CHECK-LABEL: attributes #1
 ; CHECK: = { noinline optnone }

test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll

 ; RUN: opt -functionattrs -S < %s | FileCheck %s
+; RUN: opt -passes=function-attrs -S < %s | FileCheck %s
 
 ; This checks for an iterator wraparound bug in FunctionAttrs.  The previous
 ; "incorrect" behavior was inferring readonly for the %x argument in @caller.
 ; Inferring readonly for %x *is* actually correct, since @va_func is marked
 ; readonly, but FunctionAttrs was inferring readonly for the wrong reasons (and
 ; we _need_ the readonly on @va_func to trigger the problematic code path).  It
 ; is possible that in the future FunctionAttrs becomes smart enough to infer
 ; readonly for %x for the right reasons, and at that point this test will have

test/Transforms/FunctionAttrs/readnone.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 
 ; CHECK: define void @bar(i8* nocapture readnone)
 define void @bar(i8* readonly) {
   call void @foo(i8* %0)
     ret void
 }
 
 ; CHECK: define void @foo(i8* nocapture readnone)
 define void @foo(i8* readonly) {
   call void @bar(i8* %0)
   ret void
 }

test/Transforms/FunctionAttrs/returned.ll

 ; RUN: opt < %s -functionattrs -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
 
 ; CHECK: define i32 @test1(i32 %p, i32 %q)
 define i32 @test1(i32 %p, i32 %q) {
 entry:
   %cmp = icmp sgt i32 %p, %q
   br i1 %cmp, label %cond.end, label %lor.lhs.false
 
 lor.lhs.false:                                    ; preds = %entry

test/Transforms/Inline/cgscc-update.ll

 ; RUN: opt < %s -aa-pipeline=basic-aa -passes='cgscc(function-attrs,inline)' -S | FileCheck %s
 ; This test runs the inliner and the function attribute deduction. It ensures
 ; that when the inliner mutates the call graph it correctly updates the CGSCC
 ; iteration so that we can compute refined function attributes. In this way it
 ; is leveraging function attribute computation to observe correct call graph
 ; updates.
 
 ; Boring unknown external function call.
 ; CHECK: declare void @unknown()
 declare void @unknown()
 
 ; Sanity check: this should get annotated as readnone.
-; CHECK: Function Attrs: readnone
+; CHECK: Function Attrs: nounwind readnone
 ; CHECK-NEXT: declare void @readnone()
-declare void @readnone() readnone
+declare void @readnone() readnone nounwind
 
 ; The 'test1_' prefixed functions are designed to trigger forming a new direct
 ; call in the inlined body of the function. After that, we form a new SCC and
 ; using that can deduce precise function attrs.
 
 ; This function should no longer exist.
 ; CHECK-NOT: @test1_f()
 define internal void @test1_f(void()* %p) {
 entry:
   call void %p()
   ret void
 }
 
 ; This function should have had 'readnone' deduced for its SCC.
-; CHECK: Function Attrs: noinline readnone
+; CHECK: Function Attrs: noinline nounwind readnone
 ; CHECK-NEXT: define void @test1_g()
 define void @test1_g() noinline {
 entry:
   call void @test1_f(void()* @test1_h)
   ret void
 }
 
 ; This function should have had 'readnone' deduced for its SCC.
-; CHECK: Function Attrs: noinline readnone
+; CHECK: Function Attrs: noinline nounwind readnone
 ; CHECK-NEXT: define void @test1_h()
 define void @test1_h() noinline {
 entry:
   call void @test1_g()
   call void @readnone()
   ret void
 }
 
 
 ; The 'test2_' prefixed functions are designed to trigger forming a new direct
 ; call due to RAUW-ing the returned value of a called function into the caller.
 ; This too should form a new SCC which can then be reasoned about to compute
 ; precise function attrs.
 
 ; This function should no longer exist.
 ; CHECK-NOT: @test2_f()
 define internal void()* @test2_f() {
 entry:
   ret void()* @test2_h
 }
 
 ; This function should have had 'readnone' deduced for its SCC.
-; CHECK: Function Attrs: noinline readnone
+; CHECK: Function Attrs: noinline nounwind readnone
 ; CHECK-NEXT: define void @test2_g()
 define void @test2_g() noinline {
 entry:
   %p = call void()* @test2_f()
   call void %p()
   ret void
 }
 
 ; This function should have had 'readnone' deduced for its SCC.
-; CHECK: Function Attrs: noinline readnone
+; CHECK: Function Attrs: noinline nounwind readnone
 ; CHECK-NEXT: define void @test2_h()
 define void @test2_h() noinline {
 entry:
   call void @test2_g()
   call void @readnone()
   ret void
 }
 

 
 ; The 'test4_' prefixed functions are designed to trigger forming a new direct
 ; call in the inlined body of the function similar to 'test1_'. However, after
 ; that we continue to inline another edge of the graph forcing us to do a more
 ; interesting call graph update for the new call edge. Eventually, we still
 ; form a new SCC and should use that can deduce precise function attrs.
 
 ; This function should have had 'readnone' deduced for its SCC.
-; CHECK: Function Attrs: noinline readnone
+; CHECK: Function Attrs: noinline nounwind readnone
 ; CHECK-NEXT: define void @test4_f1()
 define void @test4_f1() noinline {
 entry:
   call void @test4_h()
   ret void
 }
 
 ; CHECK-NOT: @test4_f2
 define internal void @test4_f2() {
 entry:
   call void @test4_f1()
   ret void
 }
 
 ; CHECK-NOT: @test4_g
 define internal void @test4_g(void()* %p) {
 entry:
   call void %p()
   ret void
 }
 
 ; This function should have had 'readnone' deduced for its SCC.
-; CHECK: Function Attrs: noinline readnone
+; CHECK: Function Attrs: noinline nounwind readnone
 ; CHECK-NEXT: define void @test4_h()
 define void @test4_h() noinline {
 entry:
   call void @test4_g(void()* @test4_f2)
   ret void
 }

test/Transforms/PruneEH/2008-06-02-Weak.ll

-; RUN: opt < %s -prune-eh -S | not grep nounwind
+; RUN: opt < %s -prune-eh -S | FileCheck %s
+; RUN: opt < %s -passes='function-attrs,function(simplify-cfg)' -S | FileCheck %s
 
+; We should not infer 'nounwind' for/from a weak function,
+; since it can be overriden by throwing implementation.
+;
+; CHECK-LABEL: define weak void @f()
 define weak void @f() {
 entry:
         ret void
 }
 
+; CHECK-LABEL: define void @g()
 define void @g() {
 entry:
   call void @f()
   ret void
 }
+
+; CHECK-NOT: {{^}}attributes #{{[0-9].*}} nounwind

test/Transforms/PruneEH/ipo-nounwind.ll

 ; RUN: opt -S -prune-eh < %s | FileCheck %s
+; RUN: opt -S -passes='function-attrs,function(simplify-cfg)' < %s | FileCheck %s
 
 declare void @may_throw()
 
 ; @callee below may be an optimized form of this function, which can
 ; throw at runtime (see r265762 for more details):
 ; 
 ; define linkonce_odr void @callee(i32* %ptr) noinline {
 ; entry:

test/Transforms/PruneEH/operand-bundles.ll

 ; RUN: opt < %s -prune-eh -S | FileCheck %s
+; RUN: opt < %s -passes='function-attrs,function(simplify-cfg)' -S | FileCheck %s
 
 declare void @nounwind() nounwind
 
 define internal void @foo() {
   call void @nounwind()
   ret void
 }
 

test/Transforms/PruneEH/pr23971.ll

 ; RUN: opt -S -prune-eh < %s | FileCheck %s
+; RUN: opt -S -passes='function-attrs,function(simplify-cfg)' < %s | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @f() #0 {
 entry:
   call void asm sideeffect "ret\0A\09", "~{dirflag},~{fpsr},~{flags}"()
   unreachable

test/Transforms/PruneEH/pr26263.ll

-; RUN: opt -prune-eh -S < %s | FileCheck %s
+; PruneEH is less powerful than simplify-cfg in terms of cfg simplification,
+; so it leaves some of the unreachable stuff hanging around.
+; Checking it with CHECK-OLD.
+;
+; RUN: opt -prune-eh -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-OLD
+; RUN: opt -passes='function-attrs,function(simplify-cfg)' -S < %s | FileCheck %s  --check-prefix=CHECK --check-prefix=CHECK-NEW
+
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
 target triple = "i386-pc-windows-msvc"
 
 declare void @neverthrows() nounwind
 
 define void @test1() personality i32 (...)* @__CxxFrameHandler3 {
   invoke void @neverthrows()
           to label %try.cont unwind label %cleanuppad
 
 try.cont:
   ret void
 
 cleanuppad:
   %cp = cleanuppad within none []
   br label %cleanupret
 
 cleanupret:
   cleanupret from %cp unwind to caller
 }
 
 ; CHECK-LABEL: define void @test1(
-; CHECK: call void @neverthrows()
+; CHECK:       call void @neverthrows()
+; CHECK-NEW-NEXT: ret void
+; CHECK-NEW-NEXT: }
+; CHECK-OLD:    ret void
 
-; CHECK: %[[cp:.*]] = cleanuppad within none []
-; CHECK-NEXT: unreachable
+; CHECK-OLD: %[[cp:.*]] = cleanuppad within none []
+; CHECK-OLD-NEXT: unreachable
 
-; CHECK: cleanupret from %[[cp]] unwind to caller
+; CHECK-OLD: cleanupret from %[[cp]] unwind to caller
 
 define void @test2() personality i32 (...)* @__CxxFrameHandler3 {
   invoke void @neverthrows()
           to label %try.cont unwind label %catchswitch
 
 try.cont:
   ret void
 
 catchswitch:
   %cs = catchswitch within none [label %catchpad] unwind to caller
 
 catchpad:
   %cp = catchpad within %cs []
   unreachable
 
 ret:
   ret void
 }
 
 ; CHECK-LABEL: define void @test2(
-; CHECK: call void @neverthrows()
+; CHECK:       call void @neverthrows()
+; CHECK-NEW-NEXT: ret void
+; CHECK-NEW-NEXT: }
+; CHECK-OLD:      ret void
+
+; CHECK-OLD: %[[cs:.*]] = catchswitch within none [label
 
-; CHECK: %[[cs:.*]] = catchswitch within none [label
+; CHECK-OLD: catchpad within %[[cs]] []
+; CHECK-OLD-NEXT: unreachable
 
-; CHECK: catchpad within %[[cs]] []
-; CHECK-NEXT: unreachable
+; CHECK-OLD:ret void
 
 declare i32 @__CxxFrameHandler3(...)

test/Transforms/PruneEH/recursivetest.ll

-; RUN: opt < %s -prune-eh -S | not grep invoke
+; RUN: opt < %s -prune-eh -S | FileCheck %s
+; RUN: opt < %s -passes='function-attrs,function(simplify-cfg)' -S | FileCheck %s
 
+; CHECK-LABEL: define internal i32 @foo()
 define internal i32 @foo() personality i32 (...)* @__gxx_personality_v0 {
+; CHECK-NOT: invoke i32 @foo()
   invoke i32 @foo( )
       to label %Normal unwind label %Except   ; <i32>:1 [#uses=0]
 Normal:   ; preds = %0
   ret i32 12
 Except:   ; preds = %0
         landingpad { i8*, i32 }
                 catch i8* null
   ret i32 123
 }
 
+; CHECK-LABEL: define i32 @caller()
 define i32 @caller() personality i32 (...)* @__gxx_personality_v0 {
+; CHECK-NOT: invoke i32 @foo()
   invoke i32 @foo( )
       to label %Normal unwind label %Except   ; <i32>:1 [#uses=0]
 Normal:   ; preds = %0
   ret i32 0
 Except:   ; preds = %0
         landingpad { i8*, i32 }
                 catch i8* null
   ret i32 1
 }
 
 declare i32 @__gxx_personality_v0(...)

test/Transforms/PruneEH/seh-nounwind.ll

 ; RUN: opt -S -prune-eh < %s | FileCheck %s
+; RUN: opt -S -passes='function-attrs,function(simplify-cfg)' < %s | FileCheck %s
 
 ; Don't remove invokes of nounwind functions if the personality handles async
 ; exceptions. The @div function in this test can fault, even though it can't
 ; throw a synchronous exception.
 
 define i32 @div(i32 %n, i32 %d) nounwind {
 entry:
   %div = sdiv i32 %n, %d

test/Transforms/PruneEH/simpletest.ll

-; RUN: opt < %s -prune-eh -S | not grep invoke
+; RUN: opt < %s -prune-eh -S | FileCheck %s
+; RUN: opt < %s -passes='function-attrs,function(simplify-cfg)' -S | FileCheck %s
 
 declare void @nounwind() nounwind
 
 define internal void @foo() {
   call void @nounwind()
   ret void
 }
 
+; CHECK-LABEL: define i32 @caller()
 define i32 @caller() personality i32 (...)* @__gxx_personality_v0 {
+; CHECK-NOT: invoke void @foo
   invoke void @foo( )
       to label %Normal unwind label %Except
 
 Normal:   ; preds = %0
   ret i32 0
 
 Except:   ; preds = %0
         landingpad { i8*, i32 }
                 catch i8* null
   ret i32 1
 }
 
 declare i32 @__gxx_personality_v0(...)