This is an archive of the discontinued LLVM Phabricator instance.

[globalisel] Introduce LegalityQuery to better encapsulate the legalizer decisions. NFC.
ClosedPublic

Authored by dsanders on Jan 18 2018, 8:42 AM.

Details

Summary

getAction(const InstrAspect &) const breaks encapsulation by exposing
the smaller components that are used to decide how to legalize an
instruction.

This is a problem because we need to change the implementation of
LegalizerInfo so that it's able to describe particular type combinations
rather than just cartesian products of types.

For example, declaring the following

setAction({..., 0, s32}, Legal)
setAction({..., 0, s64}, Legal)
setAction({..., 1, s32}, Legal)
setAction({..., 1, s64}, Legal)

currently declares these type combinations as legal:

{s32, s32}
{s64, s32}
{s32, s64}
{s64, s64}

but we currently have no means to say that, for example, {s64, s32} is
not legal. Some operations such as G_INSERT/G_EXTRACT/G_MERGE_VALUES/
G_UNMERGE_VALUES has relationships between the types that are currently
described incorrectly.

Additionally, G_LOAD/G_STORE currently have no means to legalize non-atomics
differently to atomics. The necessary information is in the MMO but we have no
way to use this in the legalizer. Similarly, there is currently no way for the
register type and the memory type to differ so there is no way to cleanly
represent extending-load/truncating-store in a way that can't be broken by
optimizers (resulting in illegal MIR).

This patch introduces LegalityQuery which provides all the information
needed by the legalizer to make a decision on whether something is legal
and how to legalize it.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Jan 18 2018, 8:42 AM
bogner added a subscriber: bogner.Jan 18 2018, 12:58 PM
bogner added inline comments.
include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
273–276 ↗(On Diff #130422)

The bit about returning a pair in the doc isn't accurate anymore.

Could we return a small pod struct with named fields instead of a tuple here? Tuples really don't have any advantage over simple structs in C++11 and later, IMO, and it's a lot harder to tell what's happening with them. I guess that would probably be a separate change, given that similar tuples are used elsewhere already.

dsanders added inline comments.Jan 18 2018, 1:09 PM
include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
273–276 ↗(On Diff #130422)

The bit about returning a pair in the doc isn't accurate anymore.

Good point. I'll fix that

Could we return a small pod struct with named fields instead of a tuple here? Tuples really don't have any advantage over simple structs in C++11 and later, IMO, and it's a lot harder to tell what's happening with them. I guess that would probably be a separate change, given that similar tuples are used elsewhere already.

Sure. I went with a tuple since I was trying to bring it into line with the other getAction() below but it would be better to change both of them to a struct

rovka added inline comments.Jan 18 2018, 1:11 PM
include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
56 ↗(On Diff #130422)

Is it really a good idea to store an ArrayRef in the long run?

dsanders added inline comments.Jan 18 2018, 3:05 PM
include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
56 ↗(On Diff #130422)

This object is only needed for the duration of a single call to getAction() so there shouldn't be any problems with LegalityQuery::Types outliving the array used to initialize it.

dsanders updated this revision to Diff 130929.Jan 22 2018, 11:08 AM
dsanders marked 2 inline comments as done.

Change tuple to a struct
Update unit tests
Add comment explaining that LegalityQuery does not take a copy of the types array.

reames resigned from this revision.Jan 22 2018, 4:19 PM

Not really a qualified reviewer for this, but I'll comment this reads more cleanly to someone unfamiliar with the details of the code.

bogner accepted this revision.Jan 23 2018, 11:43 AM

LGTM. I have a couple of style comments you can take or leave as you see fit.

include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
62–63 ↗(On Diff #130929)

Pretty sure there's no need to spell this out. For a struct/class with only public members and no constructor you can aggregate-initialize with an initializer-list.

130–133 ↗(On Diff #130929)

I was going to say that we usually name the parameter RHS and do the std::tie(A, B) == std::tie(RHS.A, RHS.B) pattern here, but a quick look shows that we're not at all consistent about that, so I guess you can do whatever you think is clearest.

This revision is now accepted and ready to land.Jan 23 2018, 11:43 AM
dsanders marked 2 inline comments as done.Jan 24 2018, 9:19 AM
dsanders added inline comments.
include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
62–63 ↗(On Diff #130929)

I've removed it from this one. The one in LegalizeActionStep seems to be necessary though. The unit tests don't build without it

This revision was automatically updated to reflect the committed changes.
dsanders marked an inline comment as done.