[PATCH] [opaque pointer type] bitcode support for explicit type parameter to the load instruction

David Blaikie dblaikie at gmail.com
Tue Feb 24 15:58:35 PST 2015


ping

On Tue, Feb 17, 2015 at 10:30 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Sun, Feb 15, 2015 at 4:30 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
>>
>> > On 2015 Feb 15, at 16:17, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> >
>> > On Sun, Feb 15, 2015 at 3:48 PM, Duncan P. N. Exon Smith <
>> dexonsmith at apple.com> wrote:
>> >
>> > > On 2015 Feb 15, at 10:29, David Blaikie <dblaikie at gmail.com> wrote:
>> > >
>> > >
>> > >
>> > > On Sun, Feb 15, 2015 at 10:22 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> > > Hi rafael, resistor, grosbach, chandlerc,
>> > >
>> > > I've taken my best guess at this, but I've cargo culted in places & so
>> > > explanations/corrections would be great.
>> > >
>> > > This seems to pass all the tests (check-all, covering clang and llvm)
>> so I
>> > > believe that pretty well exercises both the backwards compatibility
>> and common
>> > > (same version) compatibility given the number of checked in bitcode
>> files we
>> > > already have. Is that a reasonable approach to testing here? Would
>> some more
>> > > explicit tests be desired?
>> >
>> > Have a look at the tests in test/Bitcode.  Typically we create some
>> > IR that would use the old records in some known version (typically the
>> > previous release; you should probably be using 3.6?), generate bitcode
>> > and check it in.  E.g., I wrote test/Bitcode/metadata.3.5.ll to test
>> > that bitcode from before the metadata-value split was understood going
>> > forward.
>> >
>> > 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
>> memInstructions.3.2.ll which seems to have lots of them)
>> >
>>
>> Good point; there is coverage in bitcode.  Might be nice to add a test
>> anyway, but I'm not really sure whether it's worth it.
>>
>
> In the interests of not adding extra burden to the test suite, I think the
> existing coverage (demonstrable by removing the extra size condition cases
> in the patch) seems sufficient to me - so I think the patch stands as-is
> from my perspective, awaiting further review/feedback. (just mentioning
> this in case anyone assumed they were waiting for me to update with test
> cases)
>
>
>>
>> >
>> > > 1) is this the right way to do back-compat in this case (looking at
>> the number
>> > >   of entries in the bitcode record to disambiguate between the old
>> schema and
>> > >   the new?)
>> > >
>> > > 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.
>> > >
>> >
>> > Checking the number of entries seems to be usual; that's how I've been
>> > doing it.  Another option is to take an integer field that uses fewer
>> > than 64-bits and add a bit to indicate before/after the change.
>> >
>> > For `GEP`, you can introduce a new record code and add an `_OLD` suffix
>> > to the names of the old codes.
>> >
>> > OK, makes sense I think - will have a play around with it.
>> >
>> >   You only really need one new record
>> > code; you can add an initial 1-bit field to indicate whether it's
>> > inbounds
>> >
>> > Hmm - what's the tradeoff between that and the existing design using
>> two codes, do you think?
>>
>> I assume `inbounds` was added later so it *required* another record
>> code at the time... I doubt it was an explicit design chocie (although
>> I could be wrong?).
>>
>> Having one record seems cleaner to me, but I haven't thought it through
>> carefully.
>>
>
> fair enough - thanks for the thoughts - should have that one out for
> review soon (~tomorrow) then.
>
>
>>
>> >
>> > (then it's available for versioning in future upgrades).
>> >
>> > 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?
>>
>> Right: the reader always sees it as a 64-bit value.  If/when there's a
>> need/use for more bits we can change the abbreviation.
>>
>
> Thanks - makes sense & provides some good options for optimal size now
> while providing easy backwards compatibility later.
>
>
>>
>> > ), or that the 1 bit field will have some spare bits we can use later?
>> >
>> >
>> > In terms of the abbreviations, AFAICT they're not really accessible
>> > on the reader side (except in the low-level "reading the bits").  The
>> > abbreviations just change the way the record is encoded.  They need to
>> > match what's actually written out by the writer, but don't add any
>> > semantics [1].
>> >
>> > [1]: http://llvm.org/docs/BitCodeFormat.html#abbreviations
>> >
>> > >
>> > > 2) I don't quite understand the logarithm logic to choose the
>> encoding type of
>> > >   the type parameter in the abbreviation description, but I found
>> another
>> > >   instruction doing the same thing & it seems to work. Is that the
>> right
>> > >   approach?
>> >
>> > It probably makes sense.  The logic is just saying that, since we have
>> > T types (which have been numbered from 0 to T-1), we need lg T bits to
>> > encode them.  I guess the assumption is that there are usually few
>> > types; otherwise, the usual VBR-6 might be more efficient.  I don't
>> > know how many types we tend to have, but I imagine T will drop pretty
>> > substantially with your changes, so if it's being used elsewhere it
>> > seems here as well.
>> >
>> > >
>> > > http://reviews.llvm.org/D7655
>> > >
>> > > Files:
>> > >   lib/Bitcode/Reader/BitcodeReader.cpp
>> > >   lib/Bitcode/Writer/BitcodeWriter.cpp
>> > >
>> > > Index: lib/Bitcode/Reader/BitcodeReader.cpp
>> > > ===================================================================
>> > > --- lib/Bitcode/Reader/BitcodeReader.cpp
>> > > +++ lib/Bitcode/Reader/BitcodeReader.cpp
>> > > @@ -3510,21 +3510,32 @@
>> > >        unsigned OpNum = 0;
>> > >        Value *Op;
>> > >        if (getValueTypePair(Record, OpNum, NextValueNo, Op) ||
>> > > -          OpNum+2 != Record.size())
>> > > +          (OpNum + 2 != Record.size() && OpNum + 3 != Record.size()))
>> > >          return Error("Invalid record");
>> > >
>> > > +      Type *Ty = nullptr;
>> > > +      if (OpNum + 3 == Record.size())
>> > > +        Ty = getTypeByID(Record[OpNum++]);
>> > > +
>> > >        I = new LoadInst(Op, "", Record[OpNum+1], (1 << Record[OpNum])
>> >> 1);
>> > > +
>> > > +      assert(!Ty || Ty == I->getType());
>> > > +
>> > >        InstructionList.push_back(I);
>> > >        break;
>> > >      }
>> > >      case bitc::FUNC_CODE_INST_LOADATOMIC: {
>> > >         // LOADATOMIC: [opty, op, align, vol, ordering, synchscope]
>> > >        unsigned OpNum = 0;
>> > >        Value *Op;
>> > >        if (getValueTypePair(Record, OpNum, NextValueNo, Op) ||
>> > > -          OpNum+4 != Record.size())
>> > > +          (OpNum + 4 != Record.size() && OpNum + 5 != Record.size()))
>> > >          return Error("Invalid record");
>> > >
>> > > +      Type *Ty = nullptr;
>> > > +      if (OpNum + 5 == Record.size())
>> > > +        Ty = getTypeByID(Record[OpNum++]);
>> > > +
>> > >        AtomicOrdering Ordering = GetDecodedOrdering(Record[OpNum+2]);
>> > >        if (Ordering == NotAtomic || Ordering == Release ||
>> > >            Ordering == AcquireRelease)
>> > > @@ -3535,6 +3546,9 @@
>> > >
>> > >        I = new LoadInst(Op, "", Record[OpNum+1], (1 << Record[OpNum])
>> >> 1,
>> > >                         Ordering, SynchScope);
>> > > +
>> > > +      assert(!Ty || Ty == I->getType());
>> > > +
>> > >        InstructionList.push_back(I);
>> > >        break;
>> > >      }
>> > > Index: lib/Bitcode/Writer/BitcodeWriter.cpp
>> > > ===================================================================
>> > > --- lib/Bitcode/Writer/BitcodeWriter.cpp
>> > > +++ lib/Bitcode/Writer/BitcodeWriter.cpp
>> > > @@ -1878,6 +1878,7 @@
>> > >        if (!PushValueAndType(I.getOperand(0), InstID, Vals, VE))  //
>> ptr
>> > >          AbbrevToUse = FUNCTION_INST_LOAD_ABBREV;
>> > >      }
>> > > +    Vals.push_back(VE.getTypeID(I.getType()));
>> > >      Vals.push_back(Log2_32(cast<LoadInst>(I).getAlignment())+1);
>> > >      Vals.push_back(cast<LoadInst>(I).isVolatile());
>> > >      if (cast<LoadInst>(I).isAtomic()) {
>> > > @@ -2232,6 +2233,8 @@
>> > >      BitCodeAbbrev *Abbv = new BitCodeAbbrev();
>> > >      Abbv->Add(BitCodeAbbrevOp(bitc::FUNC_CODE_INST_LOAD));
>> > >      Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Ptr
>> > > +    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed,       // dest
>> ty
>> > > +                              Log2_32_Ceil(VE.getTypes().size()+1)));
>> > >      Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4)); // Align
>> > >      Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //
>> volatile
>> > >      if (Stream.EmitBlockInfoAbbrev(bitc::FUNCTION_BLOCK_ID,
>> > >
>> > > EMAIL PREFERENCES
>> > >   http://reviews.llvm.org/settings/panel/emailpreferences/
>> > >
>> > > _______________________________________________
>> > > llvm-commits mailing list
>> > > llvm-commits at cs.uiuc.edu
>> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> > >
>> > >
>> > > _______________________________________________
>> > > llvm-commits mailing list
>> > > llvm-commits at cs.uiuc.edu
>> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150224/a99e64ce/attachment.html>


More information about the llvm-commits mailing list