[PATCH] D35633: [ThinLTO] Add FunctionAttr NoRecurse and ReadAttr propagation to ThinLTO

[Charles Saternos via Phabricator via llvm-commits]

ncharlie added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:445
+              if (!S->fflags().NoRecurse ||
+                  S->getOriginalName() == F->getOriginalName())
+                return false;
----------------
mehdi_amini wrote:
> ncharlie wrote:
> > mehdi_amini wrote:
> > > Not sure why the second condition is needed?
> > I copied that out of the original FunctionAttrs pass to prevent the function from being marked NoRecurse if it calls itself (http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionAttrs.cpp#1068).
> This comment does not display in the right place for me right now, to clarify I'm wondering about this:
> 
> ```
>             if (!S->fflags().NoRecurse ||
>                 S->getOriginalName() == F->getOriginalName())
> ```
> 
> So the second check here is intended to check for self-recursion. Are we recording self-recursion in the summaries? 
> Can we check GUID equality instead of string comparison?
> Also we need a test for this exact condition, i.e. a test that would fail with ` if (!S->fflags().NoRecurse)` alone.
> Are we recording self-recursion in the summaries? 
I think so. I'll double check, but I think this is a redundant check (in which case I'll remove it).


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+      FS->updateCallGraphEdgeMAK(P.first->getGUID(), P.second);
+    }
+  }
----------------
mehdi_amini wrote:
> ncharlie wrote:
> > mehdi_amini wrote:
> > > Why do we need to mark the edges?
> > I was planning on using that to propagate read attrs up to the caller. For instance:
> > 
> > ```
> > void x(int *k) { // can't mark readonly since k could be global
> >   *k++;
> > }
> > 
> > void j() {
> >   int i = 5;
> >   x(&i); // since i is local memory, we can mark j as readnone
> > }
> > ```
> > 
> > I'm currently working on implementing this - when I finish I'll update the patch.
> I think it is complex enough that doing it in two patches would be easier to review and assert correctness, catch edge cases, and make sure we have the right level of test coverage.
> The first patch could be just doing the "simple case", and then you can extend it with the most advanced one. For the simple case you shouldn't need to touch edges I believe.
> 
> Also if possible, I'd even break it so that you first implement propagating only readnone, forget about norecurse and other stuff, and you don't need SCC. Then you can add more stuff. Again we'll have more targeted testing and we are less likely to miss anything in review (and if we need to bisect a failure, it'll be easier to pinpoint the faulty logic as well).
Sounds good - I was thinking about splitting it into multiple patches, and I agree with that level of granularity (i.e. a patch for propagating readnone, a patch for readnone propagated across call edges, and a patch for no recurse).


https://reviews.llvm.org/D35633