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

[Hideto Ueno via Phabricator via llvm-commits]

uenoku marked 3 inline comments as done.
uenoku added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:769
+    for (Instruction *I : OpcodeInstMap[Opcode]) {
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        switch (II->getIntrinsicID()) {
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:800
+      if ((!NoFreeAA || !NoFreeAA->isAssumedNoFree()) &&
+          !ICS.hasFnAttr("nofree")) {
+        indicatePessimisticFixpoint();
----------------
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
}
```


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

https://reviews.llvm.org/D62687