[PATCH][TableGen] Fully resolve class-instance values before defs in multiclasses
Adam Nemet
anemet at apple.com
Tue Sep 16 10:47:46 PDT 2014
On Sep 15, 2014, at 1:45 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> 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.
Thanks. Committed as r217886. Filed bug as PR20961.
Adam
>
> -- 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/20140916/74f68fb4/attachment.html>
More information about the llvm-commits
mailing list