[PATCH] D95826: [SROA] Propagate correct TBAA/TBAA Struct offsets

[William Moses via Phabricator via llvm-commits]

wsmoses added a comment.

Very much agreed on someone with more TBAA experience looking over.

I'm not sure that I have the same hunch about nested structs. We only need to modify the offset in the root node so I don't immediately see how nesting presents an issue. Mind elaborating on what you think could go wrong?



================
Comment at: llvm/include/llvm/IR/Metadata.h:645
+MDNode *ShiftTBAAStruct(MDNode *M, size_t off);
+
 /// A collection of metadata nodes that might be associated with a
----------------
jdoerfert wrote:
> Any reason to expose this. If not, maybe make them static members in the AAMDNode struct, potentially hidden.
I can definitely see other users of such shifting, though if they always use AAMDNodes it will be encapsulated.

A public static method seems good to me.


================
Comment at: llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp:785
+    Sub.push_back(M->getOperand(i + 2));
+  }
+  return MDNode::get(M->getContext(), Sub);
----------------
jdoerfert wrote:
> shouldn't we break at the end?
No, we want to extract all of the relevant TBAA.struct pieces. In essence TBAA.struct is a list of triples saying the offset, size, type. There therefore may be several relevant triples.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3388
+      APInt Offset(
+          DL.getIndexSizeInBits(GEPToCompute->getPointerAddressSpace()), 0);
+      if (AATags && GEPToCompute->accumulateConstantOffset(DL, Offset))
----------------
jdoerfert wrote:
> No temporary GEPs please. What is wrong with GEP ?
The problem is that the GEP instruction itself may be constant folded by the IRBuilder (and thus why it only returns a value and not a gep instruction). 


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3442
+      APInt Offset(
+          DL.getIndexSizeInBits(GEPToCompute->getPointerAddressSpace()), 0);
+      if (AATags && GEPToCompute->accumulateConstantOffset(DL, Offset))
----------------
jdoerfert wrote:
> No temporary GEPs please. What is wrong with `InBoundsGEP` ?
Same reply as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95826