[PATCH] D62687: [Attributor] Deduce "nofree" function attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Almost there, partially because I didn't think certain problems through, see inlined comments.

The test changes are still missing.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:769
+    for (Instruction *I : OpcodeInstMap[Opcode]) {
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        switch (II->getIntrinsicID()) {
----------------
uenoku wrote:
> jdoerfert wrote:
> > Do we have any intrinsic that might free memory? I would need to double check but I don't think we have one.
> I think so too. However, I have one concern. There are some intrinsic related to GC and I can not be sure whether these intrinsic wouldn't free memory.
That is a good point. Let's do the following:

- Do not look at intrinsic here as at all so we are sound for sure (we should have a test for that!)
- Annotate intrinsics in their ".td" file definition (different patch later on)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:800
+      if ((!NoFreeAA || !NoFreeAA->isAssumedNoFree()) &&
+          !ICS.hasFnAttr("nofree")) {
+        indicatePessimisticFixpoint();
----------------
uenoku wrote:
> jdoerfert wrote:
> > My other two comments were confusing and I should have explained this better.
> > 
> > The idea (I have) is:
> > Information should be determined through other attributes if possible. That way we do not need to look at the IR in different places and also ask if an attribute exist and what the status is. That would mean do not look at the IR attributes (through the CallSite). I also confused you with the `isAssumedNoFree` as it seems. I did want that in addition to the two checks you had, so `!NoFreeAA`, `!NoFreeAA->getState().hasValidState()`, and ` !NoFreeAA->isAssumedNoFree()`, even though the last two are equivalent it makes it easier to read and consistent with more complex attributes.
> I have a question about this. 
> As you said I changed not to look IR attributes. Then I don't know how to deal with a function declaration like this:
> ```
> declare void @nofree_function() "nofree" 
> 
> define void @call_nofree_function() {
>     tail call void @nofree_function()
>     ret void
> }
> ```
Good point, very good point. Let's agree that I was wrong for now and you add back the ImmutableCallSite attribute check.


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

https://reviews.llvm.org/D62687