[PATCH][TableGen] Fully resolve class-instance values before defs in multiclasses

Sean Silva chisophugis at gmail.com
Sat Sep 13 04:45:02 PDT 2014


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.

-- 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/20140913/28ddc948/attachment.html>


More information about the llvm-commits mailing list