[llvm-commits] Speeding up instruction selection

Roman Levenstein romix.llvm at googlemail.com
Tue Mar 25 03:36:58 PDT 2008


Hi Dan,

Thank you for your feedback!

2008/3/25, Dan Gohman <gohman at apple.com>:
> 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.

Very good point! Sorry for not doing this before.
I've fixed all relevant places. The updated patch is attached.

I also rerun all dejagnu tests with the following failures:

# of expected passes          2669
# of unexpected failures      6
# of expected failures          5


XFAIL: /opt/llvm/test/Analysis/ScalarEvolution/2008-02-11-ReversedCondition.ll
FAIL: /opt/llvm/test/C++Frontend/2006-11-06-StackTrace.cpp
FAIL: /opt/llvm/test/C++Frontend/2008-02-13-sret.cpp
FAIL: /opt/llvm/test/CFrontend/2008-01-28-PragmaMark.c
FAIL: /opt/llvm/test/CFrontend/2008-02-11-AnnotateBuiltin.c
FAIL: /opt/llvm/test/CFrontend/2008-03-03-CtorAttrType.c
FAIL: /opt/llvm/test/CFrontend/2008-03-05-syncPtr.c
XFAIL: /opt/llvm/test/CodeGen/Alpha/mul5.ll
XFAIL: /opt/llvm/test/CodeGen/Generic/2007-04-14-EHSelectorCrash.ll
for PR1508,1326
XFAIL: /opt/llvm/test/CodeGen/Generic/2007-11-21-UndeadIllegalNode.ll
XFAIL: /opt/llvm/test/Transforms/InstCombine/zext-fold.ll for PR1570

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: UsesList2.patch
Type: text/x-patch
Size: 28771 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080325/357a8807/attachment.bin>


More information about the llvm-commits mailing list