This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][InsertVSETVLI] Avoid VL toggles for extractelement patterns
ClosedPublic

Authored by reames on Apr 20 2023, 12:29 PM.

Details

Summary

Noticed this while looking at some SLP output. If we have an extractelement, we're probably using a slidedown into an destination with no contents. Given this, we can allow the slideup to use a larger VL and clobber tail elements of the destination vector. Doing this allows us to avoid vsetvli toggles in many fixed length vector examples.

Diff Detail

Event Timeline

reames created this revision.Apr 20 2023, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 12:29 PM
reames requested review of this revision.Apr 20 2023, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 12:29 PM
asb added a comment.Apr 21 2023, 10:03 AM

Left a very minor stylistic comment. LGTM for what it's worth, but this needs a review from someone more active in the RVV space.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
836

Would it be better to change this to case LMUL_RESERVED: llvm_unreachable("reserved LMUL value") so -Wswitch will pick up if the enumeration is expanded.

craig.topper added inline comments.Apr 21 2023, 10:28 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
836

Can we use

auto [LMul, Fractional] = RISCVVType::decodeVLMUL(LMUL);
return Fractional || LMul == 1;
865

"a IMPLICIT_DEF" -> "an IMPLICIT_DEF"

reames updated this revision to Diff 517341.Apr 26 2023, 2:58 PM

Address review comment

This revision is now accepted and ready to land.May 1 2023, 9:45 AM

Hi all,

we were observing errors when running the llvm testsuitw with -march=rv64gcv. git bisect pointed to this change.

Failed Tests (2):
  test-suite :: SingleSource/UnitTests/Vector/Vector-constpool.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Can anyone else reproduce this?

Can anyone else reproduce this?

I can reproduce a failure, haven't yet confirmed this patch is guilty due to annoying cross build headaches. Investigating now, will either revert or fix today.

Can anyone else reproduce this?

I think this is now fixed, but my cross build environment for test-suite is a giant collection of hacks. Confirmation that this now works end to end would be appreciated.

Can anyone else reproduce this?

I think this is now fixed, but my cross build environment for test-suite is a giant collection of hacks. Confirmation that this now works end to end would be appreciated.

Thanks a lot @reames, I'll check ASAP.

Can anyone else reproduce this?

I think this is now fixed, but my cross build environment for test-suite is a giant collection of hacks. Confirmation that this now works end to end would be appreciated.

Thanks a lot @reames, I'll check ASAP.

Confirmed these two tests are now back to green. Thanks again for the prompt fix @reames.