This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Support narrowing zextload/sextload
ClosedPublic

Authored by arsenm on Dec 18 2018, 2:08 AM.

Diff Detail

Event Timeline

arsenm created this revision.Dec 18 2018, 2:08 AM
aemerson added inline comments.Jan 7 2019, 1:17 PM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
526

Is this really needed?

arsenm marked an inline comment as done.Jan 17 2019, 2:01 AM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
526

I’m not sure what you mean? Otherwise you’ll end up with an extload with the same source and dest size (which I think the verifier should reject)

dsanders added inline comments.Jan 18 2019, 10:42 AM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
526

I agree the verifier should reject it. 's32 = G_ZEXTLOAD ... :: (load 4 from ...)' isn't exactly wrong but it serves no purpose compared to 's32 = G_LOAD ... :: (load 4 from ...)'

540

However, this should be invalid for the same reason, 's32 = G_SEXT s32' doesn't make sense. It would be better for the case where it lowers to G_LOAD to just replace the opcode in place. If we have observers plumbed into the legalizer yet we should also call the changingInstr()+changedInstr() events.

lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
210

This is redundant given that clampScalar() limits the type to exactly s32. It's nice for consistency but it costs a small amount of compile-time to perform the test.

arsenm marked an inline comment as done.Jan 18 2019, 9:45 PM
arsenm added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
540

I don't see why this would actually happen? I suppose that depends on what the constraints are for the specified legalize actions. I don't think you should be allow to specify narrow on a type to the same type. Assuming narrowing a load to the same type isn't allowed, this shouldn't be an issue.

dsanders accepted this revision.Jan 22 2019, 9:42 AM

LGTM

lib/CodeGen/GlobalISel/LegalizerHelper.cpp
540

Sorry, I was thinking that if the code above selects the G_LOAD then we can end up with a G_[SZ]EXT that doesn't extend but that can only happen if the G_[SZ]EXTLOAD was invalid in the first place.

This revision is now accepted and ready to land.Jan 22 2019, 9:42 AM
arsenm closed this revision.Jan 22 2019, 11:02 AM

r351856