[llvm-commits] Speeding up instruction selection

[Dan Gohman]

Hi Roman,

I'm starting to run tests on your SDNode uses patch. Here's
what I've found so far.

First, it causes the following build error in the PPC target:

lib/Target/PowerPC/PPCISelLowering.cpp: In member function ‘virtual  
llvm::SDOperand  
llvm::PPCTargetLowering::PerformDAGCombine(llvm::SDNode*,  
llvm::TargetLowering::DAGCombinerInfo&) const’:
lib/Target/PowerPC/PPCISelLowering.cpp:3663: error: base operand of ‘- 
 >’ has non-pointer type ‘llvm::SDOperand’
lib/Target/PowerPC/PPCISelLowering.cpp:3664: error: base operand of ‘- 
 >’ has non-pointer type ‘llvm::SDOperand’
lib/Target/PowerPC/PPCISelLowering.cpp:3665: error: base operand of ‘- 
 >’ has non-pointer type ‘llvm::SDOperand’
lib/Target/PowerPC/PPCISelLowering.cpp:3666: error: base operand of ‘- 
 >’ has non-pointer type ‘llvm::SDOperand’

Can you do a build with all targets enabled? It's also useful to run
the dejagnu tests this way, as it exercises all targets' dejagnu tests.

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.
Can you explain what this is doing?

 >      for (unsigned i = 0; i != NumOps; ++i) {
 >        OperandList[i] = Ops[i];
 > -      Ops[i].Val->Uses.push_back(this);
 > +      OperandList[i].setParent(this);
 > +      Ops[i].Val->addUse(OperandList[i]);
 > +      ++Ops[i].Val->UsesSize;
 >      }

It looks like the new addUse call is making each operand
a use of itself here. That doesn't seem right.

Dan

On Mar 19, 2008, at 4:12 AM, Roman Levenstein wrote:
> Hi Evan,
>
> 2008/3/12, Evan Cheng <evan.cheng at apple.com>:
>> Thanks. Some comments:
>
> Thanks for your comments. All tabs violations are fixed. All "if("
> without spaces are fixed.
>
>> +  typedef uses_iterator use_iterator;
>> This seems silly, why not just change all references to use_iterator
>> to uses_iterator?
>
> Done.
>
>> +    //NodesUsed = NULL;
>>
>>      // no cycles in the graph.
>>      for (SDNode::op_iterator I = N->op_begin(), E = N->op_end(); I !
>> = E; ++I) {
>>        SDNode *Operand = I->Val;
>> -      Operand->removeUser(N);
>> +      Operand->removeUser(std::distance(N->op_begin(), I), N);
>>
>> Can you iterate from end to begin to avoid calling std::distance()  
>> for
>> every user?
>
> Well, I'm afraid it is the only reliable way to compute the index,
> because it does not depend on the knowledge about how operands of
> SDNode are stored.  The old code was assuming the SDOperand[], which
> is not exactly the case now and will probably change in the future.
> BTW, I thought that std::distance() is actually very cheap and is
> probably expanded into a subtraction. Am I wrong?
>
>> -  return cast<ConstantSDNode>(OperandList[Num])->getValue();
>> +  return (cast<ConstantSDNode>(OperandList[Num]))->getValue();
>>
>> Why the extra parentheses?
>
> Fixed.
>
>> -  if (N->hasOneUse() && (*N->use_begin())->getOpcode() ==
>> ISD::FP_ROUND)
>> +  if (N->hasOneUse() && ((SDNode*)(*N->use_begin()))->getOpcode() ==
>> ISD::FP_ROUND)
>>
>> Is the casting to SDNode* needed? use_begin() returns a uses_iterator
>> and its operator* returns a SDNode? Seems like this can be cleaned  
>> up.
>>>
>
> Fixed.
>
> I attach the new patch. Running the dejagnu tests in llvm/test does
> not introduce any new failures.
> llvm-test test-suite is huge and takes hours to run on my machine. Due
> to this fact,  I'm still not sure if it doesn't break anything. I'd
> really appreciate if someone
> with a faster machine could run llvm-test through it and report any
> problems, if there are any.
>
> -Roman