This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][NFC] Mark rs1 in most memory instructions as memory operand.
ClosedPublic

Authored by dybv-sc on Oct 27 2022, 7:25 AM.

Details

Summary

Marking rs1 (memory offset base) as memory operand provides additional
semantic value to this operand that can be used by different tools
(e.g. llvm-exegesis).

This change does not affect neigther Isel nor assembler. However it
required some tweaks in tablegen compressed inst emmiter.

Diff Detail

Event Timeline

dybv-sc created this revision.Oct 27 2022, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 7:25 AM
dybv-sc requested review of this revision.Oct 27 2022, 7:25 AM
jrtc27 added inline comments.Oct 27 2022, 9:19 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
131–135

Or change the RegisterOperand<GPR> to MemOperand<GPR>

140–146

You're missing a bunch of whitespace before tokens

144

I don't think we want an Operand suffix on these, we don't for other things

506

So what about all the immediate offsets? OPERAND_MEMORY surely isn't very useful without knowing the offset? It just tells you the instruction is touching memory somewhere in the +/- 2 KiB region centred around that register.

dybv-sc added inline comments.Oct 27 2022, 9:57 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
506

Yes, it is an issue. But there is really not much can be done for immediate. It is not really possible to change it's operand type to OPERAND_MEMORY because the operand type defines their size and signness (and reg operands have their properties defined in RegClass not in type, so it's safe to change their type).
However I imagine it to be enough for tools, because I want them to know following: instruction is explicit load/store (mayStore or mayLoad == true is not enough) and their address is based on specific operand marked memory operand(and maybe some other input operands). For example, it may help in llvm-exegesis to produce serial execution for load/store instructions(we can ignore immediate offset here if we assume it to be fixed specific value).

dybv-sc updated this revision to Diff 471460.Oct 28 2022, 2:46 AM
  • Removed Operand suffixes
  • Added whitespaces between definitons

I want to ping on this again

This revision is now accepted and ready to land.Nov 15 2022, 2:50 PM
jrtc27 added inline comments.Nov 15 2022, 3:12 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
131

: needs a space before it (repeated multiple times below)

dybv-sc updated this revision to Diff 475759.Nov 16 2022, 3:43 AM

Fixed formatting issues.

All fixed now. Ping for review.

reames added a subscriber: reames.Nov 21 2022, 11:56 AM

All fixed now. Ping for review.

Why? Craig has given an LGTM and later comments were style only. Our usual take is that LGTM still holds and you can land.

(I have not looked at this patch at all, just making a process comment.)

Thanks! I didn't know that.
I will rebase and ask for merge.

dybv-sc updated this revision to Diff 477121.Nov 22 2022, 2:25 AM

Rebased and resolved merge conflicts.