llvm dev - Mar 2009 - [LLVMdev] visitBIT_CONVERT (previous 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

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

Hi Gabriele,
> 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).
if the LegalOperations flag is set in the DAGCombiner, then
it is not allowed to create illegal operations out of legal
operations.
> My questions:
> 1) Is my understanding right?
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.
> 2) Is there any bug report generated after the discussion below?
I don't know, sorry.

Ciao,

Duncan.