This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Start rewriting load/store legality rules
ClosedPublic

Authored by arsenm on May 26 2020, 8:21 AM.

Details

Summary

The current set is an incomprehensible mess riddled with ordering
hacks for various limitations in the legalizer at the time of writing,
many of which have been fixed. This takes a very small step in
correcting this.

The core first change is to start checking for fully legal cases
first, rather than trying to figure out all of the actions that could
need to be performed. It's recommended to check the legal cases first
for faster legality checks in the common case. This still has a table
listing some common cases, but it needs measuring whether this really
helps or not.

More significantly, stop trying to allow any arbitrary type with a
legal bitwidth as a legal memory type, and start using the bitcast
legalize action for them. Allowing loads of these weird vector types
produced new burdens we don't need for handling all of the
legalization artifacts. Unlike the SelectionDAG handling, this is
still not casting 64 or 16-bit element vectors to 32-bit
vectors. These cases should still be handled by increasing/decreasing
the number of 16-bit elements. This is primarily to fix 8-bit element
vectors.

Another change is to stop trying to handle the load-widening based on
a higher alignment. We should still do this, but the way it was
handled wasn't really correct. We really need to modify the MMO's size
at the same time, and not just increase the result type. The
LegalizerHelper does not do this, and I think this would really
require a separate WidenMemory action (or to add a memory action
payload to the LegalizeMutation). These will now fail to legalize.

The structure of the legalizer rules makes writing concise rules here
difficult. It would be easier if the same function could answer the
query the query, and report the action to perform at the same
time. Instead these two are split into distinct predicate and action
functions. This is mostly tolerable for other cases, but the
load/store rules get pretty complicated so it's difficult to keep two
versions of these functions in sync.

Diff Detail

Event Timeline

arsenm created this revision.May 26 2020, 8:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 8:21 AM
kerbowa accepted this revision.Jun 6 2020, 12:06 AM

LGTM

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
188

Or 128 or 256-bit elements right?

237

subtarget.

This revision is now accepted and ready to land.Jun 6 2020, 12:06 AM
arsenm marked an inline comment as done.Jun 6 2020, 6:59 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
188

I think loads and some extracts of those should be legal, but they aren't really naturally legal for any operation