Page MenuHomePhabricator

[RISCV 7/10] Add RISCVInstPrinter and basic MC assembler tests
ClosedPublic

Authored by asb on Aug 16 2016, 8:35 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

asb updated this revision to Diff 68190.Aug 16 2016, 8:35 AM
asb retitled this revision from to [RISCV 7/10] Add RISCVInstPrinter and basic MC assembler tests.
asb updated this object.
asb added reviewers: theraven, jyknight.
asb added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Aug 16 2016, 2:43 PM
reames added a subscriber: reames.Aug 21 2016, 12:31 PM

Getting 7 patches in before our first real test seems less than ideal. That's a lot of code to exercise and not many tests doing it. One possibility would be to move this patch a bit earlier in the sequence, and write some c++ code which prints manually constructed instructions. Not sure that's actually worth doing, but I'm intentionally being pedantic as requested. :)

(p.s. I don't work on the backend, so I'm learning as I go through the patches. If something doesn't make sense, that's the probable reason.)

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
10 ↗(On Diff #68190)

I believe you mentioned in another thread that the format is not well specified. We should call that out here. What is the goal for this implementation? Compatibility with existing implementation? Implementing some newly defined spec?

23 ↗(On Diff #68190)

Should this be riscv-asm-printer? Not sure what convention we use in various backends.

44 ↗(On Diff #68190)

reuse printRegName?

asb marked 2 inline comments as done.Aug 26 2016, 5:11 AM

Getting 7 patches in before our first real test seems less than ideal. That's a lot of code to exercise and not many tests doing it. One possibility would be to move this patch a bit earlier in the sequence, and write some c++ code which prints manually constructed instructions. Not sure that's actually worth doing, but I'm intentionally being pedantic as requested. :)

I'd say that backend work doesn't start until patch 3, which adds the dummy backend. At that stage there's nothing to test anyway, so really it's patch 4+5+6 that add code that can't yet be tested, then we add the test here. Other than merging this patch into the previous one, I'm not sure what I can do to get the tests started earlier. I'd appreciate suggestions though.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
10 ↗(On Diff #68190)

Basic assembly printing is fine. I think it's more that the accepted pseudo-instructions are under-documented, relocations aren't documented etc. These don't pose an issue for basic printing.

23 ↗(On Diff #68190)

I've verified that every other in-tree backend uses DEBUG_TYPE "asm-printer". Was worth checking though, thanks

asb updated this revision to Diff 69353.Aug 26 2016, 5:25 AM
asb marked an inline comment as done.

Refresh patch and reuse getRegName as suggested by @reames

theraven accepted this revision.Aug 26 2016, 6:47 AM
theraven edited edge metadata.

LGTM.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
24 ↗(On Diff #69353)

Not much danger of ambiguity - I doubt many people are debugging LLVM with test cases that emit assembly for two different architectures in the same test run...

This revision is now accepted and ready to land.Aug 26 2016, 6:47 AM

Shouldn't we be testing the code we're adding here? At the moment we seem to be adding support for InstPrinter but only testing MCTargetDesc (the encodings) & AsmPrinter code.

jyknight added inline comments.Oct 3 2016, 9:04 AM
lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
10 ↗(On Diff #68190)

I'd just note that the spec does not document any asm syntax, leaving it up to implementors' imagination. Still, yes, for basic printing, there's not much ambiguity.

test/MC/RISCV/rv32i-valid.s
4 ↗(On Diff #69353)

Each of these lines should be like:

# CHECK: addi ra, sp, 2 # encoding: [0x93,0x00,0x21,0x00]
addi ra, sp, 2

So that it is actually checking the output of the the InstPrinter code in addition to the encoding.

asb added inline comments.Oct 3 2016, 9:42 AM
test/MC/RISCV/rv32i-valid.s
4 ↗(On Diff #69353)

I'm about to refresh my patchset, I decided to add a CHECK-INST to separate the checking of assembly printing and encoding. It isn't necessary for this patch, but it's desirable when adding %lo etc:

lui s11, %hi(0x87000000) # CHECK: encoding: [0xb7,0x0d,0x00,0x87]
# CHECK-INST: lui s11, 552960
asb updated this revision to Diff 74025.Oct 8 2016, 6:12 AM
asb edited edge metadata.
asb marked 2 inline comments as done.

Tests have been modified to check instruction printing.

jyknight added inline comments.Oct 11 2016, 10:14 AM
test/MC/RISCV/rv32i-valid.s
7 ↗(On Diff #74025)

If you flip this order around, putting the "CHECK-INST" first, then the "CHECK: encoding", you wouldn't need to run llvm-mc twice for each processor mode, once with show-encoding and once without; just give FileCheck both prefixes in one run.

I'd also suggest moving the encoding check up to its own line, I think it'll be more readable that way. e.g.:

# RUN: llvm-mc %s -triple=riscv32 -show-encoding | FileCheck -check-prefixes=CHECK,CHECK-INST %s
# RUN: llvm-mc %s -triple=riscv64 -show-encoding | FileCheck -check-prefixes=CHECK,CHECK-INST %s

# CHECK-INST: addi ra, sp, 2
# CHECK: encoding: [0x93,0x00,0x21,0x00]
addi ra, sp, 2
asb updated this revision to Diff 74378.Oct 12 2016, 7:54 AM
asb marked an inline comment as done.

Update test style as suggested by @jyknight

test/MC/RISCV/rv32i-valid.s
7 ↗(On Diff #74025)

That's definitely nicer - thanks! I've switched to using this style.

jyknight accepted this revision.Oct 13 2016, 12:10 PM
jyknight edited edge metadata.
Razer6 added a subscriber: Razer6.Feb 14 2017, 6:29 AM
This revision was automatically updated to reflect the committed changes.