[llvm-commits] Tblgen refactoring

[Sean Silva]

A little bit more review on the patch before I commit it:

+  if (RecTy::baseClassOf(rTy)) //argument and the receiver are the same type
+    return (dyn_cast<BitsRecTy>(rTy)->Size == Size);

This is not a correct use of dyn_cast<>, since it immediately
dereferences the pointer (the whole point of using dyn_cast<> is that
it can return NULL to indicate the wrong type). Use cast<> instead if
you know that rTy is the correct type. Also, it is not common LLVM
style to put parentheses around the returned expression. Please get
rid of them here and elsewhere.

In the future, please submit patches with unix-style LF line endings
(rather than CRLF).

Also, what kind of testing have you done on this patch? I recommend at
least checking that all the generated .inc files are identical. (an
easy way to check this is to build the .inc files without the patch
applied, then create a git repository in the build directory and add
all the .inc files. then rebuild and do `git diff` to see if any have
changed).

-- Sean Silva

On Tue, Jan 1, 2013 at 2:06 AM, Malul, Elior <elior.malul at intel.com> wrote:
> Attached is a revised patch.
> Thx, Elior
>
> -----Original Message-----
> From: Sean Silva [mailto:silvas at purdue.edu]
> Sent: Tuesday, January 01, 2013 02:31
> To: Malul, Elior
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] Tblgen refactoring
>
> Please submit a revised patch for review.
>
> -- Sean Silva
>
> On Mon, Dec 31, 2012 at 6:20 AM, Malul, Elior <elior.malul at intel.com> wrote:
>> Hi Sean.
>> As you mentioned, the main idea behind the patch is to eliminate the (what seems to me as) excessive overloading of 'subClassOf' for every derived type.
>> Instead, I have changed it so there will be only one virtual method with the prototype 'virtual void baseClassOf(const RecTy*)'.
>> Then, the dynamic type to the parameter is done (when needed) by querying the 'getRecTyKind()' method.
>> It does add some branches to the code, but I think all together, it conforms better to the DRY principle.
>>
>> Thx for your comment. See rest of my comment in the text below:
>>
>> -----Original Message-----
>> From: Sean Silva [mailto:silvas at purdue.edu]
>> Sent: Monday, December 31, 2012 13:09
>> To: Malul, Elior
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] Tblgen refactoring
>>
>> This is something that has been bugging me too!
>>
>> Maybe you could explain a bit better the idea behind the patch a bit more? I think that it's the right direction. My understanding is that what you are doing is removing all the different overloads and having a single baseClassOf which dynamically looks at the records it is interested in? If that's correct then that sounds good to me.
>>
>> A few comments:
>>
>> -//===----------------------------------------------------------------------===//
>> -//    std::string wrapper for DenseMap purposes
>> -//===----------------------------------------------------------------
>> ------===//
>> -
>>
>> --Elior:
>> OK, I will separate for a different patch.
>>
>> This is unrelated to the current patch. I think it's a good change, but please separate it out into a different patch.
>>
>> +//
>> +//RecTy
>> +//
>> +
>>
>> --Elior:
>> Same goes here.
>>
>> Same for these "headers" too. I think they are a worthwhile change, but keep them in a separate patch.
>>
>> -    return true;
>> +   return true;
>>
>> What's the point of dedenting this by 1 space?
>>
>> --Elior:
>> Accidental space deletion.
>>
>> -- Sean Silva
>>
>> On Mon, Dec 31, 2012 at 2:25 AM, Malul, Elior <elior.malul at intel.com> wrote:
>>> Hi all, attached is a small patch that I think makes tblgen somewhat
>>> more maintainable.
>>>
>>>
>>>
>>> The way code is at the moment, if one want's to add a new tblgen type
>>> (that is to subclass 'RecTy'),
>>>
>>> Not only he/she compelled to add implementation to all variants of
>>> 'baseClassOf',
>>>
>>> one also needs to add a new 'baseClassOf' method for the newly added
>>> types to *all* the types in the hierarchy.
>>>
>>> The attached patch tries to deal with that phenomena.
>>>
>>>
>>>
>>> I will appreciate if those who care about tblgen will review it.
>>>
>>> Thx, Elior
>>>
>>> ---------------------------------------------------------------------
>>> Intel Israel (74) Limited
>>>
>>> This e-mail and any attachments may contain confidential material for
>>> the sole use of the intended recipient(s). Any review or distribution
>>> by others is strictly prohibited. If you are not the intended
>>> recipient, please contact the sender and delete all copies.
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>> ---------------------------------------------------------------------
>> Intel Israel (74) Limited
>>
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.