[PATCH] D100717: [InstCombine] Transform memcpy to ptr load/stores if TBAA says so

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

Thank you for the comments. I'll update this patch to contain tests that were creating inttoptr before.

In D100717#2707333 <https://reviews.llvm.org/D100717#2707333>, @jeroen.dobbelaere wrote:

> It is done for vtable pointers, but that is only used by the thread sanitizer. There was discussion about using the available information in tbaa for guiding the type to load/store (1) vs forcing to use tbaa to guide what kind of load/stores are used. (2)
>
> (1) is deemed to be acceptable, if it is clearly documented. The forced naming based on what names clang produces is unfortunate, but as a stopgap this would be acceptable until we come up with a cleaner mechanism.
> That cleaner mechanism could be part of the rework of `new struct path tbaa` (that was also discussed at the meeting).
>
> For (2) we don't have a good idea yet. Maybe a separate `!tb.struct` with the same layout could be used for this particular purpose if `!tbaa.struct` is not available (just thinking out loud).

I believe this patch falls into category (1) - this patch is mainly for removal of inttoptrs created by middle-end transformations, helping deletion of `inttoptr(ptrtoint p) -> p` fold.

I'll create a LangRef patch that describes the policy & share it to llvm-dev tomorrow.

In D100717#2699004 <https://reviews.llvm.org/D100717#2699004>, @arichardson wrote:

> For CHERI/Morello the type of the memory transfer does matter. For us pointers are 65/129-bit capabilities (32/64-bit addresses, bounds & permission metadata, and a hidden validity bit). The hardware clears the validity bit when storing an integer to a memory location that holds a valid capability. So if we convert a pointer-type memcpy to a i64/i128 load+store the pointer becomes invalid and will trigger in a trap on next dereference.
> We therefore have to use pointer-type loads+stores as the default if the size matches the pointer size and pointers in that AS are capabilities.
> In our fork this can be checked using `DL.isFatPointer(AS)` (a weaker version of `DL.isNonIntegralAddressSpace(AS)` that allows `ptrtoint`, but not `inttoptr`).
>
> A few years ago @theraven added a workaround to our fork to allow the `memcpy->load+store` transformation for 128-bit CHERI (since it is guarded by Size <=8): https://github.com/CTSRD-CHERI/llvm-project/commit/f477ef4d292eb45871fc6cab0262d0844ce5d5aa

This is really interesting! IIUC my patch will bring benefit to this case as well? `cheri-memcpy.ll` also uses `!tbaa.struct` and my patch preserves the address space of the pointer type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100717