Page MenuHomePhabricator

[tablegen] Handle common load/store predicates inside tablegen. NFC.
ClosedPublic

Authored by dsanders on Sep 4 2017, 10:18 AM.

Details

Summary

GlobalISel and SelectionDAG require different code for the common
load/store predicates due to differences in the representation.
For example:

SelectionDAG: (load<signext,i8>:i32 GPR32:$addr) // The <> denote properties of the SDNode that are not printed in the DAG
GlobalISel: (G_SEXT:s32 (G_LOAD:s8 GPR32:$addr))

Even without that, differences in the IR (SDNode vs MachineInstr) require
differences in the C++ predicate.

This patch moves the implementation of the common load/store predicates
into tablegen so that it can handle these differences.

It's NFC for SelectionDAG since it emits equivalent code and it's NFC for
GlobalISel since the rules involving the relevant predicates are still
rejected by the importer.

Depends on D36618

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Sep 4 2017, 10:18 AM
dsanders updated this revision to Diff 118448.Oct 10 2017, 12:06 PM

Rebase and ping

qcolombet edited edge metadata.Oct 12 2017, 12:06 PM

Looks mostly good to me.
Comments below.

include/llvm/Target/TargetSelectionDAG.td
656 ↗(On Diff #118448)

That comment reads like an easy way to shot ourselves in the foot :).
Could we add a check that when ImmediateCode is set, none of the following bits are set?

656 ↗(On Diff #118448)

At first, I thought it would be better to (painfully) recover this information from the patfrag name in the GI emitter, but looking at how ImmediateCode is handled, the proposed approach is fine.

674 ↗(On Diff #118448)

Do we really need both NonTruncStore and TruncStore?

679 ↗(On Diff #118448)

Maybe have just one MemoryVT and one ScalarMemoryVT and something to check if that a store or load?

utils/TableGen/CodeGenDAGPatterns.cpp
863 ↗(On Diff #118448)

If we have an unindexed bit and a load/store bit. Then this configuration wouldn't be possible.

dsanders added inline comments.Oct 12 2017, 2:00 PM
include/llvm/Target/TargetSelectionDAG.td
656 ↗(On Diff #118448)

That comment reads like an easy way to shot ourselves in the foot :).

Yep, I'd like to make them unavailable in the *ImmLeaf subclasses but TableGen doesn't have a way to remove fields.

Could we add a check that when ImmediateCode is set, none of the following bits are set?

Sure

674 ↗(On Diff #118448)

I should be able to fold them together. The reason I haven't is pretty weak. It's just that testing getValueAsBitOrUnset() returns false for the unset case so it was slightly simpler to only consider 1 or unset.

679 ↗(On Diff #118448)

Ok

utils/TableGen/CodeGenDAGPatterns.cpp
863 ↗(On Diff #118448)

Ok

dsanders updated this revision to Diff 119048.Oct 14 2017, 6:06 PM
dsanders marked 9 inline comments as done.

Rebase
Check that the pre-packaged predicates are not used with ImmLeaf (by checking in getImmCode()).
Fold IsTruncStore and IsNonTruncStore together.
Fold LoadMemoryVT and StoreMemoryVT together.
Fold LoadScalarMemoryVT and StoreScalarMemoryVT together.
Fold IsUnindexedLoad and IsUnindexedStore together.

This revision was automatically updated to reflect the committed changes.