This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Remove SEW operand for load/store and SEW-aware pseudos
Needs ReviewPublic

Authored by wangpc on Sep 1 2023, 10:32 AM.

Details

Summary

We can remove the SEW operand and encode the SEW value in TSFlags.

There are some exceptions that we can't remove the SEW operand:

  1. Mask load/store. Their SEW are always 1.
  2. Indexed load/store. Data SEW and index SEW can be different.

The MatcherTable in RISCVGenDAGISel.inc is reduced by 22808 bytes
and RISCVGenInstrInfo.inc has some reductions too. Besides, about
0.53% of compile-time can be reduced (not a stable result).

But it will also add some complexities and make the RVV pseudos
inconsistent.

Diff Detail

Event Timeline

wangpc created this revision.Sep 1 2023, 10:32 AM
wangpc requested review of this revision.Sep 1 2023, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2023, 10:32 AM
wangpc updated this revision to Diff 555862.Sep 5 2023, 7:20 AM

Resolved all issues.

wangpc retitled this revision from [WIP][RISCV] Remove SEW operand for SEW-aware pseudos to [RISCV] Remove SEW operand for load/store and SEW-aware pseudos.Sep 5 2023, 7:22 AM
wangpc edited the summary of this revision. (Show Details)
wangpc added reviewers: kito-cheng, jrtc27.

I wonder if it would be a good idea to add a function getSEW(MachineInstr) which gets the SEW from the Opcode or from the Operand, depending on how the pseudo tracks SEW. This indirection would remove the complication of having inconsistent pseudos. WDYT?

kito-cheng added inline comments.Sep 5 2023, 8:19 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
50–51

This seems could be a separated NFC refactor patch?

wangpc updated this revision to Diff 555978.Sep 6 2023, 12:57 AM

Rebase and do some refactors.

wangpc edited the summary of this revision. (Show Details)Sep 6 2023, 12:57 AM
wangpc marked an inline comment as done.Sep 6 2023, 1:00 AM

I wonder if it would be a good idea to add a function getSEW(MachineInstr) which gets the SEW from the Opcode or from the Operand, depending on how the pseudo tracks SEW. This indirection would remove the complication of having inconsistent pseudos. WDYT?

Done, I added a getSEWOp.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
50–51

No, we have changed how it is used. We should use getSEWOp now.

wangpc updated this revision to Diff 555981.Sep 6 2023, 1:19 AM
wangpc marked an inline comment as done.
wangpc edited the summary of this revision. (Show Details)

Rename sew_div_8 to sewDividedBy8.

michaelmaitland added inline comments.Sep 6 2023, 9:12 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
233

I think this patch creates a scenario where !hasSEW(TSFlags) && pseudoNameContainsSEW(Opcode). This patch removes SEW operand from loads and stores that have SEW in the pseudo name. These instructions still have SEW information in them that we are not extracting in this function, since this creates the case !hasSEW(TSFlags) for these loads and stores.

Obviously we cannot return MachineOperand when SEW is a part of the Opcode name and not an operand. However, in the places that getSEWOp wants the MachineOperand just for the immediate value, then we should be able to say what that immediate is with getLog2SEW that either gets it from TSFlags or from Opcode (if it exists as a part of either).

I think the issue of missing opportunities to report SEW, in the case that it is only part of the opcode and not an operand also applies to getLog2SEW.

Ping.
Do we agree to do this?

Ping.
Do we agree to do this?

I think so. My last comment expresses that there may be some lost opportunity since less instructions hasSEWOp now. The diff in this patch shows that we bail out of some optimizations when !hasSEWOp. I am concerned that the SEW operand we remove from the instructions in this patch may lead to bailing out early, when we still have SEW information. I think we could be doing hasSEWOp() || hasSEWInPseudoName() to guard these optimizations and add something like getSEWFromPseudoName(). WDYT?

wangpc added a comment.EditedSep 27 2023, 12:52 AM

Ping.
Do we agree to do this?

I think so. My last comment expresses that there may be some lost opportunity since less instructions hasSEWOp now. The diff in this patch shows that we bail out of some optimizations when !hasSEWOp. I am concerned that the SEW operand we remove from the instructions in this patch may lead to bailing out early, when we still have SEW information. I think we could be doing hasSEWOp() || hasSEWInPseudoName() to guard these optimizations and add something like getSEWFromPseudoName(). WDYT?

IIUC, I think I have done the hasSEWOp() || hasSEWInPseudoName() thing in newly-added RISCVII::hasSEW?