[PATCH] D26635: [TBAA] Don't generate invalid TBAA when merging nodes

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:462
+  // the root node.  In either case, we don't have any useful TBAA
+  // metadata to attach.
+  if (!Ret || Ret->getNumOperands() < 2)
----------------
manmanren wrote:
> sanjoy wrote:
> > manmanren wrote:
> > > When the only common base is the root node, is it illegal to generate a tag based on the root?
> > The comment at the top of the file states:
> > 
> > ```
> > The second field is the access type node, it
> > // must be a scalar type node.
> > ```
> I see. I was wondering if it caused any compilation failure or miscompile.
> 
I don't think today it would lead to an actual miscompile.  It just breaks the spec and hence the verifier.


================
Comment at: unittests/Analysis/TBAATest.cpp:75
+  auto *RootMD = MD.createTBAARoot("tbaa-root");
+  auto *MD1 = MD.createTBAANode("scalar-a", RootMD);
+  auto *StructTag1 = MD.createTBAAStructTagNode(MD1, MD1, 0);
----------------
manmanren wrote:
> sanjoy wrote:
> > manmanren wrote:
> > > Why are we testing the old scalar TBAA here with createTBAANode?
> > The `OldTBAATest` name is misleading here, I'll change it to be more generic.  However, the only thing we've deprecated and removed support is the using scalar tbaa *tags*.  So far I don't see anything in the TBAA implementation that indicates we don't support scalar TBAA *nodes*.  For instance, see line 247:
> > 
> > ```
> >     // Fast path for a scalar type node and a struct type node with a single
> >     // field.
> >     if (Node->getNumOperands() <= 3) {
> >       uint64_t Cur = Node->getNumOperands() == 2
> >                          ? 0
> >                          : mdconst::extract<ConstantInt>(Node->getOperand(2))
> >                                ->getZExtValue();
> >       Offset -= Cur;
> >       MDNode *P = dyn_cast_or_null<MDNode>(Node->getOperand(1));
> >       if (!P)
> >         return TBAAStructTypeNode();
> >       return TBAAStructTypeNode(P);
> >     }
> > ```
> > 
> > Even if removing scalar TBAA nodes completely is going to ultimately be a better final state (I think it will be), the amount of work is non-trivial I'd rather not fold in all of that work in this cleanup work I'm currently doing.
> We have createTBAAScalarTypeNode and it is used by clang currently to generate a scalar type node.
> createTBAANode was used by clang with the old TBAA scalar system.
Yes, but there are out of tree non-clang users who use `createTBAANode` today.

I agree that it would be nice to get rid of scalar TBAA completely, but I don't want to change too much at once.  In particular, I think once D26438 is checked in (I'm also waiting for you to take a look at that one, btw :) ) it will likely break out of tree users since today it is too easy to generate slightly malformed TBAA and not realize it[0].  I want to check in D26438 first, let that bake in for some time, and then ditch scalar tbaa once out of tree users have recovered from D26438.

[0]: For instance, we were generating bad TBAA internally for a while, and things actually worked okay till one day they didn't.



https://reviews.llvm.org/D26635