This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Allow targets to specify legality of extloads' result type (in addition to the memory type)
ClosedPublic

Authored by ab on Dec 4 2014, 10:55 AM.

Details

Summary

The *LoadExt* legalization handling used to only have one type, the memory type. This forced users to assume that as long as the extload for the memory type was declared legal, and the result type was legal, the whole extload was legal.

However, this isn't always the case. For instance, on X86, with AVX, this is legal:

v4i32 load, zext from v4i8

but this isn't:

v4i64 load, zext from v4i8

Whereas v4i64 is (arguably) legal, even without AVX2.

Keep in mind that this is a draft to give an idea, I mechanically changed the *LoadExt* calls in the generic and X86 parts.
I still need to change other in-tree targets, tell out-of-tree backend maintainers to change their code, and run the testsuite.

Note that the same thing was done a while ago for truncstores (r46140), but I assume no one needed it yet for extloads, so here we go.

Diff Detail

Repository
rL LLVM

Event Timeline

ab updated this revision to Diff 16936.Dec 4 2014, 10:55 AM
ab retitled this revision from to [SelectionDAG] Allow targets to specify legality of extloads' result type (in addition to the memory type).
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added reviewers: chandlerc, grosbach, hfinkel.
ab added a subscriber: Unknown Object (MLST).
ab updated this object.Dec 4 2014, 11:04 AM
arsenm accepted this revision.Dec 4 2014, 11:12 AM
arsenm added a reviewer: arsenm.
arsenm added a subscriber: arsenm.

Thanks for working on this, this has been bothering me for a long time. This LGTM besides few formatting nits where using a variable would avoid ugly line wrappings.

I have a bunch of R600 tests for this lying around somewhere where I was hacking around this problem for extloads to i64

lib/CodeGen/CodeGenPrepare.cpp
2982–2983 ↗(On Diff #16936)

Using a variable for TLI->getValueType(I->getType()) above would avoid this ugly wrapping (and would also be usable above)

lib/Target/X86/X86ISelLowering.cpp
888–889 ↗(On Diff #16936)

Variable for the cast would avoid wrapping

This revision is now accepted and ready to land.Dec 4 2014, 11:12 AM
ab added inline comments.Dec 5 2014, 10:07 AM
lib/CodeGen/CodeGenPrepare.cpp
2982–2983 ↗(On Diff #16936)

Good point, r223491 for the variables only.

lib/Target/X86/X86ISelLowering.cpp
888–889 ↗(On Diff #16936)

Right, as I was changing these I added iterator ranges in D6537; reviews welcome!

hfinkel edited edge metadata.Dec 8 2014, 6:03 AM

This also looks good to me -- this is the right thing to do -- as you've mentioned, you need to update the other in-tree targets before this lands.

ab added a comment.Jan 7 2015, 3:08 PM

FYI: while changing other targets, I decided to flip the order of
MemVT and ValVT in [gs]etLoadExtAction. In this patch, we had:

setLoadExtAction(.., MVT ValVT, MVT MemVT, ..)
setTruncStoreAction(MVT ValVT, MVT MemVT, ..)

I think it's more useful to keep "source" and "destination" types
consistent, that is:

setLoadExtAction(.., MVT MemVT, MVT ValVT, ..)
setTruncStoreAction(MVT ValVT, MVT MemVT, ..)

This avoids having consecutive calls to both, with the same arguments,
but different ordering.
I'll also rename them to SrcVT/DestVT to make that clear.

Anyway, thanks for the reviews,

-Ahmed

This revision was automatically updated to reflect the committed changes.