This is an archive of the discontinued LLVM Phabricator instance.

[ARM] More aggressive matching for vpadd and vpaddl.
ClosedPublic

Authored by efriedma on Dec 14 2016, 3:31 PM.

Details

Summary

This seems to mostly work, but I'm not sure I'm doing it at the right layer. Maybe we should be trying to match this before shuffle lowering?

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma updated this revision to Diff 81488.Dec 14 2016, 3:31 PM
efriedma retitled this revision from to [ARM] More aggressive matching for vpadd and vpaddl..
efriedma updated this object.
efriedma set the repository for this revision to rL LLVM.
efriedma added a subscriber: llvm-commits.
rengolin edited edge metadata.Dec 20 2016, 3:56 AM

Generally, looks good. I have some comments on the format of the code, which could be a bit cleaner on the depths of the ifs and commoning up the checks.

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp
9219 ↗(On Diff #81488)

Why v2i32 only? I can see from your tests that this also matches i8, so I'm suspecting this is already extended by the selection dag before getting here?

Also, VPADD works with floats, too.

9221 ↗(On Diff #81488)

Better to negate this if and move return SDValue() here.

9252 ↗(On Diff #81488)

This if is almost identical to the one above. I'd common them up in a local static function.

9256 ↗(On Diff #81488)

VPADDL works on D and Q regs, not Q only.

9257 ↗(On Diff #81488)

Same comment here, better to negate this if and return SDValue().

efriedma added inline comments.Dec 20 2016, 12:48 PM
lib/Target/ARM/ARMISelLowering.cpp
9219 ↗(On Diff #81488)

This conditional is kind of awkward; I really want to just look for VUZP, but there is no v2i32 VUZP.

(The float vpadd isn't quite the same thing, but I guess I can look into it.)

9256 ↗(On Diff #81488)

Yes... but in that case the type of N00 would be a 32-bit vector, and the whole thing kind of explodes due to the way legalization works. I'll include a testcase for reference.

efriedma updated this revision to Diff 82136.Dec 20 2016, 12:52 PM
efriedma edited edge metadata.

Refactor code; add testcases.

Don't review the new version yet; I have more changes.

efriedma updated this revision to Diff 82159.Dec 20 2016, 2:39 PM

More testcases, some refactoring, bugfix for a stupid mistake.

rengolin added inline comments.Dec 21 2016, 5:34 AM
lib/Target/ARM/ARMISelLowering.cpp
9663 ↗(On Diff #82159)

This looks weird. As much as I have long methods and nested ifs, the interaction between Early and Late increases the coupling between them and make calling code more complex. (for example, you have an early exit on Early for later match on Later).

Adding an if block in AddCombineToVPADDL with an return would be much cleaner, if the condition is clear and concise, which should be the case, with your new helper.

efriedma added inline comments.Dec 21 2016, 10:53 AM
lib/Target/ARM/ARMISelLowering.cpp
9663 ↗(On Diff #82159)

They aren't really coupled like that; when I say "we'll match it to vpadd later", I mean, way later, like after the BUILD_VECTOR is lowered by legalization. Maybe I could rename "Early" and "Late" to be a bit more clear...

Also, my eventual intent is to kill off AddCombineToVPADDLEarly; I'm just leaving it around to avoid regressing the fromExtendingExtractVectorElt_i8 and fromExtendingExtractVectorElt_i16 cases, which get lowered in stupid ways otherwise.

rengolin added inline comments.Dec 22 2016, 8:14 AM
lib/Target/ARM/ARMISelLowering.cpp
9663 ↗(On Diff #82159)

Right, I see what you mean. If you rename to what they match (instead of early/late), they'll become two different calls anyway, just like the rest of them in this function.

It'd also good to add a FIXME on the current Early one, so anyone looking will know.

9219 ↗(On Diff #81488)

Oh, I see! ok.

efriedma updated this revision to Diff 83701.Jan 9 2017, 2:29 PM

Updated comments and function names.

rengolin accepted this revision.Jan 11 2017, 3:13 AM
rengolin edited edge metadata.

This looks much better now, thanks! LGTM.

This revision is now accepted and ready to land.Jan 11 2017, 3:13 AM
This revision was automatically updated to reflect the committed changes.