[llvm-commits] Speeding up instruction selection (SelectionDAGISel)

[Roman Levenstein]

Hi,

2008/5/7 Evan Cheng <evan.cheng at apple.com>:
> A couple of requests. Since you are moving it into its own file, I
>  would like to see a bit more refinement. Specifically, can you update
>  it so its coding style is a bit more consistent with the rest of the
>  llvm world. e.g.
>
>  +    if (!isSelected(Node->getNodeId())) {
>  +      SDNode *ResNode = Select(SDOperand(Node, 0));
>  +      if (ResNode != Node) {
>  +        if (ResNode)
>  +          ReplaceUses(Node, ResNode);
>  +        if (Node->use_empty()) { // Don't delete EntryToken, etc.
>  +          ISelQueueUpdater ISQU(ISelQueue);
>  +          CurDAG->RemoveDeadNode(Node, &ISQU);
>  +          UpdateQueue(ISQU);
>  +        }
>  +      }
>  +    }
>
>  =>
>
>  +    if (isSelected(Node->getNodeId())) {
>          continue;
>  +    SDNode *ResNode = Select(SDOperand(Node, 0));
>  +    if (ResNode == Node)
>          continue;
>  +     if (ResNode)
>  +        ReplaceUses(Node, ResNode);
>  +      if (Node->use_empty()) { // Don't delete EntryToken, etc.
>  +        ISelQueueUpdater ISQU(ISelQueue);
>  +        CurDAG->RemoveDeadNode(Node, &ISQU);
>  +        UpdateQueue(ISQU);
>  +    }

Done.

>  Also, can you update the comments? Add a bit more contents and make
>  use of doxygen style comments?
>  e.g.
>  +// SelectRoot - Top level entry to DAG isel.

Done and committed.

-Roman

>  On May 6, 2008, at 5:55 AM, Roman Levenstein wrote:
>
>
>
> > Hi Even, Hi Dan,
>  >
>  > Here is a patch as discussed in this mail-thread.
>  >
>  > 1) It changes the tablegen and moves the common part of all
>  > instruciton selectors into include/llvm/CodeGen/IDAGISelHeader.h file.
>  >    Tablegen now only generates the include preprocessor directive for
>  > this file.
>  >    I prefer this method to extending the SelectionDAGISel parent
>  > class, because it may provide more flexibility. For example, based
>  >    on some #defines controlling the type of the code selector and
>  > generated by TableGen based on the comman-line parameters
>  >    different versions of the code selection algorithms cam be used.
>  >
>  > 2) I provide in the include/llvm/CodeGen/IDAGISelHeader.h the current
>  > standard LLC implementation.
>  >
>  > So, the current patch only re-factors the LLVM and tablegen code, but
>  > introduces no functionality change.
>  >
>  > Regarding the speed-up: In my previous report about this patch I did a
>  > mistake by measuring the times for the DEBUG version of llc.
>  > There it is really a win (up to 20% on some use-cases, as I reported
>  > before)! But for the optimized version of the llc, the win is almost
>  > minimal :-(
>  > Good lesson for me: never rely on the performance data produced by a
>  > debug build. They are totally different from release builds in many
>  > cases.
>  >
>  > 3) I can later provide provide the implementation of of my previous
>  > patch that does instruction selection
>  >    without using any sorted data-structures and achieves very good
>  > speed-ups due to this fact. But the question is:
>  >    Does it still make sense, if the performance improvement for
>  > optimized builds is minimal?
>  >
>  > For both standard and my implementation of instruction selection
>  > without queues:
>  >   All Dejagnu pass without any problems.
>  >   No problems on MultiSource.
>  >
>  > Please review and tell, if it is OK to commit this patch.
>  > And indicate if the approach without queues is still interesting.
>  >
>  > Thanks,
>  >  -Roman
>  >
>  > 2008/4/24 Evan Cheng <evan.cheng at apple.com>:
>  >> Yes, moving it out of tblegen is a good idea. I don't have strong
>  >> preference as to where it is moved to.
>  >>
>  >> Evan
>  >>
>  >>
>  >>
>  >> On Apr 23, 2008, at 3:31 PM, Dan Gohman wrote:
>  >>
>  >>>
>  >>> On Apr 23, 2008, at 10:27 AM, Roman Levenstein wrote:
>  >>>>
>  >>>> Just to make it clear:
>  >>>> This patch is applied currently to the output of Tablegen.
>  >>>> But the proper patch will need to change the Tablegen to generate
>  >>>> correct *.inc files.
>  >>>> Thinking about it, I have a question:
>  >>>> Right now, the beginning of the generrated instruction selector
>  >>>> files
>  >>>> is always the same, independent of the *.td file and target
>  >>>> architecture.
>  >>>> But it is generated by tablegen still and therefore cannot be
>  >>>> substituted easily, without changing the tablegen.
>  >>>> Question: May be it is better to define this common part in an
>  >>>> include
>  >>>> file, e.g. ISelHeader.h, and then tablegen would generate only the
>  >>>> #include <llvm/CodeGen/ISelHeader.h> line. What do you think?
>  >>>
>  >>> Another option is to move common code into the
>  >>> SelectionDAGISel parent class, which is a common base class
>  >>> for each of the targets' DAGToDAGISel classes.
>  >>>
>  >>> Dan
>  >>>
>  >>> _______________________________________________
>  >>> llvm-commits mailing list
>  >>> llvm-commits at cs.uiuc.edu
>  >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>  >>
>  >> _______________________________________________
>  >> llvm-commits mailing list
>  >> llvm-commits at cs.uiuc.edu
>  >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>  >>
>  > <DAGISel.patch>_______________________________________________
>
>
> > llvm-commits mailing list
>  > llvm-commits at cs.uiuc.edu
>  > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>  _______________________________________________
>  llvm-commits mailing list
>  llvm-commits at cs.uiuc.edu
>  http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>