Page MenuHomePhabricator

[Attributor] AAReachability : use isPotentiallyReachable in isKnownReachable
ClosedPublic

Authored by okura on Sun, Mar 15, 9:29 PM.

Details

Summary

isKnownReachable had only interface (always returns true).
Changed it to call isPotentiallyReachable.
This change enables deductions of other Abstract Attributes depending on AAReachability to use reachability information obtained from CFG, and it can make them stronger.

Diff Detail

Event Timeline

okura created this revision.Sun, Mar 15, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Mar 15, 9:29 PM
okura updated this revision to Diff 250462.Sun, Mar 15, 9:31 PM

add and modify tests

uenoku added a comment.EditedSun, Mar 15, 10:08 PM

Thank you for working on this! Please add isKnownReachable change.

jdoerfert added inline comments.Sun, Mar 15, 10:23 PM
llvm/test/Transforms/Attributor/noalias.ll
424

I love how this all comes together ;)

okura updated this revision to Diff 250477.Sun, Mar 15, 10:38 PM

I'm sorry for my mistake. I uploaded two disjoint patches separately.
I added all of the changes.

Please upload full context of the diff (diff -U999999)

okura updated this revision to Diff 250487.Mon, Mar 16, 12:01 AM

I'm sorry. added full context diff.

I'm sorry. added full context diff.

No worries. I still have the feeling something is missing in this diff. Make sure to squash the commits locally or diff against the proper baseline.

void use(int *);
__attribute__((noinline)) static void f1(int * restrict p, int c1, int c2) {
  if (c1) {
    use(p);
  }
  if (!c2) {
    use(p);
  }
}
void f2(int * restrict p, int c) { f1(p, c, c); }

Could you add this example to test?
In this case, c1 and c2 are always same. Because of flow sensitivity, the first callsite is not reachable to the second callsite. So %p is noalias at both callsites. It can't be deduced with isPotentiallyReachable so please add FIXME tag.

okura updated this revision to Diff 250536.Mon, Mar 16, 5:43 AM

I added a suggested test.
I squashed all commits to one commit and got diff by git diff -U999999 HEAD^ (including commit for a new test)

okura added a comment.Mon, Mar 16, 5:49 AM

I'm sorry. added full context diff.

No worries. I still have the feeling something is missing in this diff. Make sure to squash the commits locally or diff against the proper baseline.

I squashed 2 commits and got diff. But that makes no difference from the previous diff. Therefore the latest diff may be still inappropriate. If so, please notify me.

okura added a comment.EditedMon, Mar 16, 6:07 AM
void use(int *);
__attribute__((noinline)) static void f1(int * restrict p, int c1, int c2) {
  if (c1) {
    use(p);
  }
  if (!c2) {
    use(p);
  }
}
void f2(int * restrict p, int c) { f1(p, c, c); }

Could you add this example to test?
In this case, c1 and c2 are always same. Because of flow sensitivity, the first callsite is not reachable to the second callsite. So %p is noalias at both callsites. It can't be deduced with isPotentiallyReachable so please add FIXME tag.

This makes sense to me, thank you for your suggestion. I added a slightly different test.
If function use does not make any alias of p (such as only_store in test), noalias can be deduced with the current implementation. But if we impose any constraints on use, noalias should not be deduced.
Therefore, I add a test corresponding to the following C program.

__attribute__((noinline)) static void test16_sub(int * restrict p, int c1, int c2) {
  if (c1) {
    only_store(p);
    make_alias(p);
  }
  if (!c2) {
    only_store(p);
  }
}
void test16_caller(int * restrict p, int c) {
  test16_sub(p, c, c);
}

Like as your test, %p is noalias at both calllsites of only_store. But it is currently not deduced for the second callsite.

uenoku added inline comments.Tue, Mar 17, 11:20 PM
llvm/test/Transforms/Attributor/noalias.ll
410

Please add CHECK-LABEL to function definitions just for testing purposes.

There are two issues with a isPotentiallyReachable approach, not to say we cannot make it work. What we need is 1) use liveness information from AAIsDead 2) caching of the results. We can do that in follow up patches though.

okura added inline comments.Thu, Mar 19, 3:41 AM
llvm/test/Transforms/Attributor/noalias.ll
410

Do I have to add CHECK-LABEL to all function definitions?

okura added a comment.Thu, Mar 19, 3:54 AM

There are two issues with a isPotentiallyReachable approach, not to say we cannot make it work. What we need is 1) use liveness information from AAIsDead 2) caching of the results. We can do that in follow up patches though.

I understand the issues. I will try to settle them.

uenoku added inline comments.Thu, Mar 19, 4:04 AM
llvm/test/Transforms/Attributor/noalias.ll
410

No, only to functions with tests.

okura updated this revision to Diff 251363.Thu, Mar 19, 6:19 AM

add some CHECK-LABEL directive

jdoerfert accepted this revision.Thu, Mar 19, 9:17 PM

I think we should merge this and work on the issues in a follow up. LGTM.

This revision is now accepted and ready to land.Thu, Mar 19, 9:17 PM

Can you please rebase this, doesn't apply cleanly anymore. Sorry for the wait. @uenoku @sstefan1 you can also commit Attributor patches for other people ;)

Nevermind, I merged it.

This revision was automatically updated to reflect the committed changes.