[LLVMdev] Tablegen binary literals

[Sean Silva]

On Thu, Jul 31, 2014 at 11:28 AM, Pete Cooper <peter_cooper at apple.com>
wrote:

>
> On Jul 31, 2014, at 10:20 AM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
>
> On Thu, Jul 31, 2014 at 10:08 AM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>>
>> On Jul 31, 2014, at 9:02 AM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>> I think it would make sense to change this behavior to follow the
>> behavior that Pete describes. I don't have the code handy but my guess is
>> that what the code is doing here is requesting each element in the braces
>> of a bits<n> initializer to be casted to `bit`; in that case, we would want
>> to change that code to first try casting to bits and give that preference
>> (and issues relevant diagnostics). Pete, is that what your patch does? Can
>> you attach your patches?
>>
>> Yeah, thats exactly it.  Here’s a WIP patch which fixes that particular
>> issue.  I’m building the rest of the compiler now to see if anything breaks
>> with that applied.
>>
>>
>>
>>
>> Of course, before changing the behavior, we would probably want to place
>> an assert on that codepath to catch all existing cases where the behavior
>> would change.
>>
>> Sounds like a good plan.  I’ll try that and see what happens.
>>
>> Also it is important to scan all the docs in docs/TableGen/ and make any
>> necessary updates (if there is nowhere that needs to be updated, then
>> documentation needs to be *added* covering this behavior).
>>
>> I haven’t done that yet, but good point.  Depending on what changes you
>> all think are acceptable, i’ll update the documentation appropriately.
>>
>> Finally, we should post a notice to LLVMdev for out-of-tree maintainers
>> including information about how to add the necessary assert to check for
>> places where this behavior would change their .td files.
>>
>> Sure thing.
>>
>> And here’s the other 2 patches.  The first sizes binary literals.
>>
>
> Regarding this patch, does it need to create a whole new Init class? I
> would try to avoid that if at all possible. Could we just have 0b0111 be a
> shorthand for a bits<4> BitsInit? Or maybe store some extra data in IntInit?
>
> I was worried that changing IntInit to store a size would have be too much
> of a change to existing behavior.  Perhaps thats wrong, as I only really
> need to check the size in a few limited circumstances.
>

I'm personally much more in favor of the first option I suggested (0b0111
should be a bits<4>).


>
>
>
>>  The second allows them to initialize multiple bits inside { }.  Note
>> that i’m only handling certain cases inside the { } initializer.  I’m happy
>> to refactor the code and handle other cases if necessary.
>>
>
> I don't like the special casing going on in this patch. Could we just
> switch BitsInit to store pointers to *both* BitInit and BitsInit (it
> actually can already sort of do that since it has a std::vector<Init*>)?
>
> Yeah, I think this could be improved.  How about instead of
>
> Init *Bit = Vals[i]->convertInitializerTo(BitRecTy::get());
>
> I do
>
> Init *Bit = Vals[i]->convertInitializerTo(BitsRecTy::get());
>
> Or perhaps attempt the Bits case first, and Bit as a fallback, or vice
> versa?
>

Would the order of these checks cause any behavioral difference (besides
one of the orders failing to produce the desired behavior)? I would hope
not.


>
> The problem here is that I don’t necessarily want to allow { } to accept
> something like ‘-7’ because now I need to decide how many bits that is.
>

Well, try placing a printf in `Init *BitsRecTy::convertValue(IntInit *II)`
and see what you find. If it seems like nothing is crucially depending on
this behavior, then it would probably make sense to completely eliminate
this conversion.


-- Sean Silva


>  So i don’t want to allow anything to cast to Bits, but instead just
> anything with an exact size.  I’ll investigate the scope of this change and
> see what actually happens.  It might turn out ok.
>
> Thanks for the feedback.
> Pete
>
>
> I just feel like this is adding a lot of complexity to TableGen where the
> behavior that you want could largely be accomplished by removing complexity.
>
> -- Sean Silva
>
>
>>
>>
>>
>>
>> Thanks,
>> Pete
>>
>>
>> -- Sean Silva
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/530b474e/attachment.html>