[PATCH] D73679: [MemoryBuiltins] Determine the size of a global w/o initializer

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv marked an inline comment as done.
george.burgess.iv added inline comments.


================
Comment at: llvm/test/Analysis/BasicAA/bug.23540.ll:11
 ; CHECK-LABEL: f
-; CHECK: MayAlias: i32* %arrayidx, i32* %arrayidx6
+; CHECK: PartialAlias: i32* %arrayidx, i32* %arrayidx6
 define void @f() {
----------------
jdoerfert wrote:
> george.burgess.iv wrote:
> > jdoerfert wrote:
> > > george.burgess.iv wrote:
> > > > Can you expand a bit on why PartialAlias is the correct answer here, please?
> > > > 
> > > > I'd naively expect this to be either MustAlias (both pointers are to a 32 bit object, and there're only 32 bits in this allocation; derefing anything else is UB), or MayAlias (there're only 32 bits here, but maaaaybe you're pointing to `((int*)&c + 1)`, and that's OK.)
> > > My reasoning is that `arrayidx6` points right after `c` and `arrayidx` points somewhere based on `c`. There are no accesses so the alias query is done with an unspecified size on the access part which should mean the two accesses can very well overlap. 
> > I don't quite agree with the unspecified size part: if the access size were entirely unknown, we could only correctly return `PartialAlias` in cases with provably identical object offsets, since `PartialAlias` *guarantees* the two accesses will overlap somehow.
> > 
> > Experimentally, we report `NoAlias` for `(%b, %c)` in:
> > ```
> > define void @f() {
> >   %a = alloca i32
> >   %b = bitcast i32* %a to i8*
> >   %c = getelementptr i8, i8* %b, i64 1
> >   ret void
> > }
> > ```
> > 
> > So I think the sizes here should be 4 bytes. If that's the case, `MustAlias` seems like the optimal answer, which means that `PartialAlias` is also correct.  `sizeof(access) == sizeof(underlying_object)` in both cases means there's really no room for nonzero object offsets. :)
> > 
> > If this sounds correct, please update the newly-added comment.
> So I added a `d = gep %b, undef` which is closer to the test case that changed and which results in a `MayAlias`: https://godbolt.org/z/KT4F5z
> 
> I don't follow the `MustAlias` argument. There is no access here, so we talk about pointers only. 
> The pointers are based on the same thing with an unknown offset from each other (at least that seems to me how `undef` is interpreted here).
> 
> I'll come up with more tests asap.
> There is no access here, so we talk about pointers only.

Correct; I was just interpreting

> There are no accesses so the alias query is done with an unspecified size on the access part

as "the alias queries get handed `LocationSize`s that don't `hasValue()`." If the sizes were unspecified, we'd get `MayAlias` instead of `NoAlias` for the `(%b, %c)` query on your godbolt link.

The `MustAlias` argument is based on this, but is honestly more of a tangent than anything. Referring again to the godbolt link, the only derefable (in a well-defined way) `i32*` we can form from `%a` must point directly to `%a`. So if we have two `i32*`s based on `%a`, ISTM inferring `MustAlias` would be valid, since `MustAlias` doesn't imply pointer equality.

In any case, we both now agree that `PartialAlias` is OK here, so I'm happy to call this resolved unless you wanna dive further into this :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73679/new/

https://reviews.llvm.org/D73679