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

Duncan P. N. Exon Smith dexonsmith at apple.com
Tue Feb 24 16:25:58 PST 2015


> On 2015-Feb-24, at 15:58, David Blaikie <dblaikie at gmail.com> wrote:
> 
> ping
> 

This is still the original patch you sent to the list, right?

Given your point about existing bitcode upgrade coverage for `load`s,
this LGTM with a few nits:

> 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());

A message on this assertion might be nice.

> +
>        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());

Same here.

> +
>        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)));

clang-format?

Does it make sense to make `Log2_32_Ceil(VE.getTypes().size()+1)))`
a method in `ValueEnumerator`?

>      Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4)); // Align
>      Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // volatile
>      if (Stream.EmitBlockInfoAbbrev(bitc::FUNCTION_BLOCK_ID,
> 


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





More information about the llvm-commits mailing list