Tablgen refactoring

[Malul, Elior]

Good point, I think what we are to stick with the dynamic type querying, your suggestion is certainly better than my last patch.
However, I think that having a 3-level type system ("hello" is instance of 'StringInit', which in turn is an instance of 'StringTy'),
Has proven itself useful in other languages (e.g., java, C#, ruby), and I don't think we should reinvent the wheel in tblgen.

Anyways, I'm merely trying to make tblgen as good as it can be, I'm not trying (nor I am in a position to) impose my opinion in any way.
Thx for the review, Elior

-----Original Message-----
From: Sean Silva [mailto:silvas at purdue.edu] 
Sent: Monday, February 04, 2013 02:22
To: Malul, Elior
Cc: llvm-commits at cs.uiuc.edu; stoklund at 2pi.dk
Subject: Re: Tablgen refactoring

Looking at the code in this region, I think that the approach that you are taking now (which I thought was good previously) actually makes things more convoluted and complicated. Basically, all of these
convertValue() methods are only called inside the
convertInitializerTo() methods of the Init hierarchy, like this:

  virtual Init *convertInitializerTo(RecTy *Ty) const {
    return Ty->convertValue(const_cast<BitsInit *>(this));
  }

So basically, the current patch just complicates things further by pointlessly erasing the type information and then getting it back with a dyn_cast<>, when all of the call sites of this method have the type information (and convertValue() is not used anywhere else).

All of these call sites use this pattern where the argument type chooses a specific convertValue overload on T*, which then gets dynamically dispatched to the convertValue(T*) in the RecTy hierarchy.
This is really confusing because the convertValue() methods then call back into convertInitializerTo, leading to an inter-hierarchy recursion (and a cyclical dependency).

Since the convertInitializerTo methods are literally the only places that call convertValue, you might as well inline those methods into convertInitializerTo (although move the implementation to Record.cpp).
That is, for each convertValue(FooInit*) overload, look at all the dynamic dispatching that it does across all of the RecTy hierarchy; then directly implement that dynamic dispatching (and the corresponding code) inside FooInit::convertInitializerTo with dyn_cast<>. (You will also want to change the doxygen comment on the base class convertInitializerTo which says that "it is just a callback", since that design is being changed). The net result of these changes should be to completely remove all the convertValue() methods in the RecTy hierarchy (their functionality will still be present in convertInitializerTo).

I suspect that removing convertValue will open the door to further simplifications.

There is also a similar confusion happening where
RecTy::typeIsConvertibleTo() just calls into RecTy::baseClassOf(). You should also inline the logic for that function also in a separate patchset.

-- Sean Silva
---------------------------------------------------------------------
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.