Page MenuHomePhabricator

NoFree argument attribute.
AcceptedPublic

Authored by sstefan1 on Sat, Sep 21, 2:56 PM.

Details

Reviewers
jdoerfert
uenoku
Summary

Deducing nofree atrribute for function arguments.

Event Timeline

sstefan1 created this revision.Sat, Sep 21, 2:56 PM
Herald added a project: Restricted Project. · View Herald Transcript

There should be other test changes, will update tomorrow.

I'm glad you work on this, this will be helpful for h2s and also deref analysis. I added some comments.

llvm/lib/IR/Verifier.cpp
1504–1505

remove it completely please

llvm/lib/Transforms/IPO/Attributor.cpp
1419

Look at the function attribute first, as long as it is set we do not have to prove anything but can just keep the optimistic state.

1459

This above is not necessary if you use the function of the argument first to justify nofree.

1476

unknown users need to be treated as potential frees.

1477

I doubt calling indicateOptimisticFixpoint is correct.

1513

I would put the logic that follows uses into the floating version and make everything that is not function scope (function & call site) derive from floating. (We need to have a generic use visitor but that is a different story)

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll
277

We need tests where one argument is freed and one is not. Also where either one of two is freed but we don't know which (e.g., due to a phi). And we need uncertainty tests, e.g. the argument escapes and the function (wrt. to the argument) is not nofree.

sstefan1 marked 3 inline comments as done.Mon, Sep 23, 12:48 PM
sstefan1 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
1419

Ok, I agree. This is how I should have done it.

1513

I can do that. Just want to make sure I understand what you are saying. I should keep the current function & call site implementations, right?

I also thought about generic use visitor, I'll think about it and maybe write a patch this week.

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll
277

I will add more tests soon. Actually a lot of tests will be affected, so maybe we are already covering some of the things you are mentioning.

jdoerfert added inline comments.Mon, Sep 23, 1:48 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1513

Keep function & call site as is, yes.

If you want to work on a use visitor that is fine with me. Having edge-wise liveness information is also up there on the list of things I'd like to have.

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll
277

OK, we could add them either way to make it explicit ;)

sstefan1 updated this revision to Diff 222916.Wed, Oct 2, 2:43 PM
  • update tests

I think the logic is fine, some minor comments though. Also, could you update the use in AAHeapToStackImpl::updateImpl to ask for the call site argument instead (with test for the improvement if we don't have one yet)?

llvm/lib/IR/Verifier.cpp
1554

LangRef also needs to be updated I guess.

llvm/lib/Transforms/IPO/Attributor.cpp
1419

F = getIRPosition().getAnchorScope()

1422

Why do we need an argument in the first place here (and why do you dereference it in 1419 before you check it for null in 1421, it can be null btw.).

1430

(Arg->getParent()) -> F

1450

use cast here. Users *have* to be instructions or we have way more problems (and we want it to explode if a user is not an instruction).

1456

you need to treat uses in the operand bundle as "freeing", so, isCalleeOperand -> ok, isArgOperand -> test further, if neither -> not nofree.

1458

no need for this check

1546

overwrite manifest here to ensure we do not add "no-free" to the return value even if we derive it. We can actually derive it as we do not need to restrict it to arguments in the AANoFreeFloating::update.

4426–4427

leftover

sstefan1 updated this revision to Diff 224794.Sun, Oct 13, 2:12 PM
  • addressing comments

quick feedback

llvm/lib/Transforms/IPO/Attributor.cpp
1433

Why do we need an argument and not just the associated value?

1478

Allow PHI nodes and selects as well. Same as bitcast above.

1546

This is still open, see my "why arguments" comment above.

Btw. later we can actually add "nofree" to return values (and call site returns) as it can help to keep dereferenceable. But that is for a follow up patch after we get dereferenceable_globally

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll
267

ATTRIBUTOR: check line missing

Also replace the CHECK with ATTRIBUTOR

sstefan1 updated this revision to Diff 224817.Mon, Oct 14, 1:07 AM
sstefan1 marked an inline comment as done.
  • minor updates.
jdoerfert accepted this revision.Mon, Oct 14, 10:05 AM

Small nits. Otherwise LGTM.
Also, make sure we have a test with 2 pointer arguments, one freed, one no-free annotated.

llvm/lib/Transforms/IPO/Attributor.cpp
1421

Replace IRPosition::function(*F)) with IRPosition::function_scope(IRP).
IRP is getIRPosition(). That should remove the need to look at the call site explicitly below.

1442

Hoist this out of the loop

1457

I don't think this is necessary but fine with me. (should be implied and checked by the call site argument attr).

This revision is now accepted and ready to land.Mon, Oct 14, 10:05 AM
sstefan1 updated this revision to Diff 224926.Mon, Oct 14, 4:11 PM
  • update tests