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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added reviewers: asbirlea, nikic, fhahn, jeroen.dobbelaere, kosarev.
jdoerfert added a comment.

We need a test case with nested structs as well, I somehow have the feeling that doesn't work properly rn.

Also some comments that seem straight forward. For the logic part, I hope someone else could take a look, I'd have to read up on struct TBAA.



================
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
----------------
Any reason to expose this. If not, maybe make them static members in the AAMDNode struct, potentially hidden.


================
Comment at: llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp:746
+    return M;
+  }
+
----------------
Style:
No braces.
`Offset` not `off`, same elsewhere.
`MD` (or similar) not `M`, same elsewhere.


================
Comment at: llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp:749
+  TBAAStructTagNode Tag(M);
+  SmallVector<Metadata *, 3> Sub;
+  Sub.push_back(M->getOperand(0));
----------------
you push back up to 4 elements, let's go with 4 here too.


================
Comment at: llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp:785
+    Sub.push_back(M->getOperand(i + 2));
+  }
+  return MDNode::get(M->getContext(), Sub);
----------------
shouldn't we break at the end?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3388
+      APInt Offset(
+          DL.getIndexSizeInBits(GEPToCompute->getPointerAddressSpace()), 0);
+      if (AATags && GEPToCompute->accumulateConstantOffset(DL, Offset))
----------------
No temporary GEPs please. What is wrong with GEP ?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95826