Page MenuHomePhabricator

[Attributor] NoAlias on return values.
ClosedPublic

Authored by sstefan1 on Jun 9 2019, 3:50 PM.

Details

Summary

Porting function return value attribute noalias to attributor.
This will be followed with a patch for callsite and function argumets.

Diff Detail

Repository
rL LLVM

Event Timeline

sstefan1 created this revision.Jun 9 2019, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2019, 3:50 PM
sstefan1 retitled this revision from NoAlias on return values. to [Attributor] NoAlias on return values..Jun 9 2019, 3:56 PM
jdoerfert added inline comments.Jun 9 2019, 8:23 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
657 ↗(On Diff #203759)

"does not alias." is confusing, maybe phrase it is "noalias".

llvm/lib/Transforms/IPO/Attributor.cpp
364 ↗(On Diff #203759)

Use the AAReturnedValuesImpl instead of an explicit traversal. The AAReturnedValuesImpl knows what is returned and through which return instructions.

371 ↗(On Diff #203759)

Move this check in the initialize method.

380 ↗(On Diff #203759)

This is not what we want to check. Look at the returned values and the locations where they are returned.

sstefan1 updated this revision to Diff 203953.Jun 10 2019, 6:33 PM
  • Addressing comments.
  • Updated test cases in nonnull.ll
  • Added noalias.ll which, for now, has only one test case. More will be added with other patches regarding noalias attribute.

Why should the return values in nonnull.ll be noalias?

Please also add more explicit noalias return value tests.

llvm/lib/Transforms/IPO/Attributor.cpp
707 ↗(On Diff #203953)

Why a pessimistic fixpoint? It should be optimistic, right?

723 ↗(On Diff #203953)

In short:
for (auto &It : *AARetValImpl)

725 ↗(On Diff #203953)

Make it a const reference, thus add &

731 ↗(On Diff #203953)

Stores capture, so make the last one a true, also use the /* argumentname */ syntax to explain the booleans.

737 ↗(On Diff #203953)

You want to check RV here not the return instructions it can be returned through. The latter is only interesting to improve the escape query above but that is future work.

738 ↗(On Diff #203953)

The call site should be checked separately, e.g. before you ask for the AANoAlias attribute for RV. If RV is a call site and is returnDoesNotAlias all is good. Otherwise, we look at the AANoAlias. That should actually suffice (no Returned needed) and making it more general might make the code potentially more applicable in the future.

903 ↗(On Diff #203953)

Add the Whitelist check please.

llvm/test/Transforms/FunctionAttrs/noalias.ll
1 ↗(On Diff #203953)

Can you rename the file to noalias_returned.ll or returned_noalias.ll and also explicitly enable the attributor via -attributor-disable=false

sstefan1 marked an inline comment as done.Jun 11 2019, 7:15 AM

Why should the return values in nonnull.ll be noalias?

All functions returning pointer never actually capture it, or they return null. Isn't that enough?

llvm/lib/Transforms/IPO/Attributor.cpp
707 ↗(On Diff #203953)

Yes, don't know how it happened.

Why should the return values in nonnull.ll be noalias?

All functions returning pointer never actually capture it, or they return null. Isn't that enough?

This doesn't seem sufficient to me.
Consider:

file1.c
int* getter();
int* callee0() { // will get marked as noalias
  return getter();
}
int* callee1() { // will get marked as noalias
  return getter();
}
int entry() {
  int* p0 = callee0();
  int* p1 = callee1();
  // do p0 and p1 alias?
}

file2.c
int* global;
int* getter() { return global; } // oops

Why should the return values in nonnull.ll be noalias?

All functions returning pointer never actually capture it, or they return null. Isn't that enough?

This doesn't seem sufficient to me.
Consider:

file1.c
int* getter();
int* callee0() { // will get marked as noalias
  return getter();
}
int* callee1() { // will get marked as noalias
  return getter();
}
int entry() {
  int* p0 = callee0();
  int* p1 = callee1();
  // do p0 and p1 alias?
}

file2.c
int* global;
int* getter() { return global; } // oops

The problem is the pointers returned need to be noalias return pointers as well (that is the missing sufficient condition).

sstefan1 updated this revision to Diff 204086.Jun 11 2019, 9:25 AM

addressing comments.

sstefan1 marked an inline comment as done.Jun 11 2019, 9:26 AM
sstefan1 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
903 ↗(On Diff #203953)

I added the check here, since all noalias attrs will have same id, and both returned and noalias returned are only one per fucntion.

More comments. Can you diff it against the master branch so one can easily see all changes?

llvm/lib/Transforms/IPO/Attributor.cpp
738 ↗(On Diff #203953)

This was not correct. You need to check for a call here explicitly but you want to check RV.

903 ↗(On Diff #203953)

The Whitelist check for AANoAliasReturned should be separate and look for AANoAliasReturned::ID.

728 ↗(On Diff #204086)

Please add /* ReturnCaptures */ and a comment (TODO or FIXME) that mentions we could improve the capture check in two ways. 1) Use the AANoCapture facilities, 2) use the location of the associated return insts.

(I just saw, part of the FIXME is down there already!)

733 ↗(On Diff #204086)

I think you should move a check before the capture check that makes sure we actually have a call site here (for now, thus add a FIXME to support more). If we do not have a call site, we can for now not derive noalias for the return value.

sstefan1 updated this revision to Diff 204211.Jun 11 2019, 8:12 PM

fixed diff, addressed comments

We need more test cases, e.g. as the one below which shows some limits of the current impl.

#include <stdlib.h>

void *baz();
void *foo(int a);

void *bar()  {
  foo(0);
  return baz();
}

void *foo(int a)  {
  if (a)
    bar();
  return malloc(4);
}
llvm/lib/Transforms/IPO/Attributor.cpp
716 ↗(On Diff #204211)

You have to actually change the status to indicate a pessimistic fixpoint.

725 ↗(On Diff #204211)

You need to actually give up if RV is not a call site, so if ICS is null.

llvm/test/Transforms/FunctionAttrs/noalias_returned.ll
33 ↗(On Diff #204211)

No need for this check line, I think.

sstefan1 updated this revision to Diff 204410.EditedJun 12 2019, 8:19 PM

We need more test cases, e.g. as the one below which shows some limits of the current impl.

I added just one, besides your example, will add more soon.

  • small changes
  • added some tests.
jdoerfert added inline comments.Jun 18 2019, 1:23 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
592 ↗(On Diff #204410)

is "noalias"

llvm/lib/Transforms/IPO/Attributor.cpp
677 ↗(On Diff #204410)

No need to overload getDeducedAttributes. The default one will do the right thing.

sstefan1 updated this revision to Diff 210971.Jul 20 2019, 1:36 PM

Rebased and added more tests.

sstefan1 updated this revision to Diff 210972.Jul 20 2019, 1:37 PM
  • removing unnecessary llvm_debug
jdoerfert accepted this revision.Jul 20 2019, 7:37 PM

Fix one comment and Test 5, no check lines and unclear what it is supposed to test. Maybe just remove it.

LGTM otherwise.

llvm/include/llvm/Transforms/IPO/Attributor.h
795 ↗(On Diff #210972)

is noalias

This revision is now accepted and ready to land.Jul 20 2019, 7:37 PM
Closed by commit rL366728: [Attributor] NoAlias on return values. (authored by sstefan, committed by ). · Explain WhyMon, Jul 22, 12:35 PM
This revision was automatically updated to reflect the committed changes.