[LLVMdev] DAGCombiner rant

[Scott Michel]

On Wed, Jan 28, 2009 at 1:42 PM, Dan Gohman <gohman at apple.com> wrote:

> Hi Scott,
>
> I'm not clear on what you're saying here; some of your
> points below seem to be contradictory. The advice to
> use target-independent nodes when feasible seems
> sound to me, so I wrote up a comment about it in
> SelectionDAGNodes.h. If you can formulate your
> thoughts in the form of specific documentation changes,
> that would be helpful.


Sorry if I wasn't clear, since I was venting some of my frustration with my
continual battle with DAGCombine and the CellSPU backend. DAGCombine should
operate according to the principle of least surprise; yet, I'm continually
being surprised.

My biggest frustration, until recently, was finding new i64 constants being
generated by DAGCombine after operation legalization. Prior to that,
InstCombine was inserting i64 multiplications; I ended up biting the bullet
and generating the full 48+ instruction sequence for i64 mul for CellSPU in
terms of the i16 mulipliers that Cell's SPU actually support.I moved all of
i64 constant generation to instruction selection, when originally, i64
constants were legalized by target-specific nodes.

My approach now is the path of least pain and better citizenship within the
LLVM framework itself.

Specfic guidance for backend developers:

(a) Avoid target-specific nodes.
(b) Legalize operations in terms of existing ISD instructions.
(c) Don't be surprised if DAGCombiner is too smart for your backend's
capabilities.
(d) The size and scope of changing DAGCombiner is daunting, so if you can
avoid changing or altering DAGCombiner, don't change DAGCombiner. For
example, how many changes need to be made to check of i64 constants are
legal during a particular phase of DAGCombiner -- you have to do a
__complete__ job, not just patch the place that just bit you in the
backside.
(e) Handle as much target-specific <foo> during instruction selection (e.g.,
i64 constants). IFF there's no way to handle <foo> during instruction
selection, then consider adding a new target-specific node for operation
legalization.
(f) If in doubt about any of the above suggestions, refer to (e) and (a).

Regarding (d), I'd rather do a complete job than a half-baked job if I were
going to submit a patch against DAGCombiner. Looking at the code, even using
NetBeans' IDE, tracking down "illegal" i64 constants is a daunting task. The
path of least pain was to move i64 constant generation to instruction
selection and leave DAGCombiner alone. Basically, the guidance is "Do as
much of what you can during instruction selection, rewrite operations as
target-independent nodes insofar as feasible, and only as a last resort, add
new target-specific nodes because the pain threshold for altering
DAGCombiner or SelectionDAGLegalize is sufficiently intolerable." (Yes, all
one needs to look for are occurances of "DAG->getConstant(MVT::i64", but
there's the possibility of either getting it wrong or being incomplete.)

In (e), however, most backed developers will immediately notice that they
have to run a set of predicates that will be executed a second time by the
generated Select() code. There's probably a better way to do that.

Example: Cell's SPU is a vector processor with a "unform register set",
meaning that scalars are always in slot 0 of the vector (mostly.) Unformity
has to be preserved at all times. Hence, at least in CellSPU's case, i64
constants __have__ to be represented as BUILD_VECTOR nodes and the various
selection predicates are run against the node to determine if the result is
a simple constant load that propagates to all vector elements or whether a
constant pool spill needs to occur (depends on the constant and avoiding a
CP spill.) If a predicate succeeds, then the generated Select() method is
invoked, which runs all of the predicates a second time.

Hope that clears up some of the confusion.


-scooter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090130/04507975/attachment.html>