[PATCH] D27839: Strip invalid TBAA when reading bitcode

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:157
+    if (F.isMaterializable())
+      continue;
+    for (auto &I : instructions(F))
----------------
sanjoy wrote:
> mehdi_amini wrote:
> > sanjoy wrote:
> > > Why are you skipping these?  Would be nice to add a one-liner, unless it is patently obvious.
> > A function that is Materializable hasn't been materialized, so it does not have a body. It is not a declaration either as its body may be parsed later. Trying to iterate it results in an assertion.
> I think this use of `dropUnknownNonDebugMetadata` is wrong (and so is the usage in `CodeGenPrepare::stripInvariantGroupMetadata`).
> 
> The argument to `dropUnknownNonDebugMetadata` is the set of metadata IDs that are "known" and should not be dropped.  All non-debug metadata *not* in that set is dropped.
> 
> The reason why this works is that `TBAA->getMetadataID()` is an entirely different thing.  It is the IDs that are used to distinguish between `MDString`, `MDNode` etc. (the subclass id, basically, which also feeds into `dyn_cast` etc.).  Since that ID is not equal to `LLVMContext::MD_tbaa`, we end up dropping `MD_tbaa`.
> 
> The right usage here is: `I.setMetadata(LLVMContext::MD_tbaa, nullptr)`.
Uh... I took indeed example on the usage in CodeGenPrepare!
I'll switch to ` I.setMetadata(LLVMContext::MD_tbaa, nullptr).`


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:476
   bool StripDebugInfo = false;
+  bool StripTBAA = false;
+  TBAAVerifier TBAAVerifyHelper;
----------------
sanjoy wrote:
> mehdi_amini wrote:
> > sanjoy wrote:
> > > I'd have named this as `TBAAStripped` since it really indicates if we've already stripped out TBAA metadata (and hence there is no need to check this in for newer functions).  Right now this seems imperative, and it sounds like setting this to `true` will cause TBAA to be stripped.
> > > 
> > Actually it will also cause TBAA to be stripped :)
> > The first time you encounter an invalid TBAA, you set this boolean to request that from now on when parsing the body of a function, it has to ignore TBAA MD. 
> > It is also used line 4469 to see if we're in a mode where we strip TBAA during reading, so we don't need to check every instruction in the function.
> > Make sense?
> > The first time you encounter an invalid TBAA, you set this boolean to request that from now on when parsing the body of a function, it has to ignore TBAA MD. 
> 
> But that is not //this// flag is it?  I thought you were dropping future TBAA by setting the flag on `MetadataLoaderImpl`.
> 
> From what I can gather locally, if I manually `bool StripTBAA = false;` to `bool StripTBAA = true;` then I would not drop TBAA at all, and you're using this flag solely to remember if you've asked `MDLoader` to not load new TBAA, and so you don't have to walk `instructions(F)`.  In that sense, it isn't imperative, it just "remembers" if a certain action was done in the past.
> 
> However, I'm not cognizant of the inter-relationships between these classes that you changed, so I may be missing the forest for the trees.
Indeed it is redundant with the state on the MetadataLoader. I think I can remove it from here and add a getter on the MetadataLoader. You'll tell me with the next update if it helps :)


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1347
+          if (StripTBAA)
+            break;
           assert(!MD->isTemporary() && "should load MDs before attachments");
----------------
sanjoy wrote:
> mehdi_amini wrote:
> > sanjoy wrote:
> > > Should this not `continue` (otherwise you'll drop all metadata once you've seen `!tbaa`)?
> > > 
> > > Actually, I'd rather write this out separately as:
> > > 
> > > ```
> > > if (I->second == LLVMContext::MD_tbaa && StripTBAA)
> > >   continue;
> > > ```
> > > 
> > > right after the `if (!MD)` check.
> > It is breaking out of the switch. I'll move the check above.
> Right, but isn't it also breaking out of the innermost `for`?
Right, I missed the inner for! I was looking at the break that follows a few lines below, but missed the fact that we had an enclosing for.


https://reviews.llvm.org/D27839