[PATCH] D24533: Fix auto-upgrade of TBAA tags in Bitcode Reader

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Sep-13, at 16:37, Mehdi AMINI <mehdi.amini at apple.com> wrote:
> 
> mehdi_amini added a comment.
> 
> Sorry forgot the test case, will update soon!
> 
> 
> https://reviews.llvm.org/D24533

> Index: lib/Bitcode/Reader/BitcodeReader.cpp
> ===================================================================
> --- lib/Bitcode/Reader/BitcodeReader.cpp
> +++ lib/Bitcode/Reader/BitcodeReader.cpp
> @@ -4404,11 +4403,11 @@
>          if (HasSeenOldLoopTags && I->second == LLVMContext::MD_loop)
>            MD = upgradeInstructionLoopAttachment(*MD);
>  
> -        Inst->setMetadata(I->second, MD);
>          if (I->second == LLVMContext::MD_tbaa) {
> -          InstsWithTBAATag.push_back(Inst);
> -          continue;
> +          assert(!isa<TempMDTuple>() && "should load metadata before attachment");

I don't think this compiles (perhaps you didn't try an asserts build?).
Two reasons: isa<>() takes an argument, and TempMDTuple is defined
something like this:
--
typedef std::unique_ptr<MDTuple, SomeDeleter> TempMDTuple;
--
I think you should call 'MD->isTemporary()' instead.

> +          MD = UpgradeTBAANode(Inst->getContext(), MD);
>          }
> +        Inst->setMetadata(I->second, MD);
>        }
>        break;
>      }
> Index: include/llvm/IR/AutoUpgrade.h
> ===================================================================
> --- include/llvm/IR/AutoUpgrade.h
> +++ include/llvm/IR/AutoUpgrade.h
> @@ -51,6 +52,11 @@
>    /// module is modified.
>    bool UpgradeModuleFlags(Module &M);
>  
> +  /// If the given TBAA tag uses the scalar TBAA format, create a new node
> +  /// corresponding to the upgrade to the struct-path aware TBAA format.
> +  /// Otherwise return the \p TBAANode itself.
> +  MDNode *UpgradeTBAANode(LLVMContext &Context, MDNode *TBAANode);
> +
>    /// If the TBAA tag for the given instruction uses the scalar TBAA format,
>    /// we upgrade it to the struct-path aware TBAA format.
>    void UpgradeInstWithTBAATag(Instruction *I);

Do we need this still?  I know there's a caller in LLParser, but that
could also be updated to use this API.  Even better, we could delete the
upgrade logic from the LLParser entirely, given that we don't support
reading old textual IR (that would be a separate commit though).

>