This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Deduce "nofree" function attribute
ClosedPublic

Authored by uenoku on May 30 2019, 9:17 AM.

Details

Summary

Deduce "nofree" function attribute. A more concise description of "nofree" is on D49165.

Diff Detail

Repository
rL LLVM

Event Timeline

uenoku created this revision.May 30 2019, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 9:17 AM

Are langref changes missing?
How do these attributes get documented there?

llvm/include/llvm/IR/Attributes.td
205 ↗(On Diff #202201)

Can this please be more explanatory?
I can read this as "free() isn't being called", "all calls here are non-free (as in non-cheap)" etc

llvm/lib/Transforms/IPO/Attributor.cpp
186 ↗(On Diff #202201)

StringRef

1089 ↗(On Diff #202201)

Hm, there is no .empty() ?

uenoku edited the summary of this revision. (Show Details)May 30 2019, 9:28 AM

@lebedev.ri.

Are langref changes missing? How do these attributes get documented there?

Langref and concise description are on D49165. This patch goes on the assumption that D49165 will be rebased and upstreamed.
Please excuse my lack of explanation.

uenoku edited the summary of this revision. (Show Details)May 30 2019, 9:46 AM

Please make this depend on the no-free patch by @hfinkel. We should also see if we extract the definition from there or if he plans to work on that patch further.

Some comments inlined.

I don't know why we would need TLI here but could you split the addition of it so we have it for the future.

llvm/lib/Transforms/IPO/Attributor.cpp
187 ↗(On Diff #202201)

We want an llvm::StringSwitch here.

We could also move the whole bookkeeping into the AbstractAttributes. That might actually be nicer.

724 ↗(On Diff #202201)

Please add bool isAssumedNoFree() and bool isKnownNoFree().

759 ↗(On Diff #202201)

You can directly get the AnchorScope

771 ↗(On Diff #202201)

I don't see why we'd need this conditional (and the Realloc check).

800 ↗(On Diff #202201)

No need for the assert, and if you want one, add a message.

801 ↗(On Diff #202201)

You can make safe indention if you continue for nofree call sites. Though, I don't see why we should check the attribute in the first place. The following check is what we want.

803 ↗(On Diff #202201)

Please add || !NoFreeAA->isAssumedNoFree() for concistency.

Revert the condition for less indented code.

810 ↗(On Diff #202201)

There is no fixpoint reached here, at least not if getAAFor was called at least once! Just remove the line and let the framework determine this for you.

uenoku updated this revision to Diff 202266.EditedMay 30 2019, 12:04 PM
uenoku edited the summary of this revision. (Show Details)

Sorry, I was quite mistaken about Attributor. I removed TLI and followed comments.

uenoku marked 8 inline comments as done.May 30 2019, 12:06 PM

Sorry, I was quite mistaken about Attributor. I removed TLI and followed comments.

You misunderstood, there is absolutely no need to apologize. I like the patch and it is totally normal that I have comments now.
This is the first patch to a new framework and I have a lot of thoughts in my head how the code should look like/interact with the rest.

The patch is almost ready as far as I can tell, I added some (mostly minor) comments though.

Did you actually run the pass and try it on the test case you have? The changes to the test case should be part of this patch (pretend the test cases are already in tree).

llvm/lib/Transforms/IPO/Attributor.cpp
716 ↗(On Diff #202266)

If you make it a struct you don't need the public anymore.

731 ↗(On Diff #202266)

Comment and new line pls.

751 ↗(On Diff #202266)

Comments please.

769 ↗(On Diff #202266)

Do we have any intrinsic that might free memory? I would need to double check but I don't think we have one.

800 ↗(On Diff #202266)

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.

971 ↗(On Diff #202266)

Please add an assert here to make sure we did not miss any CallBase instruction opcode.

uenoku updated this revision to Diff 202386.May 31 2019, 1:32 AM

Assume that all intrinsic function wouldn't free memory.

uenoku marked 3 inline comments as done.May 31 2019, 1:32 AM
uenoku marked 3 inline comments as done.May 31 2019, 1:50 AM
uenoku added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
769 ↗(On Diff #202266)

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.

800 ↗(On Diff #202266)

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
}

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

The test changes are still missing.

llvm/lib/Transforms/IPO/Attributor.cpp
769 ↗(On Diff #202266)

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)
800 ↗(On Diff #202266)

Good point, very good point. Let's agree that I was wrong for now and you add back the ImmutableCallSite attribute check.

uenoku updated this revision to Diff 202535.May 31 2019, 10:50 PM

Assume that all intrinsic would free memory and use IR Attribute.

The test changes are still missing.

Oh, I missed it. I'm now writing this but I'm in trouble.

In D62313, you said that it is preferable to write a test like this:

; CHECK:       Function Attrs: noinline norecurse nounwind readnone uwtable
; CHECK-NEXT:     define double* @ret_undef_arg_undef(i32* readnone %b)

However, I think this format would not work for string attribute, isn't it?

The test changes are still missing.

Oh, I missed it. I'm now writing this but I'm in trouble.

In D62313, you said that it is preferable to write a test like this:

; CHECK:       Function Attrs: noinline norecurse nounwind readnone uwtable
; CHECK-NEXT:     define double* @ret_undef_arg_undef(i32* readnone %b)

However, I think this format would not work for string attribute, isn't it?

I honestly do not know, if it is not, you need to choose a way that works. If you find that difficult, let me know. (Do not delete the checks but remove the CHECK part or rename it).

uenoku updated this revision to Diff 202552.Jun 1 2019, 7:46 AM

Add test changes. A few test cases are added.

uenoku updated this revision to Diff 202553.Jun 1 2019, 7:48 AM

Fix patch.

Last comments wrt the test cases. I hope D62784 is uncontroversial and we can use the Function Attr: check lines. That is the last change I'd like to see.

llvm/test/Transforms/FunctionAttrs/nofree.ll
32 ↗(On Diff #202553)

Assuming I can land D62784, you can check for "nofree" as you do for the FNATTR attributes.

Use -NEXT: for the define lines so you make sure that you check the attributes of that definition.

Use -NOT: nofree for the negative test cases to make sure we do not derive it.

174 ↗(On Diff #202553)

Great!

uenoku updated this revision to Diff 202601.Jun 2 2019, 7:25 AM

Fix test format and some typo.

uenoku marked an inline comment as done.Jun 2 2019, 7:48 AM

I hope D62784 is uncontroversial and we can use the Function Attr: check lines.

Thanks. It works.

jdoerfert accepted this revision.Jun 2 2019, 8:21 AM

This depends on three other patches now but once they are in, this one can go in as well. LGTM.

This revision is now accepted and ready to land.Jun 2 2019, 8:21 AM

Why does this depend on D63046?

Why does this depend on D63046?

I included "nofree" attribute in D63046 test change for now(I thought it is convenient for merging). So I regarded this as a parent just in case.

Can you make the test use an enum version, thus nofree not "nofree".

llvm/include/llvm/IR/Attributes.td
206 ↗(On Diff #202601)

Remove this line please.

uenoku updated this revision to Diff 205251.Jun 17 2019, 9:50 PM
uenoku edited the summary of this revision. (Show Details)
  • use enum attribute
  • rename test file
This comment was removed by uenoku.

Once the test file is in, rebase this one and make sure ninja check-all works before you commit it. Also, there is a leftover FIXME in the code.

llvm/lib/Transforms/IPO/Attributor.cpp
298 ↗(On Diff #205251)

Leftover comment.

uenoku updated this revision to Diff 209499.Jul 12 2019, 8:56 AM

Add tests and check-all passed.

This revision was automatically updated to reflect the committed changes.