[llvm-commits] [llvm] r55886 - /llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

[Bill Wendling]

On Sep 8, 2008, at 11:30 AM, Dan Gohman wrote:

> On Sep 8, 2008, at 11:03 AM, Bill Wendling wrote:
>
>> On Mon, Sep 8, 2008 at 1:16 AM, Evan Cheng <evan.cheng at apple.com>
>> wrote:
>>>
>>> On Sep 7, 2008, at 5:35 PM, Bill Wendling wrote:
>>>
>>>> It's not a bug in that this wasn't being done. I was doing a static
>>>> analysis of the DAG combiner and noticed that this wasn't doing  
>>>> what
>>>> the comment claimed it should be doing. If this transformation is
>>>> performed in the selection DAG code, then it should probably be
>>>> removed from here entirely (along with the analogous "ADD"
>>>> transformation). What do you think?
>>>
>>> No. The dag combiner xforms can still trigger. The issue is the
>>> operand can be updated by something like ReplaceAllUsesWith which
>>> will
>>> not transform the uses.
>>>
>> Okay, so should the dag combiner transforms be no-ops or actually do
>> something?
>
> We want folding to happen both at getNode time and DAGCombiner time.
> I think it would be possible to refactor the code to eliminate the
> code redundancy though, by moving getNode's folding code out
> into something like this:
>
>   SDValue foldBinaryOp(ISD::NodeType Opcode, SDValue LHS, SDValue  
> RHS);
>
> It would return a null SDValue instead of creating a new node in the
> case that it doesn't do a fold. Then getNode and DAGCombiner could  
> both
> use it. This would also have the advantage of making it easier to do
> more folding at getNode time, which would make SelectionDAG more
> efficient and make some existing optimizations more effective. What
> do you think?
>
That sounds fine to me. I much prefer having one blob of code do one  
thing instead of two blobs doing kind-of similar things. :-)

-bw