This is an archive of the discontinued LLVM Phabricator instance.

[Xtensa 7/10] Add Xtensa instruction printer.
ClosedPublic

Authored by andreisfr on Jul 16 2019, 3:27 PM.

Details

Summary

Add printer for current instructions and operands subsets.
Also add basic tests of the Xtensa instructions.

Diff Detail

Event Timeline

andreisfr created this revision.Jul 16 2019, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 3:27 PM
andreisfr updated this revision to Diff 212691.Jul 31 2019, 4:00 PM

Register names are capitalized.

andreisfr updated this revision to Diff 242217.Feb 3 2020, 3:18 PM

Patch is updated according to latest upstream version. Updated licenses.

andreisfr updated this revision to Diff 328699.Mar 5 2021, 4:46 PM

Patch is updated according to latest upstream version

andreisfr updated this revision to Diff 329499.Mar 9 2021, 4:51 PM

Patch is updated according to LLVM upstream version and latest Xtensa backend version.

andreisfr updated this revision to Diff 335956.Apr 7 2021, 4:55 PM

The llvm_unreachable is substituted to report_fatal_error

andreisfr updated this revision to Diff 351603.Jun 11 2021, 5:40 PM

Remove redundant whitespaces.

Patch is updated according to LLVM upstream version.

bero added a subscriber: bero.Apr 14 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 2:00 PM
andreisfr updated this revision to Diff 425076.Apr 25 2022, 5:17 PM

Patch is updated according to LLVM upstream version

saugustine added inline comments.
llvm/test/MC/Xtensa/xtensa-invalid.s
13 ↗(On Diff #425076)

It would be good to check instruction arguments out of order, and invalid register names.

llvm/test/MC/Xtensa/xtensa-valid.s
4 ↗(On Diff #425076)

It would be good here to comments marking which instruction formats (RRR, RRI4, RRI8 and so on). re covered, as the difference between them is a common source of bugs. Perhaps next to one of the instructions.

I assume that eventually there will be comprehensive coverage of all the different formats.

61 ↗(On Diff #425076)

Spaces after the commas in this file is inconsistent.

andreisfr updated this revision to Diff 455444.Aug 24 2022, 6:00 PM

Minor changes: corrected comment

andreisfr added inline comments.Aug 24 2022, 6:19 PM
llvm/test/MC/Xtensa/xtensa-invalid.s
13 ↗(On Diff #425076)

You mean for each instruction implement extended tests with argument out of order and invalid register names?

saugustine added inline comments.Aug 30 2022, 1:47 PM
llvm/test/MC/Xtensa/xtensa-invalid.s
13 ↗(On Diff #425076)

Probably just once for each format, but yes.

phosek added a subscriber: phosek.Sep 1 2022, 11:44 AM
tpambor added a subscriber: tpambor.Sep 4 2022, 3:08 AM
arsenm added a subscriber: arsenm.Sep 12 2022, 6:42 AM
arsenm added inline comments.
llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp
34–35

Just use cast<>

MaskRay added inline comments.
llvm/test/MC/Xtensa/elf-header.s
5 ↗(On Diff #455444)

Add -NEXT when applicable.

llvm/test/MC/Xtensa/xtensa-valid.s
1 ↗(On Diff #455444)

Split this into multiple files.

MC/AArch64/armv8* and MC/LoongArch/Basic can give some insights how tests are organized.

MaskRay added inline comments.Sep 12 2022, 10:24 PM
llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp
90

isInt<8> from MathExtras.h

94

Add braces. then uses braces and else should use, too.

101

isInt<16> from MathExtras.h

andreisfr updated this revision to Diff 461114.Sep 18 2022, 6:20 PM

Minor code corrections. Test file xtensa-valid.s is splitted to several files by instructions groups.

andreisfr updated this revision to Diff 461118.Sep 18 2022, 6:35 PM

Remove xtensa-valid.s

andreisfr added inline comments.Sep 18 2022, 6:37 PM
llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp
34–35

Corrected

90

Corrected

94

Corrected

101

Corrected

llvm/test/MC/Xtensa/elf-header.s
5 ↗(On Diff #455444)

Corrected

llvm/test/MC/Xtensa/xtensa-invalid.s
13 ↗(On Diff #425076)

I added more tests for different instruction formats.

llvm/test/MC/Xtensa/xtensa-valid.s
1 ↗(On Diff #455444)

Thank you for advice, I've splitted file to several files by commands groups.

4 ↗(On Diff #425076)

Added format information to each instruction.

@MaskRay , may I ask you about new test structure of the instructions set? I divided test file xtensa-valid.s to several files by instructions groups. Whether such division matches to your suggestions?

JKRhb added a subscriber: JKRhb.Oct 9 2022, 2:30 PM
arsenm accepted this revision.Dec 13 2022, 8:59 AM

LGTM with nit

llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp
35

Also means you can get rid of the assert(false) and if

This revision is now accepted and ready to land.Dec 13 2022, 8:59 AM
saugustine accepted this revision.Dec 13 2022, 10:40 AM
jrtc27 added a subscriber: jrtc27.Dec 13 2022, 12:06 PM
jrtc27 added inline comments.
llvm/test/MC/Xtensa/elf-header.s
2 ↗(On Diff #461118)

Why not just use CHECK? And please keep these uppercase so they stand out.

andreisfr updated this revision to Diff 484120.Dec 19 2022, 3:41 PM

Fixed "elf-header.s" test according to comments

andreisfr marked an inline comment as done.Dec 19 2022, 3:42 PM
andreisfr added inline comments.
llvm/test/MC/Xtensa/elf-header.s
2 ↗(On Diff #461118)

Thank you for comment, test is fixed.

MaskRay accepted this revision.Dec 19 2022, 3:57 PM
MaskRay added inline comments.
llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.h
53
llvm/test/MC/Xtensa/elf-header.s
5 ↗(On Diff #455444)

Not done.

llvm/test/MC/Xtensa/xtensa-invalid.s
1 ↗(On Diff #484120)

llvm-mc doesn't need <

delete excess spaces

9 ↗(On Diff #484120)

Add locations for all diagnostics ( [[#@LINE+1]])

19 ↗(On Diff #484120)

[[#LINE]] is legacy syntax. Use [[#@LINE]]. Fix this everywhere.

barannikov88 accepted this revision.Dec 20 2022, 2:31 AM
barannikov88 added a subscriber: barannikov88.
barannikov88 added inline comments.
llvm/lib/Target/Xtensa/Xtensa.td
56

I think this bit does not affect anything.

MaskRay added a comment.EditedDec 20 2022, 1:14 PM

The xtensa- prefix in test filenames like llvm/test/MC/Xtensa/xtensa-arith.s is unnecessary.
You may look at M68k and LoongArch for how ISA tests are organized in subdirectories. I think the two newer ports have more organized hierarchy.

andreisfr updated this revision to Diff 485252.Dec 25 2022, 4:32 PM
andreisfr marked an inline comment as done.

Added fixes according to comments

andreisfr added inline comments.Dec 25 2022, 4:33 PM
llvm/lib/Target/Xtensa/Xtensa.td
56

Fixed

@MaskRay , I changed Xtensa tests structure according to your suggestions, PTAL.

This revision was landed with ongoing or failed builds.Dec 26 2022, 4:39 AM
This revision was automatically updated to reflect the committed changes.