Page MenuHomePhabricator

DAG: Try scalarizing when expanding saturating add/sub
ClosedPublic

Authored by arsenm on Tue, Jul 14, 5:41 AM.

Details

Summary

In an upcoming AMDGPU patch, the scalar cases will be legal and vector
ops should be scalarized, rather than producing a long sequence of
vector ops which will also need to be scalarized.

Use a lazy heuristic that seems to work and improves the thumb2 MVE
test.

Diff Detail

Event Timeline

arsenm created this revision.Tue, Jul 14, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 14, 5:41 AM

Tests sound fine to me. They are mostly the "don't crash" kind of tests, we don't have native codegen for.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7327

Would this ever happen? Having the operation legal on a smaller vector width, without it being legal in the higher size?

7329

64bit VSelect could actually be fine on MVE. Other operations like add and settcc would actually be more trouble.
Should we at least be testing for something like !isOperationLegalOrCustom(OverflowOp, VT) too?

arsenm marked 2 inline comments as done.Tue, Jul 14, 9:05 AM
arsenm added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7327

Yes, this ends up being the the case for all 16-bit vector operations for AMDGPU (which we just hack around with custom lowering). The vector types are all legal, so they reach the final legalization phase but really need to be split into <2 x i16> operations

7329

Tried that first, but didn't work for x86

dmgreen accepted this revision.Thu, Jul 16, 5:30 AM

Righteo. Looks OK to me.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7329

OK. I can see that. I guess it might actually be the ADD or the setcc that could be checked, either of which would be expanded on MVE.

VSELECT is probably fine for the moment. And if we need to adjust this if VSELECT did become legal, we always can.

This revision is now accepted and ready to land.Thu, Jul 16, 5:30 AM