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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:445
+              if (!S->fflags().NoRecurse ||
+                  S->getOriginalName() == F->getOriginalName())
+                return false;
----------------
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.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+      FS->updateCallGraphEdgeMAK(P.first->getGUID(), P.second);
+    }
+  }
----------------
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).


https://reviews.llvm.org/D35633