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

[David Blaikie]

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?


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


>
> 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?
>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150215/f3f491fa/attachment.html>