Page MenuHomePhabricator

TableGen: allow non-leaf ComplexPatterns
ClosedPublic

Authored by t.p.northover on May 14 2014, 7:33 AM.

Details

Reviewers
adasgupt
grosbach
Summary

Hi,

The attached patch is a first step to (hopefully) using less monolithic "ADDRESS!!!" operands for load/store operations on ARM64 while still allowing C++ selection (to avoid AArch64's TableGen pattern nightmare).

The idea is that instead of just being able to use ComplexPatterns as leaf nodes (e.g. "my_pat:$address") you can specify how the results map to named operands on a finer level: "(my_pat GPR64:$base, imm:$offset)".

I believe the only XYZGenDAGISel.inc changed by this patch is the Hexagon one, and looking at the file its changes appear sensible.

The Hexagon change is actually a bugfix that this code revealed, but serves as a reasonable example for the code: what *was* happening was that the Hexagon result instructions were getting the ComplexPattern *input* and a newly-materialized "0" as an operand: the ComplexPattern results were being ignored completely. Since the ComplexPatterns were so trivial this didn't actually affect any output, but it was fairly clearly not intended.

So how does it look?

Cheers.

Tim.

Diff Detail

Event Timeline

t.p.northover retitled this revision from to TableGen: allow non-leaf ComplexPatterns.
t.p.northover updated this object.
t.p.northover edited the test plan for this revision. (Show Details)
t.p.northover added reviewers: grosbach, adasgupt.
t.p.northover added a subscriber: Unknown Object (MLST).
adasgupt edited edge metadata.May 14 2014, 10:32 AM

Thanks for identifying the bug and providing a fix, Tim. We'll take a look at the Hexagon bugfix.

jverma added a subscriber: jverma.May 15 2014, 12:25 PM
def : Pat <(stOp (OpNode (ldOp (addrPred IntRegs:$addr, extPred:$offset)),
                         immPred:$bitend),
                 (addrPred (i32 IntRegs:$addr), extPred:$offset)),
           (MI IntRegs:$addr, extPred:$offset, (xformFunc immPred:$bitend))>;

}

Hi Tim,

Your patch seems to be doing the right thing. However, I still don't understand the implication of your change on the complex patterns, SelectADDRriU6_[0-2], defined in HexagonISelDAGToDAG.cpp. Can you please explain it a little?

Thanks,
Jyotsna

Hi Jyotsna,

Your patch seems to be doing the right thing. However, I still don't understand the
implication of your change on the complex patterns, SelectADDRriU6_[0-2],
defined in HexagonISelDAGToDAG.cpp. Can you please explain it a little?

Sure, suppose someone changed (say) SelectADDRriU2_0(SDValue Addr,
SDValue &Base, SDValue &Offset) so that it examined Addr, looking for
something like (add IntRegs:$base, imm:$offset) and set the Base &
Offset references appropriately.

At the moment what would happen is that a (MemOPw_ADDr_V4 Addr, 0,
whatever) node would be produced. Addr would then go through a second
round of selection (even though SelectADDRriU2_0 had already dealt
with it), leading to multiple instructions and assembly looking like:

r0 = Base + Offset
mem(r0+#0) += whatever

(this won't actually be seen now, because you have a *real* pattern
that looks for "(add IntRegs:$base, extPred:$offset)" and has a higher
complexity so it gets tried first, but imagine some other cunning ruse
by SelectADDRriU2_0 to simplify things).

What I think you'd want to happen instead is that (MemOpw_ADDr_V4
Base, Offset, whatever) is produced and then selection carried on from
Base & Offset if needed. This would give code like:

mem(Base+Offset) += whatever

Interpreting your question completely differently (which may be
helpful for any out-of-tree targets you're considering): if XYZ is a
ComplexPattern and you only use it where both the input and the output
match up to that monolithic entity completely then you should see no
difference. That is, both of these should be unchanged by my work:

def : Pat<(whatever XYZ:$a, ...), (INST XYZ:$a)>;
def : Instruction<(ins XYZ:$a, ...), [(set ..., (whatever XYZ:$a))]>;

XYZ is used in both the input and the output, so everything matches up
just as it did before.

I hope this helps, but please do ask for any clarification if I've not
been clear enough.

Cheers.

Tim.

Thanks for the explanation Tim. It makes perfect sense.

adasgupt accepted this revision.May 19 2014, 10:48 AM
adasgupt edited edge metadata.

LGTM. Jyotsna looked at the patch and she is okay with the Hexagon changes as well.

This revision is now accepted and ready to land.May 19 2014, 10:48 AM
t.p.northover closed this revision.May 20 2014, 5:01 AM

Thanks! I've committed it as r209206.

Tim.