This is an archive of the discontinued LLVM Phabricator instance.

[CFLAA] Remove redundant aliasing relation between GEP indices and GEP result
ClosedPublic

Authored by grievejia on May 28 2016, 1:20 PM.

Details

Summary

As shown in the diff, if cfl-aa encounters a GEP instructions y = gep x, z, we used to treat y and z as potential aliases. However, according to the "Pointer Aliasing Rule" section of LLVM Language Reference, this decision turns out to be overly defensive.

Diff Detail

Repository
rL LLVM

Event Timeline

grievejia updated this revision to Diff 58902.May 28 2016, 1:20 PM
grievejia retitled this revision from to [CFLAA] Remove redundant aliasing relation between GEP indices and GEP result.
grievejia updated this object.
grievejia added a subscriber: llvm-commits.

Hey Jia,
You need to add testcases for this :)

Hey Jia,
You need to add testcases for this :)

I tried and found it pretty hard to come up with a test case that can target this particular patch specifically.

The issue here is that the gep indices must be integers. To test for aliasing relation between a gep result and another pointer that acts like a gep index, there must be a ptr-to-int cast or an int-to-ptr cast. Cfl-aa is currently very conservative about both inttoptr and ptrtoint. Therefore as long as the inttoptr/ptrtoint handling stays the same, it's not possible for this patch to alter the behavior of cfl-aa in any observable way!

I'll try to upload another patch later that fix this issue by making cfl-aa treats ptrtoint as a no-op. Is it ok if I attach the test cases to that patch instead?

I would think (read: haven't tried it yet :) ) that something like:

define void @foo(i32 %n) {
  %a = alloca [2 x i32], align 4
  %b = alloca [2 x i32], align 4
  %c = getelementptr inbounds [2 x i32]* %a, i32 0, i32 %n
  %d = getelementptr inbounds [2 x i32]* %b, i32 0, i32 %n
  ret void
}

Would work as a test case for this (used to alias before, doesn't alias now). Does it not?

Good point!

I start to wonder why I haven't thought about this earlier...

grievejia updated this revision to Diff 58994.May 30 2016, 1:46 PM
grievejia edited edge metadata.

Added test case

george.burgess.iv edited edge metadata.

LGTM; will commit. Thanks for the patch :)

This revision is now accepted and ready to land.May 31 2016, 12:56 PM
This revision was automatically updated to reflect the committed changes.