Tablgen refactoring

Sean Silva silvas at purdue.edu
Sun Feb 3 16:22:10 PST 2013


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



More information about the llvm-commits mailing list