This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Lower fixed length vector integer SMIN/SMAX
ClosedPublic

Authored by cameron.mcinally on Aug 12 2020, 2:34 PM.

Details

Summary

Similar patch for unsigned MIN/MAX coming soon. I wanted to keep the Diff small since there are a lot of tests.

Diff Detail

Event Timeline

cameron.mcinally requested review of this revision.Aug 12 2020, 2:34 PM
This revision is now accepted and ready to land.Aug 12 2020, 2:40 PM
paulwalker-arm added inline comments.Aug 13 2020, 3:28 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-minmax.ll
25

SMAX

31

Has something happened here meaning you could not match the expect register usage?

Same goes for the other 64/128-bit tests.

38

For the 128-bit tests you can pass the data in as parameters and return the result directly.

I can see smin_v2i32 and smin_v4i32 are good so just need to ensure the other 64/128-bit tests are consistent with those.

307–335

I hit a similar problem with i64 based multiplies. LowerToPredicatedOp now has an OverrideNEON parameter that allows the use of SVE instructions even for NEON sized vectors (although only enabled when wide vectors are enabled until we build up enough confidence to make it the default).

You'll also need a couple of extra setOperations lines, just search for "These operations are not supported on NEON but SVE can do them"

412

SMIN

Updated based on Paul's review. Inline comments to come shortly...

cameron.mcinally marked 4 inline comments as done.Aug 13 2020, 8:16 AM

Actually, I made a mistake. New patch coming soon...

Updated patch based on Paul's comments.

cameron.mcinally marked an inline comment as done.Aug 13 2020, 8:32 AM
cameron.mcinally added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3643

Is this what you had in mind, @paulwalker-arm?

paulwalker-arm accepted this revision.Aug 13 2020, 8:51 AM

Thanks @cameron.mcinally, just a final typo to fix, plus a decision as to which version you'd rather land.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3643

Yes, although this and the previous version were both valid.

For LowerMul I needed the explicit i64 check because the function is called for other element types, whereas in this instance you know only those types requested by this patch will be lowered.

If something weird happens the previous version would still work because it's safe to lower the i8, i16 and i32 vector element types to SVE, whereas the new version will likely crash and burn.

Personally I prefer the resilience of the previous version but since these are failure cases I doubt that matters plus an assert's build will show where somebody has gone wrong.

llvm/test/CodeGen/AArch64/sve-fixed-length-int-minmax.ll
43

Rouge ";"

cameron.mcinally marked 2 inline comments as done.Aug 13 2020, 9:00 AM
cameron.mcinally added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3643

If something weird happens the previous version would still work because it's safe to lower the i8, i16 and i32 vector element types to SVE, whereas the new version will likely crash and burn.

That's a good point. I'll go with the old version. Will commit now...

This revision was automatically updated to reflect the committed changes.
cameron.mcinally marked an inline comment as done.