Page MenuHomePhabricator

[GlobalISel] Handle non-multiples of the base type in narrowScalarInsert
ClosedPublic

Authored by bogner on Mar 2 2021, 11:17 AM.

Details

Summary

When narrowing G_INSERT, handle types that aren't a multiple of the
type we're narrowing to. This comes up if we're narrowing something
like an s96 to fit in 64 bit registers and also for non-byte multiple
packed types if they come up.

This implementation handles these cases by extending the extra bits to
the narrow size and truncating the result back to the destination
size.

Diff Detail

Event Timeline

bogner created this revision.Mar 2 2021, 11:17 AM
bogner requested review of this revision.Mar 2 2021, 11:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 11:17 AM

I've been thinking we should just stop using G_INSERT/G_EXTRACT for generic legalization. They're weak abstractions over INSERT_SUBREG/EXTRACT_SUBREG, and they probably shouldn't form with illegal types/offsets.

I've been thinking we should just stop using G_INSERT/G_EXTRACT for generic legalization. They're weak abstractions over INSERT_SUBREG/EXTRACT_SUBREG, and they probably shouldn't form with illegal types/offsets.

That's probably a reasonable direction but it'll take some doing given that extractParts and insertParts are pretty prevalent in LegalizerHelper today. We need to do something about this if we want any of the other narrowScalar code to do the right thing for these kinds of types.

arsenm added inline comments.Mar 2 2021, 12:28 PM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4719

Does this just break vectors? Part of of the problem is deciding what these even mean for vectors. Does anything break if you just give up on them?

bogner marked an inline comment as done.Mar 2 2021, 1:00 PM
bogner added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4719

This is narrowScalarInsert, you can't get here with a vector. This handling was added as part of a change that updated a number of buildMerge callers and clearly didn't take the context into account.

bogner marked an inline comment as done.May 27 2021, 11:11 AM

@arsenm Other than the high level "we should probably move away from G_EXTRACT/G_INSERT" in legalization" point, do you have any other concerns here? Can we get this in and deal with that more general problem later?

dsanders accepted this revision.Jun 2 2021, 11:42 AM

LGTM (we should wait for Matt to confirm he's ok with dealing with the high-level issue later though)

This revision is now accepted and ready to land.Jun 2 2021, 11:42 AM
arsenm accepted this revision.Jun 2 2021, 11:48 AM
This revision was landed with ongoing or failed builds.Jun 8 2021, 10:14 AM
This revision was automatically updated to reflect the committed changes.