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

[Duncan P. N. Exon Smith]

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

> 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.  You only really need one new record
code; you can add an initial 1-bit field to indicate whether it's
inbounds (then it's available for versioning in future upgrades).

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