Page MenuHomePhabricator

[globalisel][tablegen] Map ld and st to G_LOAD and G_STORE. NFC
ClosedPublic

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

Details

Summary

There is an important mismatch between ISD::LOAD and G_LOAD (and likewise for
ISD::STORE and G_STORE). In SelectionDAG, ISD::LOAD is a non-atomic load
and atomic loads are handled by a separate node. However, this is not true of
GlobalISel's G_LOAD. For G_LOAD, the MachineMemOperand indicates the atomicity
of the operation. As a result, this mapping must also add a predicate that
checks for non-atomic MachineMemOperands.

This is NFC since these nodes always have predicates in practice and are
therefore always rejected at the moment.

Depends on D37443

Event Timeline

dsanders created this revision.Sep 4 2017, 10:31 AM
dsanders updated this revision to Diff 118449.Oct 10 2017, 12:20 PM

Rebase and ping

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

The added predicate part looks fine, but I didn't get why we need to change CodeGenInstruction into record just yet.

include/llvm/Target/GlobalISel/SelectionDAGCompat.td
83

Although I get the comment, I don't see how this affect the following definition.
I.e., what in the following definition changed compared to the surrounding ones to take this into account?

88

Typo (G_STORE (G_TRUNCATE x))

utils/TableGen/GlobalISelEmitter.cpp
1996

I don't get why this change is required yet. G_LOAD and G_STORE records are still CodeGenInstruction, aren't they? (I haven't checked)

dsanders added inline comments.Oct 12 2017, 2:07 PM
include/llvm/Target/GlobalISel/SelectionDAGCompat.td
83

I think I've missed a 'For example' in the comment but it would be clearer to explain the Atomic bit as well. I'll update the comment.

88

Thanks for spotting that. I'm not sure what I was thinking there.

utils/TableGen/GlobalISelEmitter.cpp
1996

I think this is another confusing comment. It's still technically correct but it makes it sound like it's a DenseMap<SDNode, Instruction> when it's now DenseMap<SDNode, GINodeEquiv>. I had to change the values of the map to the GINodeEquiv so I could access the new CheckMMOIsNonAtomic field.

qcolombet accepted this revision.Oct 12 2017, 2:21 PM

LGTM with the previous nitpicks fixed.

utils/TableGen/GlobalISelEmitter.cpp
1996

Make sense.
I thought we could have kept CodeGenInstruction and then access the definition of the record to read the CheckMMOIsNonAtomic field, but that wouldn't work given the record would give us the instruction, not the GINodeEquiv.

Thanks for the clarifications.

This revision is now accepted and ready to land.Oct 12 2017, 2:21 PM
dsanders closed this revision.Oct 14 2017, 7:41 PM
dsanders marked 7 inline comments as done.