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

[Sean Silva]

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.

-- 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/20140912/c9ba670c/attachment.html>