[llvm-commits] Speeding up instruction selection

[Dan Gohman]

On Mar 28, 2008, at 12:41 AM, Roman Levenstein wrote:
> 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.

I'm saying we should keep the SmallVector trick (for now), and
that we can make it work without inheritance. Adding an implicit
conversion to SDOperand in SDUse:

   operator SDOperand() const { return MySDOperand; }

is enough to let the SmallVector trick work. This has the
nice property of being the same solution that the high-level
Use uses.

Dan