Page MenuHomePhabricator

Improve min/max vector reductions on arm

Authored by calebzulawski on Mar 19 2023, 10:10 PM.



This nearly resolves issues such as #50466, #40981, #38190.

In the added test, the extra long vector reduction seems to skip my added reduction, where should I look for that?

I'm not sure who to add to review this, feel free to add anyone else or remove yourself.

Diff Detail

Event Timeline

calebzulawski created this revision.Mar 19 2023, 10:10 PM
calebzulawski requested review of this revision.Mar 19 2023, 10:10 PM

Sounds like a nice idea. This may want to custom lower 128bit vectors too, to turn them into vpmin.u8 lowhalf, highhalf as a first step.

Can you upload with full context, as per It can make them easier to review.


I think I would move this into one of the if (Subtarget->hasNEON()) { blocks in ARMTargetLowering::ARMTargetLowering. Maybe near all the legal extload setLoadExtAction's. At least that seems to be how things have worked so far.


Is there a missing chunk where this gets called from LowerOperation?


This can just check for if (!ST->hasNEON()) - Neon and MVE are mutually exclusive.


Capitalize the U in use.




Should this do SIGN_EXTEND or ZERO_EXTEND, depending on the sign of the min/max?


Adding -float-abi=hard might simplify the tests a little. Maybe use -mtriple=armv7-none-eabi for a newer target triple too? And the --check-prefix=CHECK is already the default and can be removed.


Can you add some tests for <16 x i8> and other 128bit vector types. They will be quite common.

calebzulawski marked 7 inline comments as done.Mar 20 2023, 8:14 PM

Thanks for the great review! I haven't touched codegen before, this helped quite a bit.


Whoops, something was a little wonky with the last patch


Yes, I think so. I copied it from the MVE vector reduce, but maybe that's doing something different.


Aha, this also had the desired effect on much longer vectors, since they appear to break into 128-bit.

calebzulawski marked 3 inline comments as done.
calebzulawski marked an inline comment as done.
dmgreen accepted this revision.Mar 22 2023, 4:48 AM

Thanks - this looks like a good patch. LGTM.


-> Op.getValueType()

This revision is now accepted and ready to land.Mar 22 2023, 4:48 AM
calebzulawski marked an inline comment as done.Mar 22 2023, 6:16 AM

Thanks! Made that small change. By the way, I don't have commit access, if you or @nikic could commit it for me.

Can do. What "name <>" should be used for git attribution?

You can use "Caleb Zulawski <>", thanks!

This revision was landed with ongoing or failed builds.Mar 22 2023, 9:00 AM
This revision was automatically updated to reflect the committed changes.