[PATCH] D38499: [BasicAA] Fix adjustToPointerSize in BasicAliasAnalysis.cpp for ptr > 64b

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D38499#888784, @mppf wrote:

> >> In my use case, I have 128-bit pointers but "adding" to one of these will only ever modify the lower 64 bits. So for my use case, offsets > 64 bits just don't matter.
>
>
>
> > That's completely different.
>
>
>
> > ... we have a bunch of code which actually assumes it represents two's complement arithmetic in the width of the pointer; for example, instcombine has a transform which turns a pointer comparison into a comparison of pointer offsets, and some analysis passes use an APInt whose width is the pointer width rather than the int64_t AA uses. If pointer arithmetic wraps differently on your target, we probably need to add an attribute to the data layout, and adjust LangRef to define what it means.
>
> I don't think that's necessary (for me at least) because it's OK for me to get undefined behavior / core dumps if one of these pointers actually overflows/underflows (assuming signed arithmetic). I.e. as long as adding 1, -1 or other small values works, that's good enough for me at the present time. In other words, you could assume that all my pointer operations are 'inbounds'. *technically* adding -1 causes overflow, if you look at it in the right way, but it happens in a way that plays well with truncation to a smaller sized pointer. My use-case involves a late optimization pass that changes these 128-bit pointers to {i64, 64-bit pointer} structures, at which point handling any necessary truncation is my problem. I'm saying that i64 arithmetic for my pointers is fine, but so is i128 arithmetic. I don't care what happens if you do something like load from NULL - 1.
>
> It's certainly the case that we *could* try to modify LangRef to define pointers with offsets of a different size than the pointer, and I *could* use such semantics. But we'd have to adjust every optimization. I just don't think it's called for right now.
>
> Remember, I'm looking for a bug fix?
>
> But anyway this leaves me wondering - what in particular do we need to do, from your point of view, to move this patch forward? There have been quite a few suggestions and I can't tell which ones would be satisfactory (or which ones would be my problem to solve).


I'm working on a patch for this (replacing essentially all of the internal computations with APInt). Currently, my patch causes Analysis/BasicAA/gep-and-alias.ll to fail. I think this might be a real bug (I've made what I believe is a 64-bit version of this test case and it fails on trunk too). I'm investigating...


https://reviews.llvm.org/D38499