Page MenuHomePhabricator

[DAGCombiner] Fold sext_inreg of a masked load into a signed extended masked load
ClosedPublic

Authored by samtebbs on Jul 22 2020, 8:49 AM.

Details

Diff Detail

Event Timeline

samtebbs created this revision.Jul 22 2020, 8:49 AM
dmgreen added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11135

I think you need to check for hasOneUse. And maybe with more of the checks from the above fold (sext_inreg (zextload x)) case too, like checking types match and maybe for legal masked loads.

llvm/test/CodeGen/Thumb2/sext-masked-load.ll
22 ↗(On Diff #279839)

Can you simplify this down to just a llvm.masked.load.v4i16.p0v4i16 and a sitofp ?
Ideally they would look something like the tests in llvm/test/CodeGen/Thumb2/mve-masked-load.ll.

If so can you also add some other tests for different types and for uitofp.

samtebbs marked 2 inline comments as done.Jul 23 2020, 7:56 AM
samtebbs added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11135

Thanks, I do need to add those. But for checking if masked loads are legal, is that necessary if there is already a masked load in the DAG?

llvm/test/CodeGen/Thumb2/sext-masked-load.ll
22 ↗(On Diff #279839)

Sure! I'll give that a shot.

dmgreen added inline comments.Jul 23 2020, 8:34 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11135

OK. tryToFoldExtOfMaskedLoad and the load fold above both have a isLoadExtLegal check. That does seem to make some assumptions, but they seem to be valid so far.

It would be a strange architecture that included zext/anyext masked loads, but didn't include sext masked loads. But even if just for consistency it sounds like it would be worth included that isLoadExtLegal check too.

samtebbs updated this revision to Diff 280479.Jul 24 2020, 9:04 AM
samtebbs marked 2 inline comments as done and 2 inline comments as done.

Simplify test and add extra checks to sext masked load generation.

dmgreen added inline comments.Jul 27 2020, 4:42 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11136

This needs a bit of a clang-format.

llvm/test/CodeGen/Thumb2/sext-masked-load.ll
4 ↗(On Diff #280479)

Can you add tests for these legal types:
8xi8 -> half
4xi8 -> float
4xi16 -> float

I think it's worth having some illegal types too, Maybe something like a 4xi32 -> double?

And can you then replicate them for uitofp too. If I'm right that might need some fixup too, but it may be better to handle that in a different patch.

Oh, and do mind moving the test to llvm/test/CodeGen/Thumb2/mve-sext-masked-load.ll to keep it with the others MVE test.

samtebbs updated this revision to Diff 280917.Jul 27 2020, 7:38 AM
samtebbs marked 2 inline comments as done.

Add more tests and clean up checks in visitSIGN_EXTEND_INREG

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11136

+1

llvm/test/CodeGen/Thumb2/sext-masked-load.ll
4 ↗(On Diff #280479)

I've added those tests, thanks for the suggestions (I already had a 4xi16 -> float test, if I understand correctly).

I'd prefer to handle the uitofp cases in a separate patch too.

dmgreen added inline comments.Jul 27 2020, 9:37 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11136

Hmm. One last thing... If the extension type is a SEXTLOAD, should we be removing the sign_ext_inreg anyway?
I guess with NON_EXTLOAD the ExtVT and MemoryVT would not match?

llvm/test/CodeGen/Thumb2/mve-sext-masked-load.ll
3

You likely don't need -tail-predication=enabled any more.

samtebbs updated this revision to Diff 281172.Jul 28 2020, 3:28 AM
samtebbs marked 2 inline comments as done.

Remove -tail-predication option from test and change SEXTLOAD to NON_EXTLOAD in DAGCombiner check.

dmgreen accepted this revision.Jul 28 2020, 3:51 AM

Thanks. LGTM, but please give others a day in case they have comments.

This revision is now accepted and ready to land.Jul 28 2020, 3:51 AM