[PATCH][TableGen] Fully resolve class-instance values before defs in multiclasses
Sean Silva
chisophugis at gmail.com
Mon Sep 15 13:45:49 PDT 2014
On Mon, Sep 15, 2014 at 12:44 PM, Adam Nemet <anemet at apple.com> wrote:
>
> On Sep 13, 2014, at 11:03 AM, Adam Nemet <anemet at apple.com> wrote:
>
>
> On Sep 13, 2014, at 4:45 AM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
> On Fri, Sep 12, 2014 at 4:02 PM, Adam Nemet <anemet at apple.com> wrote:
>
>>
>> On Sep 12, 2014, at 1:34 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>>
>> On Thu, Sep 11, 2014 at 9:57 PM, Adam Nemet <anemet at apple.com> wrote:
>>
>>>
>>> On Sep 11, 2014, at 12:56 PM, Tom Stellard <tom at stellard.net> wrote:
>>>
>>> On Wed, Sep 10, 2014 at 05:56:57PM -0700, Adam Nemet wrote:
>>>
>>> By class-instance values I mean 'Class<Arg>' in 'Class<Arg>.Field' or in
>>> 'Other<Class<Arg>>' (syntactically a SimpleValue). This is to
>>> differentiate
>>> from unnamed/anonymous record definitions (syntactically an ObjectBody)
>>> which
>>> are not affected by this change.
>>>
>>> Consider the testcase:
>>>
>>> class Struct<int i> {
>>> int I = !shl(i, 1);
>>> int J = !shl(I, 1);
>>> }
>>>
>>> class Class<Struct s> {
>>> int Class_J = s.J;
>>> }
>>>
>>> multiclass MultiClass<int i> {
>>> def Def : Class<Struct<i>>;
>>> }
>>>
>>> defm Defm : MultiClass<2>;
>>>
>>> Before this fix, DefmDef.Class_J yields !shl(I, 1) instead of 8.
>>>
>>> This is the sequence of events. We start with this:
>>>
>>> multiclass MultiClass<int i> {
>>> def Def : Class<Struct<i>>;
>>> }
>>>
>>> During ParseDef the anonymous object for the class-instance value is
>>> created:
>>>
>>> multiclass Multiclass<int i> {
>>> def anonymous_0 : Struct<i>;
>>>
>>> def Def : Class<NAME#anonymous_0>;
>>> }
>>>
>>> Then class Struct<i> is added to anonymous_0. Also
>>> Class<NAME#anonymous_0> is
>>> added to Def:
>>>
>>> multiclass Multiclass<int i> {
>>> def anonymous_0 {
>>> int I = !shl(i, 1);
>>> int J = !shl(I, 1);
>>> }
>>>
>>> def Def {
>>> int Class_J = NAME#anonymous_0.J;
>>> }
>>> }
>>>
>>> So far so good but then we move on to instantiating this in the defm
>>> by substituting the template arg 'i'.
>>>
>>> This is how the anonymous prototype looks after fully instantiating.
>>>
>>> defm Defm = {
>>> def Defmanonymous_0 {
>>> int I = 4;
>>> int J = !shl(I, 1);
>>> }
>>>
>>> Note that we only resolved the reference to the template arg. The
>>> non-template-arg reference in 'J' has not been resolved yet.
>>>
>>> Then we go on to instantiating the Def prototype:
>>>
>>> def DefmDef {
>>> int Class_J = NAME#anonymous_0.J;
>>> }
>>>
>>> Which is resolved to Defmanonymous_0.J and then to !shl(I, 1).
>>>
>>> When we fully resolve each record in a defm, Defmanonymous_0.J does get
>>> set to 8
>>> but that's too late for its use.
>>>
>>> The patch adds a new attribute to the Record class that indicates that
>>> this
>>> def is actually a class-instance value that may be *used* by other defs
>>> in a multiclass. (This is unlike regular defs which don't reference each
>>> other and thus can be resolved independently.) These are then fully
>>> resolved before the
>>> other defs while the multiclass is instantiated.
>>>
>>>
>>> Hi Adam,
>>>
>>> Is there any reason why your patch works on defs inside multiclasses and
>>> not all
>>> defs? I'm running into a similar problem with this example:
>>>
>>> class A <bits<2> op> {
>>> field bits<2> OP = op;
>>> }
>>>
>>> class B <bits <2> op> {
>>> field A OP = A<op>;
>>> }
>>>
>>> def : B <0>;
>>>
>>> As you can see, the value of B.OP.OP fails to resolve:
>>>
>>> ------------- Classes -----------------
>>> class A<bits<2> A:op = { ?, ? }> {
>>> field bits<2> OP = { A:op{1}, A:op{0} };
>>> string NAME = ?;
>>> }
>>> class B<bits<2> B:op = { ?, ? }> {
>>> field A OP = anonymous_0;
>>> string NAME = ?;
>>> }
>>> ------------- Defs -----------------
>>> def anonymous_0 { // A
>>> field bits<2> OP = { B:op{1}, B:op{0} };
>>> string NAME = ?;
>>> }
>>> def anonymous_1 { // B
>>> field A OP = anonymous_0;
>>> string NAME = ?;
>>> }
>>>
>>> I'm wondering if modifying your patch to work everywhere and not just
>>> in multiclasses will fix this. Do you think this might work?
>>>
>>>
>>> Hi Tom,
>>>
>>> I think conceptually you’re right, these should be handled similarly but
>>> unfortunately they are not. In fact I am not even sure I understand how
>>> your case is supposed to be handled by the current design.
>>>
>>> Multiclasses have this concept of defprototypes. E.g. in:
>>>
>>> multiclass Multiclass<int i> {
>>> def anonymous_0 : Struct<i>;
>>>
>>> def Def : Class<NAME#anonymous_0>;
>>> }
>>>
>>> Even though we create anonymous_0 on the fly we don’t make it a
>>> top-level def but rather a defprototype just like Def. Defprototypes (as
>>> opposed to definitions) still depend on template args of the enclosing
>>> multiclass. This all happens when we instantiate the multiclass at the
>>> defm.
>>>
>>> Classes don’t have this intermediate concepts. This is also pretty
>>> clear from what tablegen prints. As you can see anonymous_0 becomes as
>>> top-level def and is detached from class B. So there is little surprise we
>>> don’t substitute B:op upon instantiating B.
>>>
>>> My immediate thought is that we may want to extend the defprototype
>>> concept to regular classes…
>>>
>>> For the record, I also had another idea to mostly push the existing
>>> design. If you used specific fields from the class instance value rather
>>> than the entire thing, we could make this work by fully resolving the class
>>> before substituting template args:
>>>
>>> class B <bits <2> op> {
>>> field bits<2> OP = A<op>.OP;
>>> }
>>>
>>> The last line is anonymous_0.OP and it would become { B:op{1}, B:op{0} }
>>> with the new round of reference-resolution. So after substituting the
>>> template arg B:op, field OP resolves properly.
>>>
>>> This unfortunately still breaks down if you hold on to the entire object
>>> just like in your original code and let’s say you try to use one of its
>>> fields outside. E.g. note z.z. in:
>>>
>>> class A <int op1> {
>>> int OP1 = op1;
>>> }
>>>
>>> class B <int op2> {
>>> A OP2 = A<op2>;
>>> int OP3 = OP2.OP1;
>>> }
>>>
>>> def b : B <1>;
>>>
>>> def z {
>>> int k = b.OP3;
>>> int z = b.OP2.OP1;
>>> }
>>>
>>> As expected z.z fails to resolve:
>>>
>>> ------------- Classes -----------------
>>> class A<int A:op1 = ?> {
>>> int OP1 = A:op1;
>>> string NAME = ?;
>>> }
>>> class B<int B:op2 = ?> {
>>> A OP2 = anonymous_0;
>>> int OP3 = OP2.OP1;
>>> string NAME = ?;
>>> }
>>> ------------- Defs -----------------
>>> def anonymous_0 { // A
>>> int OP1 = B:op2;
>>> string NAME = ?;
>>> }
>>> def b { // B
>>> A OP2 = anonymous_0;
>>> int OP3 = 1;
>>> string NAME = ?;
>>> }
>>> def z {
>>> int k = 1;
>>> int z = B:op2;
>>> string NAME = ?;
>>> }
>>>
>>> I’m copying Jakob to see if he has any ideas how this is supposed to
>>> work but my current assumption is that these two cases are distinct, so the
>>> patch still stands. That said, we may want to move classes more in the
>>> direction of multiclasses…
>>>
>>
>> Both cases fall into the general pile of "TableGen's
>> resolution/evaluation machinery is extremely convoluted". Your original
>> patch makes this worse; I would categorize it as a band-aid. If you really
>> need it as a band-aid to fix an important use case, then carry on, but if
>> that's the case then a little bit of refactoring in another part of
>> TableGen to pay back that technical debt would be appreciated.
>>
>>
>> Hi Sean,
>>
>> Yes, this bug is blocking further cleanup and development using the new
>> X86VTVectorInfo abstraction in AVX512 so I would prefer to get that
>> unblocked first before moving on to other refactoring work.
>>
>> Having been through debugging these problems, I do agree that there is
>> room for improvement in tablegen. I can certainly look into these as
>> follow-on work.
>>
>> For example I would like to get multiclasses printed as well just like we
>> print classes. There may be other ways to improve the debugging
>> experience...
>>
>> We also need a new design for how class-instance values are handled when
>> enclosed in template classes. I don’t think we currently have a design for
>> that.
>>
>> Can you please elaborate what improvements you’re proposing for
>> multiclasses (i.e. why do you think my fix is a band-aid)?
>>
>
> It's a band-aid in the sense of "oops, we don't handle this corner case;
> just add in a flag and a couple if's in random places to fix it". It's a
> form of technical debt.
>
> There's nothing inherently wrong with this kind of thing (actually, in my
> experience, a measured dose of such band-aids tends to characterize
> production-quality software), but it becomes problematic when they pile up
> (and there are plenty piled up in TableGen right now), or when they are
> used in place of a "proper" solution that simplifies the logic to be more
> general, extracts commonality to a single point of truth, etc.
>
> There's a decent argument that can be made that the difficulty of
> understanding/refactoring a piece of code increases exponentially with the
> number of "random flags" that couple disparate parts of the code, so I'd
> especially avoid adding a flag if at all possible.
>
>
> OK, that’s what I was assuming too; your yet-another-flag trigger had gone
> off.
>
> What do you think about proceeding like this? We keep the patch as
> proposed (I would appreciate a LGTM if your OK with the plan), then I file
> a bug on Tom’s test case with mentioning that we should probably abstract
> the problem with classes and multiclasses similarly and hopefully get rid
> of the flag I added along the way.
>
>
> This may not have been clear; what I meant was that besides filing the bug
> I will of course attempt to fix it as well.
>
Sounds good to me. Go for it.
-- Sean Silva
>
> Adam
>
> Adam
>
> -- Sean Silva
>
>
>> Class-instance values get fully resolved immediately (i.e. before any of
>> its use) when not inside multiclasses. My change just extends this logic
>> to multiclasses, i.e. resolve immediately as soon as the template args are
>> known. I guess we can differentiate class-instance values from the real
>> defprototypes and keep them in a separate list inside MultiClass. This
>> would remove the need for the new flag but other than that, these concepts
>> seem pretty sound to me.
>>
>> Thanks,
>> Adam
>>
>> -- Sean Silva
>>
>>
>>>
>>> Thanks,
>>> Adam
>>>
>>> Thanks,
>>> Tom
>>>
>>>
>>>
>>> I added vg_leak to the new test. I am not sure if this is necessary but
>>> I
>>> don't think I have a way to test it. I can also check in without the
>>> XFAIL
>>> and let the bots test this part.
>>>
>>> Also tested that X86.td.expanded and AAarch64.td.expanded were unchange
>>> before
>>> and after this change. (This issue triggering this problem is a WIP
>>> patch.)
>>>
>>> Part of <rdar://problem/17688758>
>>>
>>> Please let me know if it looks good.
>>>
>>> Thanks,
>>> Adam
>>>
>>>
>>>
>>> _______________________________________________
>>> 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/20140915/f63fcb70/attachment.html>
More information about the llvm-commits
mailing list