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

[David Blaikie]

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)


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


> (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?), 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/20150215/c80cc0f0/attachment.html>