[AMDGPU] Simplify handleAddressTakenFunctions. NFC.

[foad]

Summary

Use standard depth first iterators to simplify the implementation. Also
don't bother passing in ModuleCG when it is available in the class.

[madhur13490]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

246

Please move this check inside the function otherwise every caller will have to do this check.

[madhur13490]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

234

It is better to keep this assert.

[foad]

Change removeAttributes to take a reference. Reinstate an assert.

[foad]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

246

I disagree. "Remove attributes from F" is a simpler API than "remove attributes from F, except if F is null". I have changed removeAttributes to take a reference to make it more obvious that F must be a function.

Also, there is only one caller, and even before my patch there were two callers but only one of them had to cope with F possibly being null. This is caused by a quirk of the CallGraph which contains artificial nodes for external callers/callees, where getFunction returns null.

[madhur13490]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

246

Yes, the quirks of CallGraph makes me worry about this. Please make sure it does not regress as I had to specifically check for !F as a result of failed AOMP smoke tests.

[madhur13490]

Commit 660cae84c3144a42272daa16415fc9a2532773c4 reverts the base patch due to an unsupported scenario but thankfully this patch fixes that issue. Please rebase and keep the support intact.

[madhur13490]

Please remove NFC due to the new context. @JonChesterfield found an issue with recursions in the base patch. The following case depicts the scenario

target triple = "amdgcn-amd-amdhsa"
  
define i64 @f(i64 %x)  {
entry:
        br i1 undef, label %return, label %if.end

        if.end:
        %call = call i64 @f(i64 undef)
        unreachable

        return:
        ret i64 1
}

define i8* @unrelated() {
entry:
 ret i8* bitcast (i64 (i64)* @f to i8*)
}

declare amdgpu_kernel void @has_a_kernel(i32)

Please add this test as a part of the patch.

[arsenm]

This can probably be abandoned. The patch this was on top of was reverted. We also don't specially treat address taken functions anymore

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

 /// pipeline. A module pass is more powerful but has to run late, so misses
 /// library folding opportunities.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AMDGPU.h"
 #include "MCTargetDesc/AMDGPUMCTargetDesc.h"
 #include "Utils/AMDGPUBaseInfo.h"
+#include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include <queue>
public:
   // Use F as a root and propagate its attributes.
   bool process(Function &F);
 
   // Propagate attributes starting from kernel functions.
   bool process(Module &M, CallGraph *CG);
 
   // Remove attributes from F.
   // This is used in presence of address taken functions.
-  bool removeAttributes(Function *F);
+  bool removeAttributes(Function &F);
 
   // Handle call graph rooted at address taken functions.
   // This function will erase all attributes present
   // on all functions called from address taken functions transitively.
-  bool handleAddressTakenFunctions(CallGraph *CG);
+  bool handleAddressTakenFunctions();
 };
 
 // Allows to propagate attributes early, but no clonning is allowed as it must
 // be a function pass to run before any optimizations.
 // TODO: We shall only need a one instance of module pass, but that needs to be
 // in the linker pipeline which is currently not possible.
 class AMDGPUPropagateAttributesEarly : public FunctionPass {
   const TargetMachine *TM;
INITIALIZE_PASS(AMDGPUPropagateAttributesEarly,
                 "amdgpu-propagate-attributes-early",
                 "Early propagate attributes from kernels to functions",
                 false, false)
 INITIALIZE_PASS(AMDGPUPropagateAttributesLate,
                 "amdgpu-propagate-attributes-late",
                 "Late propagate attributes from kernels to functions",
                 false, false)
 
-bool AMDGPUPropagateAttributes::removeAttributes(Function *F) {
+bool AMDGPUPropagateAttributes::removeAttributes(Function &F) {
   bool Changed = false;
-  if (!F)
-    return Changed;
-  LLVM_DEBUG(dbgs() << "Removing attributes from " << F->getName() << '\n');
-  for (unsigned I = 0; I < NumAttr; ++I) {
-    if (F->hasFnAttribute(AttributeNames[I])) {
-      F->removeFnAttr(AttributeNames[I]);
+  LLVM_DEBUG(dbgs() << "Removing attributes from " << F.getName() << '\n');
+  for (const char *AttrName : AttributeNames) {
+    if (F.hasFnAttribute(AttrName)) {
+      F.removeFnAttr(AttrName);
       Changed = true;
     }
   }
   return Changed;
 }
 
-bool AMDGPUPropagateAttributes::handleAddressTakenFunctions(CallGraph *CG) {
+bool AMDGPUPropagateAttributes::handleAddressTakenFunctions() {
   assert(ModuleCG && "Call graph not present");
 
   bool Changed = false;
-  SmallSet<CallGraphNode *, 32> Visited;
+  df_iterator_default_set<CallGraphNode *> Reachable;
 
+  // Find all functions reachable from address-taken functions.
   for (Function *F : AddressTakenFunctions) {
-    CallGraphNode *CGN = (*CG)[F];
-    if (!Visited.count(CGN)) {
-      Changed |= removeAttributes(F);
-      Visited.insert(CGN);
-    }
-
-    std::queue<CallGraphNode *> SubGraph;
-    SubGraph.push(CGN);
-    while (!SubGraph.empty()) {
-      CallGraphNode *CGN = SubGraph.front();
-      SubGraph.pop();
-      if (!Visited.count(CGN)) {
-        Changed |= removeAttributes(CGN->getFunction());
-        Visited.insert(CGN);
-      }
-      for (auto N : *CGN)
-        SubGraph.push(N.second);
-    }
+    for (auto *I : depth_first_ext((*ModuleCG)[F], Reachable))
+      (void)I;
+
+  // Remove attributes from all reachable functions.
+  for (CallGraphNode *CGN : Reachable) {
+    if (Function *F = CGN->getFunction())
+      Changed |= removeAttributes(*F);
   }
+
   return Changed;
 }
 
 Function *
 AMDGPUPropagateAttributes::findFunction(const FnProperties &PropsNeeded,
                                         Function *OrigF) {
   // TODO: search for clone's clones.
   for (Clone &C : Clones)
bool AMDGPUPropagateAttributes::process() {
 
   Roots.clear();
   Clones.clear();
 
   // Keep the post processing related to indirect
   // calls separate to handle them gracefully.
   // The core traversal need not be affected by this.
   if (AllowClone)
-    Changed |= handleAddressTakenFunctions(ModuleCG);
+    Changed |= handleAddressTakenFunctions();
 
   return Changed;
 }
 
 Function *
 AMDGPUPropagateAttributes::cloneWithProperties(Function &F,
                                                const FnProperties &NewProps) {
   LLVM_DEBUG(dbgs() << "Cloning " << F.getName() << '\n');