Page MenuHomePhabricator

[Attributor] AAReachability : use isPotentiallyReachable in isKnownReachable
ClosedPublic

Authored by okura on Mar 15 2020, 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.Mar 15 2020, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2020, 9:29 PM
okura updated this revision to Diff 250462.Mar 15 2020, 9:31 PM

add and modify tests

uenoku added a comment.EditedMar 15 2020, 10:08 PM

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

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

I love how this all comes together ;)

okura updated this revision to Diff 250477.Mar 15 2020, 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.Mar 16 2020, 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.Mar 16 2020, 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.Mar 16 2020, 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.EditedMar 16 2020, 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.Mar 17 2020, 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.Mar 19 2020, 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.Mar 19 2020, 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.Mar 19 2020, 4:04 AM
llvm/test/Transforms/Attributor/noalias.ll
410

No, only to functions with tests.

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

add some CHECK-LABEL directive

jdoerfert accepted this revision.Mar 19 2020, 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.Mar 19 2020, 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.
jroelofs added inline comments.
llvm/test/Transforms/Attributor/noalias.ll
460

@okura @jdoerfert This diff added some checks with missing FileCheck colons, which silently hide the fact that these checks don't match the tool output.

Mind taking a look at this fixup patch? https://gist.github.com/jroelofs/f03ae725c6b9a5c80f9f187b80642f79

okura marked an inline comment as done.Apr 15 2020, 6:43 AM
okura added inline comments.
llvm/test/Transforms/Attributor/noalias.ll
460

That is my terrible mistake. I am sorry and thank you for the patch.
I confirmed it passes the test and It looks good to me.

jroelofs added inline comments.Apr 15 2020, 11:27 AM
llvm/test/Transforms/Attributor/noalias.ll
460

no worries, thanks for having a look!

6c9d52885deaddebe7c228392be20948f413a22f