[PATCH] D111023: [ConstantFold] Refactor load folding

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/Transforms/InstSimplify/ConstProp/loads.ll:34-38
+; LE-LABEL: @test2_addrspacecast(
+; LE-NEXT:    ret i16 -16657
+;
+; BE-LABEL: @test2_addrspacecast(
+; BE-NEXT:    ret i16 -8531
----------------
yrouban wrote:
> Nikita, Artur, Philip,
> I think this this test is not correct.
> As I found in our internal sources the method //stripPointerCastsAndOffsets()// does not allow to collect offsets through addrspacecasts. Here is the exact wording by Philip Reames:
> 
>       // NOTE: Looking through a addrspacecast is not legal here since it would
>       // change address spaces (and thus possibly pointer sizes)
> 
> I found that the LangRef specifies that the memory location is preserved through addrspacecasts. But nothing is said about sizes. What if we have big-endian in addrspace(0) and little-endian in addrspace(1)?
> 
> If you agree, I can upstream the one line change in Value.cpp that stops offset calculation at addrspacecasts.
> 
> 
> 
> 
> 
I don't know whether it's okay to look through addrspacecasts in that method -- the exact semantics of addrspacecasts are not well-specified. The implementation at least is quite careful about dealing with different-width address spaces, so someone clearly thought it was okay at some point. Note that we also accumulate offsets across different address spaces in BasicAA. (I would actually be very happy if that is incorrect and we can drop it, because it causes a large amount of complexity. Just don't know whether that's the case or not.)

> I found that the LangRef specifies that the memory location is preserved through addrspacecasts. But nothing is said about sizes. What if we have big-endian in addrspace(0) and little-endian in addrspace(1)?

This is impossible, because LLVM does not support specifying endianness per address space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111023