llvm dev - Mar 2009 - [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

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