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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added a subscriber: theraven.
arichardson added a comment.

In D100717#2698680 <https://reviews.llvm.org/D100717#2698680>, @jdoerfert wrote:

> First thought: Why is there no test in which we generate no `int2ptr` with the patch but do without?
>
>> @fhahn: I don't see how this use of TBAA metadata is within the specification of the metadata.
>
> I read through the code and I'm not sure if this is even a semantic change. I mean, can't we pick any
> type to do the memory transfer expansion? If so, TBAA metadata as a heuristic should be totally fine.
> We could also look at uses of the source and target pointers, for example. That said, we should write
> in the lang ref that we do use the TBAA names for heuristics and they should be chosen to match the code/intent.

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



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:177
+  Type* DataType = nullptr;
+  if (CopyMD && CopyMD->isTBAAPointerAccess() && SrcAddrSp == DstAddrSp &&
+      Size * 8 == DL.getPointerSizeInBits(SrcAddrSp)) {
----------------
I wonder if it would make sense to default to pointer load+store for non-integral address space?
For CHERI we would definitely want this to be the default, but we can carry that patch downstream.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:178
+  if (CopyMD && CopyMD->isTBAAPointerAccess() && SrcAddrSp == DstAddrSp &&
+      Size * 8 == DL.getPointerSizeInBits(SrcAddrSp)) {
+    // TBAA tag says this is a pointer type; follow the instruction to avoid
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100717