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

[Michael Ferguson via Phabricator via llvm-commits]

mppf added a comment.

>> 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).

Thanks.


https://reviews.llvm.org/D38499