I just ran across something interesting: DAGCombine inserts a 64-bit constant as the result of converting a (bitconvert (fabs val)) to a (and (bitconvert val), i64const).
The problem: i64 constants have to be legalized for the CellSPU platform. DAGCombine is doing the right thing but it's not doing the right thing for CellSPU and it's damed difficult to work around this "feature". Moreover, the way all of SelectionDAGLegalize and DAGCombne's code is written, it's particularly difficult to "re-legalize" nodes unless one more legalization pass is invoked after DAGCombine.
It's not like it's actually easy to load an i64 constant while maintaining the vector register uniformity that CellSPU expects. In fact, there're a lot of different ways to load a 64-bit constant depending on the constant's value. At worse case, it's a constant pool load on a memory-constrained machine (256K). But it can be a lot better than paying the 6-cycle penalty for memory loads.
OTOH, that's why I created all of that legalization code in the first place so the backend could get it right.
Suggestions apart from doing custom fabs legalization in CellSPU (sorta defeats the purpose of DAGCombine, no)?
-scooter
Before anyone asks: yes using i64 on the Cell's SPUs is a really bad and stupid idea. But, I have to support the data type to make gcc happy.
Nonetheless, i64 constants have to be legalized.
-scooter
I don't think DAGCombine should be doing a transform like that
post-legalize; if it is, it's probably a bug.
-Eli
Right. DAGCombine will insert *illegal* nodes before legalize.
Evan
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.
The issue here isn't that i64 is illegal, it's that constants of type
i64 are illegal. I'm tempted to say that having legal constants
should be a requirement for marking an integer type legal...
-Eli
Evan:
And after legalize too. DAGCombine gets run after legalization. 
-scooter
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
Evan:
And after legalize too. DAGCombine gets run after legalization.
Yes? It nows if it's run after legalization. Like Duncan said, if it's inserting illegal nodes after legalization, then it's a bug.
Evan
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.
-scooter
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
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
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.
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
Hi Gabrielle,
> 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.
how about this instead: modify the DAGCombiner so that before operations
have been legalized, the DAGCombiner is allowed to produce nodes satisfying
isOperationLegalOrCustom, while after operations have been legalized it is
only allowed to produce nodes satisfying isOperationLegal.
Have I to make a bug report ?
Sounds like a good idea.
Is there any predictable order on which all nodes are going through SelectionDAGISel::Select().
I don't know.
Ciao,
Duncan.
how about this instead: modify the DAGCombiner so that before
operations
have been legalized, the DAGCombiner is allowed to produce nodes
satisfying
isOperationLegalOrCustom, while after operations have been legalized it
is
only allowed to produce nodes satisfying isOperationLegal.
Yes, it should be good to modify DAGCombiner in order to avoid generating "unselectable" nodes. For the short-term, I plan to "uncombine" the DAG during selection as described before.
Once again, I think it is not good to replace nodes by "Custom" ones in an optimization (combine) pass. It is better to keep the DAG as it is. So, in my opinion, there is no advantage to use isOperationLegalOrCustom(), even before legalization. The usage of isOperationLegal() should be enough.
Gabriele