llvm dev - Mar 2009 - [LLVMdev] visitBIT_CONVERT (previous Shouldn't DAGCombine insert legal nodes?)

Hello,
In the combine 2 step (after legalization), in the
DAGCombiner::visitBIT_CONVERT() method, the DAG combiner is replacing an FABS
followed by a BIT_CONVERT, to a BIT_CONVERT followed by an AND
0x7FFFFFFFFFFFFFFF. Everything is 64 bit.
On my target, FABS and BIT_CONVERT are legal in 64 bit, but AND in not legal in
64 bit (is declared custom). So the dag combiner is introducing illegal (not
legal) nodes that generates an abort during the machine instruction selection.
In my understanding, this is a bug because who generates a node has to check if
it is legal, especially if it is generating it after legalization. What
discussed few weeks ago is confirming that (see below).
My questions:
1) Is my understanding right?
2) Is there any bug report generated after the discussion below?
Thanks a lot,
Gabriele

> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu]
> On Behalf Of Scott Michel
> Sent: Tuesday, January 20, 2009 5:53 PM
> To: LLVM Developers Mailing List
> Subject: Re: [LLVMdev] Shouldn't DAGCombine insert legal nodes?
> 
> Duncan:
> 
> DAGCombine is inserting an IllegalOperation after target-specific
> instruction legalization has occurred. I'm inserting the fabs and the
> bitconvert during instruction legalization; DAGCombine is converting
> the fabs/bitconvert to an 'and' on its second (third?) pass.
> 
> 
> -scooter
> 
> On Jan 20, 2009, at 12:24 AM, Duncan Sands wrote:
> 
> > On Tuesday 20 January 2009 07:52:37 Evan Cheng wrote:
> >> Right. DAGCombine will insert *illegal* nodes before legalize.
> >
> > There are two stages of legalization: legalization of types,
> > followed by legalization of operations.  Before type legalization
> > DAGCombine is allowed to create nodes with illegal types and illegal
> > operations.  After type legalization but before operation
> legalization
> > it is allowed to create nodes with illegal operations, but all types
> > must be legal.  After operation legalization it is only allowed to
> > create fully legal nodes.
> >
> > Inside DAGCombine this is specified by two flags:
> >   LegalTypes being true means that all nodes must have legal types.
> >   LegalOperations being true means that all nodes must have legal
> > operations (as well as types: LegalTypes will also be true).
> >
> > So if LegalTypes is true and nonetheless a constant with an
> > illegal type is being created then that is a DAG combiner bug.
> >
> > Ciao,
> >
> > Duncan.
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

-----Original Message-----
From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On
Behalf Of Chris Lattner
Sent: Tuesday, January 20, 2009 11:23 PM
To: LLVM Developers Mailing List
Subject: Re: [LLVMdev] Shouldn't DAGCombine insert legal nodes?

On Jan 20, 2009, at 12:32 PM, Scott Michel wrote:
> Eli:
>
> Legal constants would be all well and good for most platforms. I
> don't think that CellSPU is alone in its support for i64 constants
> (in fact, the comment in DAGCombine says that the 64-bit constant is
> inserted to "avoid a constant pool spill").
>
> In many respects, DAGCombine and operation Legalize are co-routines,
> not separate passes.
i32 immediates are not materializable in a single instruction on  
PowerPC or ARM.  They claims that they are legal and then matches them  
with the appropriate expansion.  Can you do a similar thing on Cell?

In any case, the bug fix is probably really simple: the dag combine  
xform for fabs probably just needs to check to see if ISD::CONSTANT is  
legal if post-legalize

-chris
_______________________________________________
LLVM Developers mailing list
LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Mar 5, 2009, at 12:15 PM, Duncan Sands wrote:
>> 2) Is there any bug report generated after the discussion below?
>
> I don't know, sorry.
Gabriele, Duncan, et. al.:

No, I never created a bug report. I did post some thoughts in a post  
entitled "DAGCombiner rants", expressing my frustration. Your best  
bet is to make your backend lower 64-bit constants during instruction  
selection (the ISelDAGtoDAG.cpp source) rather than fight  
DAGCombiner. Fighting DAGCombiner is a losing battle.

I would definitely concur that the whole concept of Legal vs. Custom  
vs. Promote vs. Expand needs a rethink and refactoring. It seems to  
me that the functionality is now badly overloaded, now that they mean  
different things during different DAG to instruction selection phases.


-scooter

> Historically nodes marked "custom" were considered legal, so the
> DAGCombiner would have been correct to generate it.  Not sure how
> that ever worked though.  I think Dan split the isOperationLegal
> method into isOperationLegal and isOperationLegalOrCustom for reasons
> related to this kind of thing.  I don't know whether the DAGCombiner
> is now only supposed to produce legal non-custom nodes.
In my understanding, "Custom" means that I'm in charge to build
the node. So, "Custom" means "Legal" once the node has been
build by my code (through my implementation of the
TargetLowering::LowerOperation() method).
The DAGCombiner could call TargetLowering::LowerOperation() when it needs a
"Custom" node, but it doesn't. So, it knows that the node needs a
custom build but it doesn't ask anything to the guy (the code) in charge of
building custom nodes.
The combiner is replacing some nodes by some other nodes because it thinks this
is better. So, anyway, it is preferable to not use custom nodes because these
nodes can result in a higher number of nodes. In my case, the "Custom"
i64 AND node should be replaced by 2 i32 AND modes and few glue nodes, making
the final DAG more complex than before the combine step. This makes no sense.
I plan to solve this problem by doing the reverse operation the combiner did.
During selection (in SelectionDAGISel::Select()), I check for the node pattern
having a i64 AND between an i64 constant 0x7FFFFFFFFFFFFFFF and a bit_convert,
and I replace this by a fabs and a bit_convert node. In this way, this portion
of the DAG will look as before the "combine 2" step. I think this is
not so far from what Michel suggested.
Have I to make a bug report ?
Is there any predictable order on which all nodes are going through
SelectionDAGISel::Select().
Thanks a lot,
Gabriele