This is an archive of the discontinued LLVM Phabricator instance.

[globalisel] Decouple src pattern operands from dst pattern operands.
ClosedPublic

Authored by dsanders on Feb 8 2017, 4:34 AM.

Details

Summary

This isn't testable for AArch64 by itself so this patch also adds
support for constant immediates in the pattern and physical
register uses in the result.

The new IntOperandMatcher matches the constant in patterns such as
'(set $rd:GPR32, (G_XOR $rs:GPR32, -1))'. It's always safe to fold
immediates into an instruction so this is the first rule that will match
across multiple BB's.

The Renderer hierarchy is responsible for adding operands to the result
instruction. Renderers can copy operands (CopyRenderer) or add physical
registers (in particular %wzr and %xzr) to the result instruction
in any order (OperandMatchers now import the operand names from
SelectionDAG to allow renderers to access any operand). This allows us to
emit the result instruction for:

%1 = G_XOR %0, -1 --> %1 = ORNWrr %wzr, %0
%1 = G_XOR -1, %0 --> %1 = ORNWrr %wzr, %0

although the latter is untested since the matcher/importer has not been
taught about commutativity yet.

Added BuildMIAction which can build new instructions and mutate them where
possible. W.r.t the mutation aspect, MatchActions are now told the name of
an instruction they can recycle and BuildMIAction will emit mutation code
when the renderers are appropriate. They are appropriate when all operands
are rendered using CopyRenderer and the indices are the same as the matcher.
This currently assumes that all operands have at least one matcher.

Finally, this change also fixes a crash in
AArch64InstructionSelector::select() caused by an immediate operand
passing isImm() rather than isCImm(). This was uncovered by the other
changes and was detected by existing tests.

Depends on D29711

Event Timeline

dsanders created this revision.Feb 8 2017, 4:34 AM

This is currently missing a tablegen test. I'll add that soon.

dsanders updated this revision to Diff 87658.Feb 8 2017, 7:41 AM

Tablegen test case and a fixed a compile error.

dsanders edited the summary of this revision. (Show Details)Feb 8 2017, 7:45 AM
rovka added inline comments.Feb 10 2017, 5:25 AM
utils/TableGen/GlobalISelEmitter.cpp
449

Do you really need typename here or is it a leftover?

513

This and AddRegisterRenderer could each use a comment (either here or in the RendererKind enum).

596

Typo: built

637

Nit: Since emitCxxRenderStmts may emit more than one statement in the general case, I would move this into the function itself and let it deal with all the indentation and newlines that are needed.

863

This is very copy-pasted from above, would it make sense to extract it into, say, a processChildren() taking a Handler of some sort? Only if you think it won't make things more difficult in the future, ofc.

877

...or an MBB :)

dsanders updated this revision to Diff 88733.Feb 16 2017, 7:44 AM
dsanders marked 4 inline comments as done.

Addressed the review comments.

dsanders marked 2 inline comments as done.Feb 16 2017, 7:45 AM
dsanders added inline comments.
utils/TableGen/GlobalISelEmitter.cpp
449

It's a copy/paste from predicates_begin(). It's needed there but isn't here.

863

The reason for separating the two is that the source pattern and destination pattern have different jobs and support different kinds of DAG. It's also possible for the two patterns to be very different in structure. For example:

 (add (mul $src1:GPR32, $src2:GPR32), $src3:GPR32)
=>
 (SMADDLrrr $src1:GPR32, $src2:GPR32, $src3:GPR32)

It might be possible to turn it into a processChildren() + handler setup. I'm going to be trying to turn this into a proper tree walker soon to support nested instructions.

877

Thanks. I should have changed that before splitting the loop into two :-)

dsanders marked 2 inline comments as done.Feb 23 2017, 7:15 AM

Just to double-check, is this ok to commit?

rovka accepted this revision.Feb 23 2017, 8:29 AM

LGTM.

This revision is now accepted and ready to land.Feb 23 2017, 8:29 AM
dsanders closed this revision.Feb 24 2017, 7:55 AM