[llvm-commits] Speeding up instruction selection

[Roman Levenstein]

Hi Dan, Hi Evan,

2008/3/28, Dan Gohman <gohman at apple.com>:
> Hi Roman,
>
>  I tested the SDNodeUses.patch and it passed. I have one comment
>  on this patch; I don't think this change:
>
>  -  const SDOperand &getOperand(unsigned Num) const {
>  +  const SDUse &getOperand(unsigned Num) const {
>
>  and several other related changes (getChain, getBasePtr, etc.)
>  are right. I think these should still return SDOperand references.
>  This will help keep client code more consistent.

Dan, it is funny that you mention it. After I sent the patch, I
discovered the same issues and fixed it already.

> Other than that I think this patch is ok to commit.
>
>  I also have a comment on the overall set of changes. Right now
>  SDUse inherits from SDOperand, and that makes SDUse* implicitly
>  convertible to SDOperand*, which as you noticed is unsafe when
>  there's pointer arithmetic.

I also thought about this issue... To experiement, I tried to make
SDOperand a provate base of SDUse, to see where it would break.
Basically, you cannot use the SmallVector approach afterwards.

>  I think it'd be better for SDUse to have an SDOperand member instead, with associated >  accessor methods.
I had this idea as well.


>  To keep op_iterator working, it may be easiest to add
>  an implicit conversion from SDUse to SDOperand; this happens
>  be analogous to what llvm::Use does. On the other hand, an
>  op_iterator class wouldn't be out of place here either. What
>  do you think?

I'm not sure this would really help much. Many places expect pointers
to the array of SDOperands and unfortunately, many of them are used
for real SDOperand array and for OperandList-kind of operands at the
same time. Conversion of the single pointer does not help here. So, we
either need SmallVector-trick at some places or we should continue
using SDOperand instead of SDUse. That is, SDUse becomes the SDOperand
again, as I did before.

>  More below...
>
>
>  On Mar 27, 2008, at 11:25 AM, Roman Levenstein wrote:
>  >> Would it help if there were precompiled bitcode files for
>  >> the tests in llvm-test that could be downloaded? I wonder if
>  >> we could do that. I know quite a few people working on LLVM
>  >> but not llvm-gcc that this would probably help.
>  >
>  > This would help with the test-cases that require some enhancements
>  > from the front-end. Morover, may be even SPEC testcases can be
>  > distributed in this form, as Chris indicated in his earlier mail.
>  >
>  > Another alternative could be to build the front-end every day or every
>  > hour for major OS/Target combinations and make them available as
>  > SVN/nightly builds. The build scripts are already there, since they
>  > are used to build some prepackaged versions of the GCC front-end for
>  > LLVM releases. What do you think?
>
>
> I think it comes down to resources. I'll ask around to see what
>  we might do.

OK.

>  >>>>> 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?
>  >>
>  >> We're not done with the first step yet :-).
>  >
>  > But we are almost done ;-) OK, OK. I'm too fast...
>
>
>  I don't want to be annoying, but I do want to point out that
>  doing more testing on your own would allow this process to
>  proceed more efficiently :-).

Believe me, I do testing as much as can. I think you have noticed that
none of them have broken anything, expect for this testcase with
176.gcc.
But my resources are a bit limited. Also, the changes I'm working on
affect the overall code generation and scheduling mechanisms and
therefore any problems may become too critical and break everything.
And they have to do with the performace, which is rather different
dependent on the machine, architecture, etc. Therefore I prefer if
others test these patches as well, both their performance and
correctness.

>
>  >
>  > But if we do not discover any performance issues, then the patch can
>  > be just commited as is and I can start working on mass-rename patch.
>  >
>  > BTW, do you think that the current work being done by Gabor on the
>  > "diet" Use/Value implementation could/should be later applied in the
>  > SDOpeand/SDUse/SDNode context?
>
>
>  It definitely could be. Whether it should is unclear.
>  The optimizer is often holding large numbers of whole function
>  bodies in memory at once, which is what's pushing people to
>  consider such extreme measures.

OK. I see.

Evan wrote:
> I am not sure about this. For all the places which pass the list of
> SDOperand's as a ptr to an array, you want to make a copy of it into
> an array before passing it?

Actually not for all. Only those places which are replaced by
SmallVectors now. I think there are many more places around.

> I would really need to see proof that it isn't a performance issue.
Sure. This is why the patch was not commited yet and we are testing.
The goal is to improve the performance, not to make it worse ;-)

>I haven't read this thread  carefully. Are you trying to change
OperandList from
>SDOperand vector to a vector?

By no means! The issue is that Dan wants to have a pair of classes
SDOperand/SDUse similar to the Use/Value classes. SDNode should keep
an array of SDUses as OperandList then, and everywhere esle the
SDOperand (which becomes the SDValue then) is used. But the problem is
that currently it is assumed almost everywhere in the code, that
OperandList is an array of SDOperands and not of anything else. This
causes the problems with Dan's proposal.

Any good ideas how to resolve this issue?

- Roman