This is an archive of the discontinued LLVM Phabricator instance.

[MIPS GlobalISel] Select any extending load and truncating store
ClosedPublic

Authored by Petar.Avramovic on Jan 30 2019, 7:49 AM.

Details

Summary

Make behavior of G_LOAD in widenScalar same as for G_ZEXTLOAD and
G_SEXTLOAD. That is perform widenScalarDst to size given by the target
and avoid additional checks in common code. Targets can reorder or add
additional rules in LegalizeRuleSet for the opcode to achieve desired
behavior.

Select extending load that does not have specified type of extension
into zero extending load.

Select truncating store that stores number of bytes indicated by size
in MachineMemoperand.

Diff Detail

Repository
rL LLVM

Event Timeline

Petar.Avramovic marked an inline comment as done.Jan 30 2019, 7:55 AM
Petar.Avramovic added inline comments.
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
281–284 ↗(On Diff #184300)

Hi Matt, I added this additional rule in order to keep the existing behavior (tests legalize-sextload-flat.mir and legalize-zextload-flat.mir) as I am not sure if G_LOADing one byte into s32 is supported by AMDGPU.

arsenm added inline comments.Jan 30 2019, 7:57 AM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1043–1044 ↗(On Diff #184300)

I have a patch which also drops this, but now that I think about it, I think this is just checking the wrong size. I think this should really be checking for the memory size rather than result size (but all 3 load opcodes should be consistent here)

lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
281–284 ↗(On Diff #184300)

Does anything actually break without this?

If no tests fail, you can drop this. I have several local patches rewriting the load and store legality to handle everything

281–284 ↗(On Diff #184300)

s8->s32 is fine in any address space for any load type

Petar.Avramovic marked an inline comment as done.Jan 30 2019, 8:24 AM
Petar.Avramovic added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1043–1044 ↗(On Diff #184300)

I agree that memory size should be checked, but I think it should not be done here as this is widenScalar and that it should be done in another rule concerning memsize e.g. unsupportedIfMemSizeNotPow2().
Are there any widenScalar sizes that are not target specific and that should be handled in general code?

arsenm accepted this revision.Jan 30 2019, 8:32 AM

LGTM

This revision is now accepted and ready to land.Jan 30 2019, 8:32 AM

Hi, Matt. Here are the updated legalizer and tests for AMDGPU.

arsenm added a comment.Feb 8 2019, 6:14 AM

Are you going to commit this?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 6:27 AM