-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[feature][riscv] handle target address calculation in llvm-objdump disassembly for riscv #144620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
arjunUpatel
wants to merge
12
commits into
llvm:main
Choose a base branch
from
arjunUpatel:riscv-address-resolution
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+253
−12
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
52ebb65
Remove changes affecting non-RISCV targets
arjunUpatel 49f276e
Update test output to match previous functionality
arjunUpatel 5e1cf12
Add support for zclsd and zilsd extensions + tests
arjunUpatel 11c50dc
Pass subtargetinfo as function argument
arjunUpatel 2241583
Run clang format
arjunUpatel 7635b70
Remove precanned binaries from tests and invoke clang during tests
arjunUpatel 9dff558
Fix typo
arjunUpatel b738bf1
Enable address resolution for load/store instructions relative to zer…
arjunUpatel 9361828
Edit comments to follow LLVM coding style
arjunUpatel e942f63
Use llvm-mc over clang for compilation in tests
arjunUpatel 86d6bd5
Update comments
arjunUpatel 1e1a37c
Differentiate comments in tests from llvm-lit directives
arjunUpatel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
if "RISCV" not in config.targets_to_build: | ||
config.unsupported = True |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
# RUN: llvm-mc -riscv-add-build-attributes -triple=riscv32 -filetype=obj -mattr=+zclsd,+zilsd %s -o %t | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "ar-coverage" mean in these test file names? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
# RUN: llvm-objdump -d %t | FileCheck %s | ||
|
||
# CHECK: 00000000 <_start>: | ||
# CHECK-NEXT 0: 00000517 auipc a0, 0x0 | ||
# CHECK-NEXT 4: 0559 addi a0, a0, 0x16 <target> | ||
# CHECK-NEXT 6: 00000517 auipc a0, 0x0 | ||
# CHECK-NEXT a: 6910 ld a2, 0x10(a0) <target> | ||
# CHECK-NEXT c: 00000517 auipc a0, 0x0 | ||
# CHECK-NEXT 10: 00c53523 sd a2, 0xa(a0) <target> | ||
# CHECK-NEXT 14: 0000 unimp | ||
|
||
## The structure of this test file is similar to that of riscv64-ar-coverage | ||
## with the major difference being that these tests are focused on instructions | ||
## for 32 bit architecture. | ||
|
||
.global _start | ||
.text | ||
_start: | ||
auipc a0, 0x0 | ||
addi a0, a0, 0x16 ## addi -- behavior changes with different architectures. | ||
|
||
## Test Zclsd and Zilsd instructions respectively | ||
auipc a0, 0x0 | ||
c.ld a2, 0x10(a0) | ||
|
||
auipc a0, 0x0 | ||
sd a2, 0xa(a0) | ||
|
||
.skip 0x2 | ||
target: | ||
ret: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
# RUN: llvm-mc -riscv-add-build-attributes -triple=riscv64 -filetype=obj -mattr=+f,+c,+zcb %s -o %t | ||
# RUN: llvm-objdump -d %t | FileCheck %s | ||
|
||
# CHECK: 0000000000000000 <_start>: | ||
# CHECK-NEXT: 0: 00010517 auipc a0, 0x10 | ||
# CHECK-NEXT: 4: 00450513 addi a0, a0, 0x4 <target> | ||
# CHECK-NEXT: 8: 00010517 auipc a0, 0x10 | ||
# CHECK-NEXT: c: 1571 addi a0, a0, -0x4 <target> | ||
# CHECK-NEXT: e: 6541 lui a0, 0x10 | ||
# CHECK-NEXT: 10: 0045059b addiw a1, a0, 0x4 <target> | ||
# CHECK-NEXT: 14: 6541 lui a0, 0x10 | ||
# CHECK-NEXT: 16: 2511 addiw a0, a0, 0x4 <target> | ||
# CHECK-NEXT: 18: 00110537 lui a0, 0x110 | ||
# CHECK-NEXT: 1c: c50c sw a1, 0x8(a0) <far_target> | ||
# CHECK-NEXT: 1e: 00110537 lui a0, 0x110 | ||
# CHECK-NEXT: 22: 4508 lw a0, 0x8(a0) <far_target> | ||
# CHECK-NEXT: 24: 6541 lui a0, 0x10 | ||
# CHECK-NEXT: 26: 6585 lui a1, 0x1 | ||
# CHECK-NEXT: 28: 0306 slli t1, t1, 0x1 | ||
# CHECK-NEXT: 2a: 0511 addi a0, a0, 0x4 <target> | ||
# CHECK-NEXT: 2c: 0505 addi a0, a0, 0x1 | ||
# CHECK-NEXT: 2e: 00002427 fsw ft0, 0x8(zero) <_start+0x8> | ||
# CHECK-NEXT: 32: 00110097 auipc ra, 0x110 | ||
# CHECK-NEXT: 36: fda080e7 jalr -0x26(ra) <func> | ||
# CHECK-NEXT: 3a: 6445 lui s0, 0x11 | ||
# CHECK-NEXT: 3c: 8800 sb s0, 0x0(s0) <zcb> | ||
# CHECK-NEXT: 3e: 4522 lw a0, 0x8(sp) | ||
|
||
## The core of the feature being added was address resolution for instruction | ||
## sequences where a register is populated by immediate values via two | ||
## separate instructions. First by an instruction that provides the upper bits | ||
## (auipc, lui, etc) followed by another instruction for the lower bits (addi, | ||
## jalr, ld, etc.). | ||
|
||
.global _start | ||
.text | ||
|
||
_start: | ||
## Test block 1-3 each focus on a certain starting instruction in a sequence. | ||
## Starting instructions are the ones that provide the upper bits. The other | ||
## instruction in the sequence is the one that provides the lower bits. The | ||
## second instruction is arbitrarily chosen to increase code coverage. | ||
|
||
## Test block #1. | ||
lla a0, target | ||
auipc a0, 0x10 | ||
c.addi a0, -0x4 | ||
|
||
## Test block #2. | ||
c.lui a0, 0x10 | ||
addiw a1, a0, 0x4 | ||
c.lui a0, 0x10 | ||
c.addiw a0, 0x4 | ||
|
||
## Test block #3. | ||
lui a0, 0x110 | ||
sw a1, 0x8(a0) | ||
lui a0, 0x110 | ||
c.lw a0, 0x8(a0) | ||
|
||
## Test block 4 tests instruction interleaving. Essentially the code's | ||
## ability to keep track of a valid sequence even if multiple other unrelated | ||
## instructions separate the two. | ||
lui a0, 0x10 | ||
lui a1, 0x1 ## Unrelated instruction. | ||
slli t1, t1, 0x1 ## Unrelated instruction. | ||
addi a0, a0, 0x4 | ||
addi a0, a0, 0x1 ## Verify register tracking terminates. | ||
|
||
## Test 5 checks instructions providing upper bits do not change the tracked | ||
## value of zero register. Also ensures load/store instructions accessing data | ||
## relative to the zero register trigger address resolution. The latter kind | ||
## of instructions are essentially memory accesses relative to the zero | ||
## register. | ||
fsw f0, 0x8(x0) | ||
|
||
## Test 6 ensures that the newly added functionality is compatible with | ||
## code that already worked for branch instructions. | ||
call func | ||
|
||
## Test #7 -- zcb extension. | ||
lui x8, 0x11 | ||
c.sb x8, 0(x8) | ||
|
||
## Test #8 -- stack based load/stores. | ||
c.lwsp a0, 0x8(sp) | ||
|
||
## These are the labels that the instructions above are expected to resolve to. | ||
.skip 0xffc4 | ||
target: | ||
.word 1 | ||
.skip 0xff8 | ||
zcb: | ||
.word 1 | ||
.skip 0xff004 | ||
far_target: | ||
.word 2 | ||
func: | ||
ret |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a distinct function to
evaluateBranch
in the virtual interface? It feels like the two are fundamentally the same from a high-level point of view, since a branch is an instruction. Of course, within the target-specific layer, you could split it then.If the purpose is purely so you know when a thing is branch specifically, it feels like you could achieve that through some other means, for example an enum return code that has values like
Invalid
,Branch
,Other
, or maybe a bitfield with some basic flags.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it makes more sense to merge the two since their functionality overlaps heavily. The purpose of splitting the two is that RISCV
addi
instruction behaves differently when the subtargetinfo is different (32 bit vs 64 bit). Hence, the subtargetinfo is passed as a parameter toevaluateInstruction
, whileevaluateBranch
does not accept this parameter. We can either set the subtargetinfo as a default parameter in the declaration ofevaluateBranch
or make the subtargetinfo an attribute ofMCInstrAnalysis
. I like the latter because off ofa quick skim it looks like across all the invocations of the functions that are attributes ofMCInstrAnalysis
and have subtargetinfo as a parameter, none actively change the subtargetinfo once the parent object is initialized. How about I experiment with these two approaches in a separate PR?