Page MenuHomePhabricator

NoFree argument attribute.
ClosedPublic

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

Details

Summary

Deducing nofree atrribute for function arguments.

Diff Detail

Event Timeline

sstefan1 created this revision.Sep 21 2019, 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
1508–1509

remove it completely please

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

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.

1578

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

1595

unknown users need to be treated as potential frees.

1596

I doubt calling indicateOptimisticFixpoint is correct.

1632

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
275

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.Sep 23 2019, 12:48 PM
sstefan1 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
1538

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

1632

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
275

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.Sep 23 2019, 1:48 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1632

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
275

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

sstefan1 updated this revision to Diff 222916.Oct 2 2019, 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
1558

LangRef also needs to be updated I guess.

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

F = getIRPosition().getAnchorScope()

1541

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.).

1549

(Arg->getParent()) -> F

1569

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).

1575

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

1577

no need for this check

1665

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.

5704–5705

leftover

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

quick feedback

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

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

1597

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

1665

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
265

ATTRIBUTOR: check line missing

Also replace the CHECK with ATTRIBUTOR

sstefan1 updated this revision to Diff 224817.Oct 14 2019, 1:07 AM
sstefan1 marked an inline comment as done.
  • minor updates.
jdoerfert accepted this revision.Oct 14 2019, 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
1540

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.

1561

Hoist this out of the loop

1576

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.Oct 14 2019, 10:05 AM
sstefan1 updated this revision to Diff 224926.Oct 14 2019, 4:11 PM
  • update tests
This revision was automatically updated to reflect the committed changes.

This breaks tests on all bots. Please fix quickly or revert, and please run tests locally before committing.

This breaks tests on all bots. Please fix quickly or revert, and please run tests locally before committing.

You missed to update InferFunctionAttrs/dereferenceable.ll

/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm-project/llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:11:21: error: ATTRIBUTOR-LABEL: expected string not found in input
; ATTRIBUTOR-LABEL: @PR21780(double* nocapture nonnull readonly dereferenceable(32) %ptr)
                    ^
<stdin>:1:1: note: scanning from here
; ModuleID = '<stdin>'
^
<stdin>:5:21: note: possible intended match here
define <4 x double> @PR21780(double* nocapture nofree nonnull readonly dereferenceable(32) %ptr) #0 {

Yes, I missed that one. Sorry for the mess.