Page MenuHomePhabricator

[RISCV] Use tail agnostic if inserting subvector/element at the end of the vector.
AbandonedPublic

Authored by khchen on May 13 2022, 6:51 AM.

Details

Summary

Add a policy operand for RISCVISD::VSLIDEUP.

Diff Detail

Event Timeline

khchen created this revision.May 13 2022, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 6:51 AM
khchen requested review of this revision.May 13 2022, 6:51 AM
rogfer01 added inline comments.May 19 2022, 12:17 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4429

I'd use "at the end of the vector" (I think "latest" is about time)

5360

I'm a bit confused here, probably I misunderstood what we mean by the end of the vector.

In the following testcase taken from fixed-vectors-fp-shuffles.ll

define void @insert_v32i1_v8i1_16(<32 x i1>* %vp, <8 x i1>* %svp) #0 {
  %v = load <32 x i1>, <32 x i1>* %vp, align 4
  %sv = load <8 x i1>, <8 x i1>* %svp, align 1
  %c = call <32 x i1> @llvm.experimental.vector.insert.v32i1.v8i1(<32 x i1> %v, <8 x i1> %sv, i64 16)
  store <32 x i1> %c, <32 x i1>* %vp, align 4
  ret void
}

declare <32 x i1> @llvm.experimental.vector.insert.v32i1.v8i1(<32 x i1> %v, <8 x i1> %sv, i64 %idx)

Once the mask types are promoted to integer types we have: OrigIdx == 2, VecVT == v4i8, and SubVecVT == v1i8. My impression is that the end of the vector in this case would be when OrigIdx == 3. Does this align with your expectation or my notion of "end" is incorrect?

The condition check does 2 + 1 + 1 == 4 so we make this tail agnostic. Perhaps, not sure, the last + 1 is not needed (it was needed for the insert element case because you were checking the index in the vector).

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1843

Typo vslideup

frasercrmck added inline comments.May 19 2022, 12:54 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4429

+1

5360

I think you're right, Roger. Even if you're inserting 8 elements into an 8-element vector at position zero, we'd want that as tail agnostic, right? Currently we'd miss that as 0 + 8 + 1 != 8. (I don't know if that's technically legal with INSERT_SUBVECTOR but I think the maths works out the same)

khchen planned changes to this revision.May 19 2022, 7:59 AM
khchen added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5360

Yes, you both are right, sorry for confusing. It should be "inserting at the end of the vector".
my code is wrong. I will fix it.

reames added a subscriber: reames.May 19 2022, 8:18 AM

Skimming through the code, is there any value in having a policy arg on slideup? If so, I might suggest adding it to both in an NFC change, and then doing a separate change to show the opt quality improvement.

Skimming through the code, is there any value in having a policy arg on slideup? If so, I might suggest adding it to both in an NFC change, and then doing a separate change to show the opt quality improvement.

Did you mean slidedown?

Skimming through the code, is there any value in having a policy arg on slideup? If so, I might suggest adding it to both in an NFC change, and then doing a separate change to show the opt quality improvement.

Did you mean slidedown?

Yep, sorry for the silly typo.

khchen abandoned this revision.May 24 2022, 8:55 AM

Skimming through the code, is there any value in having a policy arg on slideup? If so, I might suggest adding it to both in an NFC change, and then doing a separate change to show the opt quality improvement.

@reames thanks, I agree with you!
This patch changes policy from TU to TA at four places. It's difficult to review the improvement.
I will split them into smaller patches.