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

[Mehdi Amini via llvm-commits]

> 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.

— 
Mehdi