[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:58, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
>> 
>> On Sep 13, 2016, at 4:52 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> 
>>> 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.
> 
> Yes, I’m using a debug build to bugpoint the test and hit this :)
> 
> 
> 
>> 
>>> +          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).
>> 
> 
> I first update the LLParser to the new API, but it hasn’t loaded all the Metadata yet, so it gets a temporary node.

It can still use this API; just inline the new definition of
UpgradeInstWithTBAATag into LLParser and delete it from AutoUpgrade.cpp.