Page MenuHomePhabricator

NoFree argument attribute.

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



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.


remove it completely please


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.


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


unknown users need to be treated as potential frees.


I doubt calling indicateOptimisticFixpoint is correct.


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)


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.

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


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.


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

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.


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


LangRef also needs to be updated I guess.


F = getIRPosition().getAnchorScope()


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


(Arg->getParent()) -> F


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


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


no need for this check


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.



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

quick feedback


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


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


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


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.


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.


Hoist this out of the loop


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.