[PATCH] Tablegen binary literals should be sized

[Sean Silva]

LGTM with some nits.

Please commit 0005 first, with a mention in the commit message to the
effect that this cleans things up for the upcoming change, also state the
rationale.

Also, 0006 contains a random change to test/TableGen/BitsInit.td which
should be incorporated into one of the previous commits (or as a separate
commit), so that 0006 can be just docs changes.

-- Sean Silva


On Wed, Aug 6, 2014 at 4:42 PM, Pete Cooper <peter_cooper at apple.com> wrote:

> Hi Sean
>
> I’ve updated the patches and included the test for bits<> assigned to int.
>
>
> I’ve also found that we should only be allowing !if expressions to be
> converted to bit values when both sides of the !if are single bits.  This
> is also an improvement over today where we’d silently drop bits.  I’ve
> added a test for this too.
>
> Please let me know if you have any other comments.
>
> Thanks,
> Pete
>
>
>
>
>
>
>
>
>
> On Jul 31, 2014, at 4:26 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
>
> 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/20140806/747f4196/attachment.html>