[PATCH] Tablegen binary literals should be sized

[Sean Silva]

On Thu, Jul 31, 2014 at 5:02 PM, Pete Cooper <peter_cooper at apple.com> wrote:

>
> On Jul 31, 2014, at 3:03 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
> Did you see anywhere in the docs that needed updating?
>
> Yeah, here’s what I’ve got so far.  I’m not 100% sure on the how well i’ve
> organized the changes in LangRef.rst, but I think its close to explaining
> that we can now nest { } values.
>
>

I don't think that your patches actually change the acceptable syntax so I
don't think that your change to LangRef.rst is needed.
However, you should add a sentence following the BinInteger production
saying that it is treated as an object of type "bits<n>" (where n = number
of given bits) and not "int", but will still convert to an int if needed.
Now that I think about it, could you make sure that we have test coverage
for a 0bxx literal initializing an "int"?

The LangIntro changes LGTM.


>
>
> Regarding patch 0005, thankfully those were benign since they just had
> leading 0's. Can you doublecheck the diagnostic quality? We need to give a
> clear indication that what is wrong so that users don't get left scratching
> their heads.
>
> I added a better error message in r214374.  As an example from those that
> were fixed in patch 5, this is what I saw before I made a fix:
>
> lib/Target/Sparc/SparcInstrVIS.td:260:16: error: Value 'VISInst:opfval' of
> type 'bits<9>' is incompatible with initializer '{ 0, 0, 0, 0, 0, 1, 0, 1,
> 1, 0 }' of type bit initializer with length 10'
> def UMULXHI  : VISInst<0b0000010110, "umulxhi", I64Regs>;
>                ^
>
>
> That seems sane.

>
> +  /// resolveListElementReference - This method is used to implement
> +  /// VarListElementInit::resolveReferences.  If the list element is
> resolvable
> +  /// now, we return the resolved value, otherwise we return null.
> +  Init *resolveListElementReference(Record &R, const RecordVal *RV,
> +                                    unsigned Elt) const override {
> +    llvm_unreachable("Illegal element reference off bits<n>");
> +  }
>
> This saddens me... all this TableGen code really needs to be refactored...
> There may be a simple refactor for this resolveListElementReference
> situation that does not require plunging into the murky depths of the
> resolution machinery.
>
> I’m open to suggestions :)  Otherwise, is there anything you can think of
> thats better than having BitsInit now being a TypedInit subclass?
>

Mostly I was wondering if there are sufficiently few places that call
resolveListElementReference that we could just pull the entire logic for
resolveListElementReference out of these classes entirely and make it a
static function in the relevant .cpp file.

-- Sean Silva


>
> Thanks,
> Pete
>
>
>
> -- Sean Silva
>
>
>
> On Thu, Jul 31, 2014 at 2:48 PM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>> Hi all
>>
>> Moving this from LLVMDev (
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/075223.html) now
>> that i have patches i’d welcome reviews of.  The following is the initial
>> summary from that email thread.
>>
>> "Currently tablegen parses binary literals such as 0b011 and immediately
>> turns them in to integers internally.  Due to this, 0b011 is a 2-bit value
>> because tablegen will happily drop leading zeroes on integers.
>>
>> I propose that we change this, and store binary literals with a size.  I
>> think this is much more natural, as when the user writes a value with 3
>> bits, I think they expect to get a value with 3 bits, and not a 2-bit value
>> which is silently truncated and later silently zero extended.
>>
>> Given this, I would then like to extend bits initializers to accept these
>> new sized binary literals.  For example, I would like to be able to write
>>
>> bits<2> opc;
>> bits<8> x = { opc, 0b1100, rd{1}, rs{0} }
>>
>> This would let us write some encodings much more concisely, instead of
>> having to put each initializer on its own line.”
>>
>>
>> For those interested in the low level details, i’ve changed binary
>> literals such as 0b011 from creating an integer ‘3’, to creating a BitsInit
>> ‘{ 0, 1, 1}’.  As Sean suggested, this was better than introducing yet
>> another type just for binary literals.
>>
>> As part of this, BitsInit values now had to be able to be typed.  This is
>> because we had uses of binary literals which expected to be typed (for
>> example list<bits<2>> [0b00, 0b01]).  I’ve moved BitsInit from being a
>> subclass of Init to a subclass of TypedInit to allow this.
>>
>> Note that patches 2 and 3 really need to be committed together.  I can
>> squash them before commit, but just wanted to present the changes
>> independently.
>>
>> Patch 5 can also be committed without any of the others, but just shows
>> that this did actually catch some useful cases where we mismatched sizes of
>> binary expressions.
>>
>> Thanks,
>> Pete
>>
>>
>>
>>
>>
>>
>>
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140731/57db1428/attachment.html>