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

[Adam Nemet]

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…

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140911/a44f8f17/attachment.html>