Page MenuHomePhabricator
Differential D136847

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

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

Details

Reviewers
craig.topper
Commits
rG95ef005230e9: [RISCV][NFC] Mark rs1 in most memory instructions as memory operand.
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

Unit TestsFailed
View All

TimeTest
60,040 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::signal_sync.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/signal_sync.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/signal_sync.cpp.tmp && /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/signal_sync.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/signal_sync.cpp

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
Herald added subscribers: sunshaoce, VincentWu, vkmr and 29 others. · View Herald Transcript
dybv-sc requested review of this revision.Oct 27 2022, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 7:25 AM
Herald added subscribers: llvm-commits, pcwang-thead, eopXD and 2 others. · View Herald Transcript
dybv-sc added a reviewer: craig.topper.Oct 27 2022, 8:27 AM
Herald added a subscriber: StephenFan. · View Herald TranscriptOct 27 2022, 8:27 AM
Harbormaster completed remote builds in B194655: Diff 471162.Oct 27 2022, 9:11 AM
jrtc27 added inline comments.Oct 27 2022, 9:19 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
131

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

137–143

You're missing a bunch of whitespace before tokens

141

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

505

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
505

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
Harbormaster completed remote builds in B194874: Diff 471460.Oct 28 2022, 4:33 AM
dybv-sc added a comment.Nov 8 2022, 2:23 AM

ping

dybv-sc added a comment.Nov 15 2022, 1:58 AM

I want to ping on this again

craig.topper accepted this revision.Nov 15 2022, 2:50 PM

LGTM

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–132

: 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.

Harbormaster completed remote builds in B197957: Diff 475759.Nov 16 2022, 4:36 AM
dybv-sc updated this revision to Diff 475781.Nov 16 2022, 4:54 AM

rebased

Harbormaster completed remote builds in B197973: Diff 475781.Nov 16 2022, 5:46 AM
dybv-sc added a comment.Nov 16 2022, 6:27 AM

All fixed now. Ping for review.

reames added a subscriber: reames.Nov 21 2022, 11:56 AM
In D136847#3930781, @dybv-sc wrote:

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.)

dybv-sc added a comment.Nov 22 2022, 2:25 AM

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.

Harbormaster completed remote builds in B198938: Diff 477121.Nov 22 2022, 5:12 AM
Closed by commit rG95ef005230e9: [RISCV][NFC] Mark rs1 in most memory instructions as memory operand. (authored by dybv-sc, committed by asi-sc). · Explain WhyNov 22 2022, 5:44 AM
This revision was automatically updated to reflect the committed changes.
asi-sc added a commit: rG95ef005230e9: [RISCV][NFC] Mark rs1 in most memory instructions as memory operand..

Revision Contents
Changeset List

PathSize
llvm/
lib/
Target/
RISCV/
13 lines
104 lines
4 lines
36 lines
74 lines
utils/
TableGen/
8 lines

Diff 471162

View Options

llvm/lib/Target/RISCV/RISCVInstrInfo.td

Loading...
View Options

llvm/lib/Target/RISCV/RISCVInstrInfoC.td

Loading...
View Options

llvm/lib/Target/RISCV/RISCVInstrInfoF.td

Loading...
View Options

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

Loading...
View Options

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

Loading...
View Options

llvm/utils/TableGen/CompressInstEmitter.cpp

Loading...