This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Teach VSETVLI insertion to merge the unused VSETVLI with the one need to be insert after it.
ClosedPublic

Authored by jacquesguan on Jul 27 2021, 2:29 AM.

Details

Summary

If a vsetvli instruction is not compatible with the next vector instruction, and there is no other things that may update or use VL/VTYPE, we could merge it with the next vsetvli instruction that should be insert for the vector instruction.
This commit only merge VTYPE with the former vsetvli instruction which has the same VL.

Diff Detail

Event Timeline

jacquesguan created this revision.Jul 27 2021, 2:29 AM
jacquesguan requested review of this revision.Jul 27 2021, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 2:29 AM
frasercrmck added inline comments.Jul 27 2021, 2:57 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
636

Could this be a property of VSETVLIInfo somehow? It seems odd to track two separate bits of state.

If not I think the variable needs a better name.

678

Is this path not setting VSetMI by inserting a VSETVLI?

llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll
1

It would be helpful if the test case was pre-committed so we can better see the changes introduced by this patch.

jacquesguan added inline comments.Jul 27 2021, 5:30 AM
llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll
1

Thanks, I added the test in D106865.

Add field MI to class VSETVLIInfo for recording the merge target VSET(I)VLI.

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

Done, thanks.

craig.topper added inline comments.Jul 27 2021, 10:06 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
636

Putting it in VSETVLIINfo allocates space for every basic block as part of the global analysis. But it is only used in the function that runs after the global analysis is done. From that perspective I'm not sure it makes sense to be in the class.

690

What if the original vsetvli had a smaller vlmax than the instruction we're inserting for. Won't we lose the clipping that should have existed?

llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll
12

What if a0 is used by another instruction or a pointer increment? This changes VLMAX which changes where AVL gets clipped, potentially breaking the other uses.

jacquesguan added inline comments.Jul 28 2021, 2:22 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
690

Yes, so we can only merge vsetvli only if those two vsetvli have the same AVL and the same VLMAX or we can confirm that the first vsetvli's result won't be used.

Remove MI from VSETVLIInfo, add constraints that should have the same VLMAX for merging vsetvli.

jacquesguan added inline comments.Jul 29 2021, 12:04 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
678

No need to set it here, CurInfo is definitely not from an explicit VSET(I)VLI of this MBB.

jacquesguan added inline comments.Jul 29 2021, 12:07 AM
llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll
12

A new condition added for checking if is a virtual register.

craig.topper added inline comments.Jul 29 2021, 9:15 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
689

Is the hasSameAVL case tested? Both the tests use the output of the vsetvli intrinsic as the AVL for the instruction.

Add test cases that use the same AVL.

jacquesguan added inline comments.Jul 29 2021, 7:10 PM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
689

Done, thanks.

This revision is now accepted and ready to land.Aug 2 2021, 5:12 PM