<div dir="ltr">I think I raised another option early in the review that was never answered <a href="http://reviews.llvm.org/D17884">http://reviews.llvm.org/D17884</a> <br><br>"<span style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px">I think the fix could probably happen at a higher level to avoid the linear</span><br style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px"><span style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px">search. Perhaps we could get the canonical decl for a using decl - that way</span><br style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px"><span style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px">a redecl wouldn't be a 'new' thing? I suppose we'd still need a map of</span><br style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px"><span style="color:rgb(0,0,0);font-family:'Segoe UI','Segoe UI Web Regular','Segoe UI Symbol','Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.85px">them, that perhaps we don't have already."<br><br>But yeah, generally I think it'd be better to move this up rather than down - but I'm not too fussed/open to either.</span></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 18, 2016 at 7:15 AM, Duncan P. N. Exon Smith via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2016-Apr-18, at 07:09, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
><br>
>> On 2016-Apr-18, at 06:12, Aboud, Amjad <<a href="mailto:amjad.aboud@intel.com">amjad.aboud@intel.com</a>> wrote:<br>
>><br>
>> We thought about this direction by then too, but SetVector does implement the function emplace_back.<br>
>> If you have an idea on how to make it work with a simple change of the " AllImportedModules" container type, it would be great.<br>
><br>
> Ah, right; TrackingMDNodeRef isn't stable in value so it doesn't<br>
> belong in a DenseSet, etc.<br>
><br>
> Did you consider filtering them in DIBuilder::finalize?<br>
> --<br>
> SmallPtrSet<const Metadata *, 16> AlreadyImported;<br>
> std::remove_if(AllImportedModules.begin(), AllImportedModules.end(),<br>
> [&AlreadyImported](const TrackingMDNodeRef &Ref) {<br>
> return !AlreadyImported.insert(Ref.get()).second;<br>
> });<br>
> --<br>
<br>
</span>Or just filter them directly into the ops vector:<br>
--<br>
SmallVector<const Metadata *, 16> Ops;<br>
<span class=""> SmallPtrSet<const Metadata *, 16> AlreadyImported;<br>
</span> std::remove_copy_if(AllImportedModules.begin(), AllImportedModules.end(),<br>
std::back_inserter(Ops),<br>
<span class=""> [&AlreadyImported](const TrackingMDNodeRef &Ref) {<br>
return !AlreadyImported.insert(Ref.get()).second;<br>
});<br>
</span> MDTuple *Imports = Ops.empty() ? nullptr : MDTuple::get(Context, Ops);<br>
<div class="HOEnZb"><div class="h5">--<br>
<br>
><br>
>><br>
>> Thanks,<br>
>> Amjad<br>
>><br>
>><br>
>>> -----Original Message-----<br>
>>> From: <a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a> [mailto:<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>]<br>
>>> Sent: Monday, April 18, 2016 09:18<br>
>>> To: Aboud, Amjad <<a href="mailto:amjad.aboud@intel.com">amjad.aboud@intel.com</a>><br>
>>> Cc: llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>>; <a href="mailto:hintonda@gmail.com">hintonda@gmail.com</a>; David<br>
>>> Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>> Subject: Re: [llvm] r263379 - Fixed DIBuilder to verify that same imported entity<br>
>>> will not be added twice to the "imports" list of the DICompileUnit.<br>
>>><br>
>>><br>
>>>> On 2016-Mar-13, at 04:11, Amjad Aboud via llvm-commits <llvm-<br>
>>> <a href="mailto:commits@lists.llvm.org">commits@lists.llvm.org</a>> wrote:<br>
>>>><br>
>>>> Author: aaboud<br>
>>>> Date: Sun Mar 13 06:11:39 2016<br>
>>>> New Revision: 263379<br>
>>>><br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263379&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=263379&view=rev</a><br>
>>>> Log:<br>
>>>> Fixed DIBuilder to verify that same imported entity will not be added twice to<br>
>>> the "imports" list of the DICompileUnit.<br>
>>><br>
>>> I don't like that this peeks into the LLVMContext, and it doesn't seem like it's<br>
>>> correct. See below. (Sorry for missing this when it went<br>
>>> by.)<br>
>>><br>
>>>><br>
>>>> Differential Revision: <a href="http://reviews.llvm.org/D17884" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17884</a><br>
>>>><br>
>>>> Modified:<br>
>>>> llvm/trunk/lib/IR/DIBuilder.cpp<br>
>>>> llvm/trunk/unittests/IR/IRBuilderTest.cpp<br>
>>>><br>
>>>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp<br>
>>>> URL:<br>
>>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?re" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?re</a><br>
>>>> v=263379&r1=263378&r2=263379&view=diff<br>
>>>><br>
>>> =================================================================<br>
>>> =====<br>
>>>> ========<br>
>>>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)<br>
>>>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Sun Mar 13 06:11:39 2016<br>
>>>> @@ -19,6 +19,7 @@<br>
>>>> #include "llvm/IR/Module.h"<br>
>>>> #include "llvm/Support/Debug.h"<br>
>>>> #include "llvm/Support/Dwarf.h"<br>
>>>> +#include "LLVMContextImpl.h"<br>
>>>><br>
>>>> using namespace llvm;<br>
>>>> using namespace llvm::dwarf;<br>
>>>> @@ -168,8 +169,12 @@ static DIImportedEntity *<br>
>>>> createImportedModule(LLVMContext &C, dwarf::Tag Tag, DIScope *Context,<br>
>>>> Metadata *NS, unsigned Line, StringRef Name,<br>
>>>> SmallVectorImpl<TrackingMDNodeRef><br>
>>>> &AllImportedModules) {<br>
>>>> + unsigned EntitiesCount = C.pImpl->DIImportedEntitys.size();<br>
>>>> auto *M = DIImportedEntity::get(C, Tag, Context, DINodeRef(NS),<br>
>>>> Line, Name);<br>
>>><br>
>>> What if `M` isn't a new node because some other llvm::Module (or even a<br>
>>> different llvm::DIBuilder instance in the same llvm::Module) has added a<br>
>>> structurally equivalent DIImportedEntity? You would still need it in<br>
>>> AllImportedModules, right? But this logic won't do that.<br>
>>><br>
>>>> - AllImportedModules.emplace_back(M);<br>
>>>> + if (EntitiesCount < C.pImpl->DIImportedEntitys.size())<br>
>>>> + // A new Imported Entity was just added to the context.<br>
>>>> + // Add it to the Imported Modules list.<br>
>>>> + AllImportedModules.emplace_back(M);<br>
>>><br>
>>> I think you just want to change AllImportedModules to a SetVector so that<br>
>>> they're uniqued on insert.<br>
>>><br>
>>>> return M;<br>
>>>> }<br>
>>>><br>
>>>><br>
>>>> Modified: llvm/trunk/unittests/IR/IRBuilderTest.cpp<br>
>>>> URL:<br>
>>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/IRBuilderT" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/IRBuilderT</a><br>
>>>> est.cpp?rev=263379&r1=263378&r2=263379&view=diff<br>
>>>><br>
>>> =================================================================<br>
>>> =====<br>
>>>> ========<br>
>>>> --- llvm/trunk/unittests/IR/IRBuilderTest.cpp (original)<br>
>>>> +++ llvm/trunk/unittests/IR/IRBuilderTest.cpp Sun Mar 13 06:11:39 2016<br>
>>>> @@ -418,4 +418,19 @@ TEST_F(IRBuilderTest, DebugLoc) {<br>
>>>><br>
>>>> DIB.finalize();<br>
>>>> }<br>
>>>> +<br>
>>>> +TEST_F(IRBuilderTest, DIImportedEntity) {<br>
>>>> + IRBuilder<> Builder(BB);<br>
>>>> + DIBuilder DIB(*M);<br>
>>>> + auto File = DIB.createFile("F.CBL", "/");<br>
>>>> + auto CU = DIB.createCompileUnit(dwarf::DW_LANG_Cobol74, "F.CBL", "/",<br>
>>>> + "llvm-cobol74", true, "", 0);<br>
>>>> + auto IE1 = DIB.createImportedDeclaration(CU, nullptr, 1);<br>
>>>> + auto IE2 = DIB.createImportedDeclaration(CU, nullptr, 1);<br>
>>>> + auto IE3 = DIB.createImportedModule(CU, (DIImportedEntity*)nullptr,<br>
>>>> +2);<br>
>>>> + auto IE4 = DIB.createImportedModule(CU, (DIImportedEntity*)nullptr,<br>
>>>> +2);<br>
>>>> + DIB.finalize();<br>
>>>> + EXPECT_TRUE(verifyModule(*M));<br>
>>>> + EXPECT_TRUE(CU->getImportedEntities().size() == 2); }<br>
>>>> }<br>
>>>><br>
>>>><br>
>>>> _______________________________________________<br>
>>>> llvm-commits mailing list<br>
>>>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>><br>
>> ---------------------------------------------------------------------<br>
>> Intel Israel (74) Limited<br>
>><br>
>> This e-mail and any attachments may contain confidential material for<br>
>> the sole use of the intended recipient(s). Any review or distribution<br>
>> by others is strictly prohibited. If you are not the intended<br>
>> recipient, please contact the sender and delete all copies.<br>
>><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>