[PATCH] D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types.

Tue Mar 29 14:01:24 PDT 2022

jcranmer-intel added a comment.

Apologies for the drive-by comment, but I happened to be searching for TBAA reviews after lamenting the current documentation and this popped up.

> Agreed, strict aliasing violations are already a problem with the current level of TBAA support and we regularly see mis-compiles when new optimizations get enabled due to violations in projects. Improving precision of TBAA annotation is likely to expose more violations But there are cases where the additional guarantees could make a difference and other compiler implementations make use of them, so I think it would be worthwhile to work towards improving TBAA. I think we've also been through other disruptive transitions, like more aggressively adding `mustprogress`, but those were probably on a somewhat smaller scale.
>
> Maybe we should try improve our tooling to detect violations in parallel to the effort in this patch? I am planning on trying to resurrect the type-sanitizer patches. I've also been playing around with a simple tool that's inserting assertions to check 2 pointers are not equal if TBAA claims they are no alias. The latter seems to surface a few violations in code in `llvm-test-suite` and also in tablegen. That is even without the current patch applied.

I think better tooling for TBAA in general is needed, although I'm not sure if this is a "in parallel" or "as a prerequisite step." One of the things I started doing myself is building a DOT output for TBAA metadata so that it's easier to understand the TBAA type tree without having to stare too hard at all of the metadata numbers. I don't want to derail this patch with too much discussion, but since it's slightly relevant here, one more comment:

================
Comment at: clang/lib/CodeGen/CodeGenTBAA.cpp:200-203
+      StringRef Name =
+          cast<llvm::MDString>(
+              ScalarMD->getOperand(CodeGenOpts.NewStructPathTBAA ? 2 : 0))
+              ->getString();
----------------
Seeing this code reminds me that in a few other contexts, it would be useful to have access to the TBAA wrapper helpers in `llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp`, so it would be worthwhile at some point to start fronting those types into header files to simplify some of the queries, especially in cases where you want to support both old and new TBAA formats. I'd imagine your tbaa-checker tool would also want access to this information as well, for example; I know the helper I started writing wanted it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122573