Mondada Gabriele
2009-Mar-05 16:43 UTC
[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
Scott Michel
2009-Mar-07 00:46 UTC
[LLVMdev] visitBIT_CONVERT (previous Shouldn't DAGCombine insert legal nodes?)
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
Mondada Gabriele
2009-Mar-10 10:25 UTC
[LLVMdev] visitBIT_CONVERT (previous Shouldn't DAGCombine insert legal nodes?)
> 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
Possibly Parallel Threads
- [LLVMdev] visitBIT_CONVERT (previous Shouldn't DAGCombine insert legal nodes?)
- [LLVMdev] visitBIT_CONVERT (previous Shouldn't DAGCombine insert legal nodes?)
- [LLVMdev] visitBIT_CONVERT (previous Shouldn't DAGCombine insert legal nodes?)
- [LLVMdev] Shouldn't DAGCombine insert legal nodes?
- [LLVMdev] Possible DAGCombiner or TargetData Bug