[llvm-commits] Speeding up instruction selection

[Roman Levenstein]

Hi,

2008/3/26, Dan Gohman <gohman at apple.com>:
>  >>>> Also, I have a few quick questions on the patch itself:
>  >>>>
>  >>>>> +  /// Make it possible to use SDOperandImpl as an SDNode
>  >>>>> +  operator SDNode * () { return parent; }
>  >>>> I'm confused by this. It seems confusing to have SDOperand be
>  >>>> implicitly convertible to SDNode. I'm pretty sure it wasn't before.
>  >>>
>  >>> Before my patch, the use_iterator was defined like this:
>  >>> typedef SmallVector<SDNode*,3>::const_iterator use_iterator;
>  >>> And in many places in different code files it was typically used
>  >>> like:
>  >>> SDNode *User = *UI;
>  >>>
>  >>> With my changes, *UI now returns actually an SDOperand. Therefore,
>  >>> either all of the places as the one above should be changed or there
>  >>> should be a casting operator defined to convert an SDOperand into an
>  >>> SDNode *. I wanted to change the code as little as possible and have
>  >>> chosen the second approach. But if you think it can be dangerous, I
>  >>> can go for the first one. Then every code line of the form like
>  >>> this:
>  >>> SDNode *User = *UI;
>  >>> will be replaced by:
>  >>> SDNode *User = UI->getParent();
>  >>
>  >>
>  >> The implicit conversion does seem dangerous to me. And it's
>  >> different in a subtle way from the analogous constructs
>  >> in the high-level IR, where the Use class implicitly converts
>  >> to a Value* for the value being used, not the user of the
>  >> value.
>  >>
>  >> I'd prefer for use_iterator's operator* and operator-> to
>  >> be similar to those in value_use_iterator. Renaming getParent
>  >> to getUser would aid the analogy. Or if that's not
>  >> possible, I think making the calls explicit would be better
>  >> than the implicit conversion.
>
>  >>>
>
> >> SDOperand and SDOperandImpl are not very descriptive of
>  >> their new respective roles. How about SDUse instead of
>  >> SDOperand, and SDValue instead of SDOperandImpl?
>  >
>  > Hmm. I agree with you, that better names could be more descriptive. I
>  > even tried to do the renaming as you proposed. These are my findings:
>  > - about a few thausends of places needto  be changed in about 15
>  > source files, thus producing a huge patch.
>  > - tablegen is likely to be changed, since it produces some *.inc
>  > files for the instruction selector, where SDOperand is used.
>  >
>  > IMHO, such a renaming change would affect too much code, so I'd like
>  > to avoid doing it. But if you still tell me to go for more descriptive
>  > names, I could do it.
>  > How should we proceed here?
>
>
>  Looking at this more, I think there's a problem we should fix
>  here. All those thousands of mentions of SDOperand are places
>  that shouldn't need to know who the user is, which means that
>  they ought to be using SDOperandImpl instead of SDOperand,
>  given the way those classes are currently defined.

True. They were previously using the old definition of the SDOperand,
which is the SDOperandImpl now.

>  I would suggest starting by renaming the current SDOperand to
>  SDUse, and rename SDOperandImpl to SDOperand, leaving most
>  of the thousands of SDOperand mentions untouched.

I'll try it out tomorrow.

But if I remember correctly, I already tried this approach before.
There were quite some problems related to the following fact:
  - hundreds of function prototypes require SDOperand or SDOperand*
(pointer to an array of SDOperands) as a parameter type.
  - These SDOperand objects need to be assigned to SDUse objects. And
this is an issue! They have different sizes in memory and iteration
over the elements of SDOperand arrays becomes different from iteration
over the array of SDUse objects. Unfortunately, very many places in
the code currently assume that they have the same type and size (e.g.
all classes derived from SDNode allocate the SDOperand arrays in
place. They usually take SDOperand array as a parameter and initialize
the in-place array from them. If these in-place arrays now become
SDUse arrays then there is a type mismatch with all the consequences
of it. And this is just one example.)

To put it simply, current LLVM code is written assuming at very many
places that SDOperands (and nothing else) are used also for
OperandList arrays. Changing that will not be very easy and
straight-forward,

> The  DenseMaps that previously changed from SDOperand to SDOperandImpl
>  can go back to SDOperand then.

This is not a big deal.

> This will likely require some care to get right, but it shouldn't require huge quantities of
>  changes.
See above. I have my doubts ;-)


>  Once that's done and we're comfortable it, we can look at taking
>  the second step, which would be a mass-rename of SDOperand
>  to SDValue.

OK.

>  > BTW, with this patch in place, may of the ReplaceAll... functions can
>  > be probably implemented more efficiently (though they are much faster
>  > with this patch already), e.g. by using bulk replacements or something
>  > like this. Any ideas about what can be done?
>
>
> For the moment, I think it's best to wait until the naming
>  issues are straightened out.

OK.

>  > -Roman
>  >
>  > P.S.
>  > BTW, this patch of mine for speeding-up the instruction selection is
>  > not reviewed/tested yet. Could you have a look?
>  > http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080303/059215.html
>
>
> Yes, thanks. I hope to get to it soon. Thanks for your patience.

As long as there is a hope, I can wait ;-)
And to use the opportunity, there is my pending patch for
ScheduleDAGRRList.cpp still around (the one, where I suggested to use
std::set instead of priority queues based on heaps, see
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080303/059076.html).
Evan played with it, but there was a problem with 176.gcc (which is
probably unrelated) and somehow the review is stuck at the moment. It
would be nice to proceed with it. In the meantime, I could commit some
parts of it, that are not related to the std::set and are just obvious
optimizations that cannot break anything, e.g. fix for
SumOfUnscheduledPredsOfSuccs.

 - Roman