[llvm-commits] Speeding up instruction selection

[Tanya Lattner]

>
> As far as I can see, most of the failures are in the front-end. This
> is may be because I don't build the FE from sources but use one from
> the latest LLVM release.
>

This won't work. llvm and llvm-gcc must both come from svn or both  
come from a release.

-Tanya



> The only interesting ones are probably these:
> XFAIL: /opt/llvm/test/Analysis/ScalarEvolution/2008-02-11- 
> ReversedCondition.ll
> XFAIL: /opt/llvm/test/CodeGen/Alpha/mul5.ll
> XFAIL: /opt/llvm/test/Transforms/InstCombine/zext-fold.ll for PR1570
>
> But I think all of them are not introduced by my patch. I see the same
> on the mainline, when I run tests withe the version from the LLVM SVN.
>
>>  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();
>
>>  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.
>
> Well it adds each operand as a use to the Ops[i].Val, which is the
> SDNode this operand points to. And this is exactly what should be
> done.
>
> -Roman
>
>>  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
>>
>>
>>
>> _______________________________________________
>>  llvm-commits mailing list
>>  llvm-commits at cs.uiuc.edu
>>  http://lists.cs.uiuc.edu/mailman/listinfo/llvm- 
>> commits<UsesList2.patch>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits