<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 15, 2015 at 3:48 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
> On 2015 Feb 15, at 10:29, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Sun, Feb 15, 2015 at 10:22 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> Hi rafael, resistor, grosbach, chandlerc,<br>
><br>
> I've taken my best guess at this, but I've cargo culted in places & so<br>
> explanations/corrections would be great.<br>
><br>
> This seems to pass all the tests (check-all, covering clang and llvm) so I<br>
> believe that pretty well exercises both the backwards compatibility and common<br>
> (same version) compatibility given the number of checked in bitcode files we<br>
> already have. Is that a reasonable approach to testing here? Would some more<br>
> explicit tests be desired?<br>
<br>
</span>Have a look at the tests in test/Bitcode. Typically we create some<br>
IR that would use the old records in some known version (typically the<br>
previous release; you should probably be using 3.6?), generate bitcode<br>
and check it in. E.g., I wrote test/Bitcode/metadata.3.5.ll to test<br>
that bitcode from before the metadata-value split was understood going<br>
forward.<br></blockquote><div><br>Worth adding if it's already covered by existing tests? (searching for load instructions in test/Bitcode there are maybe 10 files with loads, and <span style="color:rgb(0,0,0)">memInstructions.3.2.ll which seems to have lots of them)</span><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> 1) is this the right way to do back-compat in this case (looking at the number<br>
> of entries in the bitcode record to disambiguate between the old schema and<br>
> the new?)<br>
><br>
> As an aside, this approach doesn't seem to work for GEP since there's less structure to its record size (it can contain any number of entries in the record). Any ideas on the right approach there? An explicit abbreviation (I guess we just version them?) or tag in some way? I'm still pretty hazy on the details.<br>
><br>
<br>
</span>Checking the number of entries seems to be usual; that's how I've been<br>
doing it. Another option is to take an integer field that uses fewer<br>
than 64-bits and add a bit to indicate before/after the change.<br>
<br>
For `GEP`, you can introduce a new record code and add an `_OLD` suffix<br>
to the names of the old codes.</blockquote><div><br>OK, makes sense I think - will have a play around with it.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> You only really need one new record<br>
code; you can add an initial 1-bit field to indicate whether it's<br>
inbounds</blockquote><div><br>Hmm - what's the tradeoff between that and the existing design using two codes, do you think?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> (then it's available for versioning in future upgrades).<br></blockquote><div><br>Not quite sure I follow here. Are you suggesting the bit size of the field can be increased in future versions (I would've thought that would break parsing? But I guess the bit size is in the abbreviation, so it's transparent to the reader who still sees a 64 bit value?), or that the 1 bit field will have some spare bits we can use later? <br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
In terms of the abbreviations, AFAICT they're not really accessible<br>
on the reader side (except in the low-level "reading the bits"). The<br>
abbreviations just change the way the record is encoded. They need to<br>
match what's actually written out by the writer, but don't add any<br>
semantics [1].<br>
<br>
[1]: <a href="http://llvm.org/docs/BitCodeFormat.html#abbreviations" target="_blank">http://llvm.org/docs/BitCodeFormat.html#abbreviations</a><br>
<span class=""><br>
><br>
> 2) I don't quite understand the logarithm logic to choose the encoding type of<br>
> the type parameter in the abbreviation description, but I found another<br>
> instruction doing the same thing & it seems to work. Is that the right<br>
> approach?<br>
<br>
</span>It probably makes sense. The logic is just saying that, since we have<br>
T types (which have been numbered from 0 to T-1), we need lg T bits to<br>
encode them. I guess the assumption is that there are usually few<br>
types; otherwise, the usual VBR-6 might be more efficient. I don't<br>
know how many types we tend to have, but I imagine T will drop pretty<br>
substantially with your changes, so if it's being used elsewhere it<br>
seems here as well.<br>
<div class=""><div class="h5"><br>
><br>
> <a href="http://reviews.llvm.org/D7655" target="_blank">http://reviews.llvm.org/D7655</a><br>
><br>
> Files:<br>
> lib/Bitcode/Reader/BitcodeReader.cpp<br>
> lib/Bitcode/Writer/BitcodeWriter.cpp<br>
><br>
> Index: lib/Bitcode/Reader/BitcodeReader.cpp<br>
> ===================================================================<br>
> --- lib/Bitcode/Reader/BitcodeReader.cpp<br>
> +++ lib/Bitcode/Reader/BitcodeReader.cpp<br>
> @@ -3510,21 +3510,32 @@<br>
> unsigned OpNum = 0;<br>
> Value *Op;<br>
> if (getValueTypePair(Record, OpNum, NextValueNo, Op) ||<br>
> - OpNum+2 != Record.size())<br>
> + (OpNum + 2 != Record.size() && OpNum + 3 != Record.size()))<br>
> return Error("Invalid record");<br>
><br>
> + Type *Ty = nullptr;<br>
> + if (OpNum + 3 == Record.size())<br>
> + Ty = getTypeByID(Record[OpNum++]);<br>
> +<br>
> I = new LoadInst(Op, "", Record[OpNum+1], (1 << Record[OpNum]) >> 1);<br>
> +<br>
> + assert(!Ty || Ty == I->getType());<br>
> +<br>
> InstructionList.push_back(I);<br>
> break;<br>
> }<br>
> case bitc::FUNC_CODE_INST_LOADATOMIC: {<br>
> // LOADATOMIC: [opty, op, align, vol, ordering, synchscope]<br>
> unsigned OpNum = 0;<br>
> Value *Op;<br>
> if (getValueTypePair(Record, OpNum, NextValueNo, Op) ||<br>
> - OpNum+4 != Record.size())<br>
> + (OpNum + 4 != Record.size() && OpNum + 5 != Record.size()))<br>
> return Error("Invalid record");<br>
><br>
> + Type *Ty = nullptr;<br>
> + if (OpNum + 5 == Record.size())<br>
> + Ty = getTypeByID(Record[OpNum++]);<br>
> +<br>
> AtomicOrdering Ordering = GetDecodedOrdering(Record[OpNum+2]);<br>
> if (Ordering == NotAtomic || Ordering == Release ||<br>
> Ordering == AcquireRelease)<br>
> @@ -3535,6 +3546,9 @@<br>
><br>
> I = new LoadInst(Op, "", Record[OpNum+1], (1 << Record[OpNum]) >> 1,<br>
> Ordering, SynchScope);<br>
> +<br>
> + assert(!Ty || Ty == I->getType());<br>
> +<br>
> InstructionList.push_back(I);<br>
> break;<br>
> }<br>
> Index: lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> ===================================================================<br>
> --- lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> +++ lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> @@ -1878,6 +1878,7 @@<br>
> if (!PushValueAndType(I.getOperand(0), InstID, Vals, VE)) // ptr<br>
> AbbrevToUse = FUNCTION_INST_LOAD_ABBREV;<br>
> }<br>
> + Vals.push_back(VE.getTypeID(I.getType()));<br>
> Vals.push_back(Log2_32(cast<LoadInst>(I).getAlignment())+1);<br>
> Vals.push_back(cast<LoadInst>(I).isVolatile());<br>
> if (cast<LoadInst>(I).isAtomic()) {<br>
> @@ -2232,6 +2233,8 @@<br>
> BitCodeAbbrev *Abbv = new BitCodeAbbrev();<br>
> Abbv->Add(BitCodeAbbrevOp(bitc::FUNC_CODE_INST_LOAD));<br>
> Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Ptr<br>
> + Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, // dest ty<br>
> + Log2_32_Ceil(VE.getTypes().size()+1)));<br>
> Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4)); // Align<br>
> Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // volatile<br>
> if (Stream.EmitBlockInfoAbbrev(bitc::FUNCTION_BLOCK_ID,<br>
><br>
> EMAIL PREFERENCES<br>
> <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>