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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp:221
+  bool Changed = false;
+  if (!F)
+    return Changed;
----------------
I don't think null F can reach here


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp:234-237
+  if (!ModuleCG) {
+    llvm_unreachable("Call graph not valid");
+    return Changed;
+  }
----------------
madhur13490 wrote:
> madhur13490 wrote:
> > rampitec wrote:
> > > madhur13490 wrote:
> > > > 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.
> > > > 
> > > Internally if was "if() assert(false ...)". assert(false) shall never be used, that's why I suggested llvm_unreachange() instead. But "assert(ModuleCG);" is a more common thing.
> > There are ways to implement this:
> > A.
> > ```
> > if (!ModuleCG) { 
> >    assert(false, "ModuleCG not present"); 
> >    return; 
> > } 
> > <rest code>
> > ```
> > B. Just   
> > ```
> > assert(ModuleCG && "Module CG not present"); 
> > < rest code>
> > ```
> > C.
> > 
> > ```
> >  if (!ModuleCG) { 
> >       llvm_unreachable("ModuleCG"); 
> >       return; 
> >  } 
> > <rest code>
> > ```
> > 
> > Matt suggests B but in release builds the code would just crash because it would end up accessing nullptr.
> > I had A initially because it asserts as well as returns gracefully so it does not crash. C is similar to A just llvm_unreachable replaces assert.
> > 
> > I prefer but C because it follows coding standard and also returns gracefully returns in release builds.
> > 
> > Which one we agree the best here?
> Typo: **I prefer C**
C doesn't follow the coding standard and adds dead code. You also should NOT be attempting to gracefully handle invalid cases in a release build. The optimizer will happily delete this null check anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103138