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

Evan Cheng evan.cheng at apple.com
Wed May 7 00:20:48 PDT 2008


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);
+    }

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.

Thanks for doing this!

Evan

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




More information about the llvm-commits mailing list