Page MenuHomePhabricator

Add a test for "nofree" function attribute
ClosedPublic

Authored by uenoku on May 23 2019, 7:00 AM.

Details

Reviewers
jdoerfert
Summary

This patches adds a test for nofree function attributes.

Diff Detail

Event Timeline

uenoku created this revision.May 23 2019, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 7:00 AM
uenoku retitled this revision from Add testcases for "nofree" function attribute. to Add testcases for "nofree" function attribute.May 23 2019, 7:02 AM
uenoku added a reviewer: jdoerfert.
uenoku retitled this revision from Add testcases for "nofree" function attribute to Add a test for "nofree" function attribute.May 23 2019, 7:04 AM
uenoku updated this revision to Diff 200966.May 23 2019, 7:07 AM

Fix test 4.

I like the tests. One comment and one change request was inlined.

llvm/test/Transforms/FunctionAttrs/nofree.ll
98 ↗(On Diff #200966)

I guess one could argue realloc/free/... are just like regular, unknown functions wrt. no-free. I'd keep the tests though. And it opens the question if we should derive "is-freed" at some point ;)

145 ↗(On Diff #200966)

So Chandler raised concerns over this kind of checking in D59903. While I have to admit I haven't fixed all test cases yet, I can see his point. Could you check for the attributes in the printed IR version just before the definition?

It would look similar to this one:

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

Could you also check if there is anything in https://reviews.llvm.org/D49165#change-hksiHW8RDcbA that is missing here?

uenoku updated this revision to Diff 201518.May 27 2019, 5:51 AM

Fix checking way.

uenoku marked an inline comment as done.May 27 2019, 5:57 AM

Could you also check if there is anything in https://reviews.llvm.org/D49165#change-hksiHW8RDcbA that is missing here?

Apparently, I cover all of them.

jdoerfert accepted this revision.May 27 2019, 11:05 AM

Assuming this passes right now it looks-good-to-me.
I would have moved it into the Attributor test folder (which doesn't exist yet), but that can be done later.

This revision is now accepted and ready to land.May 27 2019, 11:05 AM

We should (in a later patch probably) think about other attributes that would imply nofree.
I guess one can argue readnone should do the trick. There are other things we could pull if
we make nofree also a parameter attribute. Finally we could think about a selective nofree
annotation at a call site to list arguments that are not freed. We could know that because
of dereferenceability after the call.

jdoerfert requested changes to this revision.Jun 12 2019, 5:16 PM

As mentioned in D62766, we should make nofree an enum attribute. See D62766 for some pointer on how to do so.

This revision now requires changes to proceed.Jun 12 2019, 5:16 PM
uenoku added a comment.EditedJun 12 2019, 10:05 PM

As mentioned in D62766, we should make nofree an enum attribute. See D62766 for some pointer on how to do so.

As I see it, D49165 is rebased.
Which is better to wait for D49165 or to make nofree enum attribute in this patch?

As mentioned in D62766, we should make nofree an enum attribute. See D62766 for some pointer on how to do so.

As I see it, D49165 is rebased.
Which is better to wait for D49165 or to make nofree enum attribute in this patch?

Good point. If @homerdin adds the attribute in that patch you do not have to.

jdoerfert accepted this revision.Jun 13 2019, 8:46 AM
This revision is now accepted and ready to land.Jun 13 2019, 8:46 AM
uenoku updated this revision to Diff 205253.Jun 17 2019, 10:00 PM

Rename test file.

Make sure ninja check-all works with the test (we got support to derive nofree!) and then commit it please.

uenoku updated this revision to Diff 208597.EditedMon, Jul 8, 10:20 PM

Rebase and ninja check-all passed.

But even if I run with --disable-nofree-inference=false, I couldn't get any nofree (even only_return).

uenoku updated this revision to Diff 208599.Mon, Jul 8, 10:49 PM

Add --disable-nofree-inference=false.

uenoku closed this revision.Fri, Jul 12, 9:39 AM