Improve min/max vector reductions on arm

[calebzulawski]

Summary

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.

[dmgreen]

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 https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch. It can make them easier to review.

llvm/lib/Target/ARM/ARMISelLowering.cpp

250

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.

10288

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

10290

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

10322

Capitalize the U in use.

10334

Op.getValueType()

10335

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

llvm/test/CodeGen/ARM/vecreduce-minmax.ll

3

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.

5

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

[calebzulawski]

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

llvm/lib/Target/ARM/ARMISelLowering.cpp

10288

Whoops, something was a little wonky with the last patch

10335

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

llvm/test/CodeGen/ARM/vecreduce-minmax.ll

5

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

[dmgreen]

Thanks - this looks like a good patch. LGTM.

llvm/lib/Target/ARM/ARMISelLowering.cpp

10351

-> Op.getValueType()

[calebzulawski]

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

[dmgreen]

Can do. What "name <[e-mail]>" should be used for git attribution?

[calebzulawski]

You can use "Caleb Zulawski <[e-mail]>", thanks!