[PATCH] D27839: Strip invalid TBAA when reading bitcode

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:51
 #include "llvm/IR/InlineAsm.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/InstrTypes.h"
----------------
I'd have mildly preferred doing the NFC header sorting in a separate change.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:157
+    if (F.isMaterializable())
+      continue;
+    for (auto &I : instructions(F))
----------------
Why are you skipping these?  Would be nice to add a one-liner, unless it is patently obvious.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:476
   bool StripDebugInfo = false;
+  bool StripTBAA = false;
+  TBAAVerifier TBAAVerifyHelper;
----------------
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.



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1347
+          if (StripTBAA)
+            break;
           assert(!MD->isTemporary() && "should load MDs before attachments");
----------------
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.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.h:58
 
+  // Set the mode to strip TBAA metadata on load
+  void setStripTBAA(bool StripTBAA = true);
----------------
End comment with period.  Also, why not `///` for doxygen?


https://reviews.llvm.org/D27839