This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Lower fixed length FP minnum/maxnum
ClosedPublic

Authored by cameron.mcinally on Aug 11 2020, 9:10 AM.

Details

Summary

Cherry-picked from D71767 with added tests.

Diff Detail

Event Timeline

cameron.mcinally requested review of this revision.Aug 11 2020, 9:10 AM
paulwalker-arm added inline comments.Aug 11 2020, 9:28 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3680

I doubt you need the usesSVEFor... test or llvm_unreachable part. I went a bit overboard for my original patch. LowerToPredicatedOp has enough asserts to raise an alarm if something weird starts happening.

llvm/test/CodeGen/AArch64/sve-fixed-length-fp-minmax.ll
559

I'm happy with the tests, but just wanted to raise awareness of the "other" style of writing tests as shown by D85724.

Without going silly with covering all possible cases there's two options. Stronger type legalisation tests as used by this patch and stronger "end result" testing as used by D85724.

Both are equally valid, it just comes down to whether you think type legalisation is already pretty much covered by other instruction tests and possible also which allows for better protection.

As I say, I'm not expecting any changes here, I'm just presenting the options.

Update based on @paulwalker-arm's comments.

cameron.mcinally marked 2 inline comments as done.Aug 11 2020, 12:25 PM
cameron.mcinally added inline comments.
llvm/test/CodeGen/AArch64/sve-fixed-length-fp-minmax.ll
559

Ah, ok. Makes sense. I had originally based the tests off of FADD, so that's where I got the legalization style from. Updated to the end result style now.

Hopefully I didn't make any mistakes. Updating all these fixed VLs is mind numbing...

paulwalker-arm accepted this revision.Aug 11 2020, 4:11 PM
This revision is now accepted and ready to land.Aug 11 2020, 4:11 PM
This revision was automatically updated to reflect the committed changes.
cameron.mcinally marked an inline comment as done.