[PATCH] D103138: [AMDGPU] [IndirectCalls] Don't propagate attributes to address taken functions and their callees

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp:118
 
+  CallGraph *ModuleCG;
+
----------------
foad wrote:
> Add the nullptr initializion here instead of in the constructor?
May I know why?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp:248
+
+    std::queue<CallGraphNode *> SubGraph;
+    SubGraph.push(CGN);
----------------
foad wrote:
> It's more common to use a SmallVector for this, with push_back and pop_back_val.
I need a queue. This is implementing BFS. 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp:234-237
+  if (!ModuleCG) {
+    llvm_unreachable("Call graph not valid");
+    return Changed;
+  }
----------------
foad wrote:
> madhur13490 wrote:
> > arsenm wrote:
> > > Just assert, the return is dead
> > I'd say its a style choice. Preferring llvm_unreachable over assert.
> Then it's a bad choice :) I agree with Matt. `assert(CG)` or `assert(CG && "message")` are shorter and simpler and more familiar because they're already ubiquitous throughout llvm.
As per coding standards, llvm_unreachable seems more preferred.
https://llvm.org/docs/CodingStandards.html#assert-liberally

@rampitec What do you think? Internally, I changed this from assert.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103138/new/

https://reviews.llvm.org/D103138