[PATCH] [opaque pointer type] bitcode support for explicit type parameter to the load instruction
David Blaikie
dblaikie at gmail.com
Tue Feb 24 17:07:41 PST 2015
On Tue, Feb 24, 2015 at 4:25 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > 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?
>
Yep. Though I should've updated it on the ping - there were some in-tree
changes that conflicted (error handling for alignment attributes). Updated
now.
> 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.
>
Done.
Though this assert should be short-lived (well, it'll change to something
like "either an explicit type is provided xor the first operand is a
non-opaque pointer type").
>
> > +
> > 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.
>
Done
>
> > +
> > 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`?
>
Extracted such a utility in 230410 and modified this patch to use it.
>
> > 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
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150224/4edddace/attachment.html>
More information about the llvm-commits
mailing list