[llvm-commits] Speeding up instruction selection

[Roman Levenstein]

Hi Dan,

>  2008/3/26, Dan Gohman <gohman at apple.com>:

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

I tried it and it worked. I just did renaming as you suggsted mostly.
LLVM can be built with these changes.

>  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,

Just ignore my doubts! ;-)
This time I managed to do it and it builds at least.

But there were massive failures on the DejaGNU tests. And they were
exactly due to the reasons explained by me above.  At some places, an
SDUse* Ops was passed as an SDOprand* Ops and then inside the
functions there were accesses of the kind Ops[i], which obviously
cannot work correctly.

I fixed it, by creating SmallVector objects from OperandList array at
some places, e.g. in DAGCombiner.cpp and in X86ISelLowering.cpp.

Now all the DejaGnu tests pass without problems, but I'm not sure if
this fix with SmallVectors does not introduce any performance
problems. It would be nice, if you could test it on the real
test-suite, to see if the compilation speed is affected.

Because of these issues, I do not commit. Instead I attach the patch
for review and  for testing, which is even more important, since the
logic practically was not changed.

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

Done.

>
>  > This will likely require some care to get right, but it shouldn't require huge
> quantities of changes.

Done.

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

Should I do it?

-Roman
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SDNodeUses.patch
Type: text/x-patch
Size: 38643 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080327/fffed965/attachment.bin>