This is an archive of the discontinued LLVM Phabricator instance.

[Xtensa 8/10] Add support of the Xtensa shift/load/store/move and processor control instructions.
ClosedPublic

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

Details

Summary

Add new subset of Core Instructions (not full yet). Add appropriate operands description,
modify asm parser, printer and code emitter. Modify tests to support new instructions.

Diff Detail

Event Timeline

andreisfr created this revision.Jul 16 2019, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 3:28 PM
arsenm added a subscriber: arsenm.Jul 16 2019, 3:35 PM
arsenm added inline comments.
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
132–134

There are already various forms of isInt/isUInt in MathExtras.h

andreisfr updated this revision to Diff 212693.Jul 31 2019, 4:02 PM

Register names are capitalized.

andreisfr marked an inline comment as done.Jul 31 2019, 4:49 PM
andreisfr added inline comments.
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
132–134

Did you mean do not create/use special functions isImm and inRange? Initially I used the same approach as in SystemZ and AArch64 architecture, because we need to check whether expression immediate and also get value of it.

andreisfr updated this revision to Diff 242218.Feb 3 2020, 3:19 PM

Patch is updated according to latest upstream version.

andreisfr updated this revision to Diff 328700.EditedMar 5 2021, 4:47 PM

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

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

Correct instruction descriptions, format descriptions and instruction operands according to common style for *.td files. The llvm_unreachable is substituted to report_fatal_error.

andreisfr updated this revision to Diff 351607.Jun 11 2021, 5:42 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 425077.Apr 25 2022, 5:18 PM

Patch is updated according to LLVM upstream version

Minor corrections, changed argument names in the XtensaInstPrinter::printMemOperand function.

andreisfr updated this revision to Diff 455445.Aug 24 2022, 6:06 PM

Update according to changes in MEMW, EXTW, DSYNC, ESYNC, ISYNC and RSYNC instruction descriptions

saugustine accepted this revision.Aug 29 2022, 5:00 PM
saugustine added a subscriber: saugustine.

This is mostly mechanical, and the tests look correct to me. So ready.

This revision is now accepted and ready to land.Aug 29 2022, 5:00 PM
phosek added a subscriber: phosek.Sep 1 2022, 11:44 AM
andreisfr updated this revision to Diff 461120.Sep 18 2022, 6:44 PM

Minor source code corrections. Test file xtensa-valid.s is splitted to several files.

barannikov88 added inline comments.
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
149

use cast<> here and below. dyn_cast is for the cases when the result is allowed to be nullptr.

466

Bug: std::string termporary is destroyed at ';' and RegName references freed memory.

andreisfr updated this revision to Diff 485253.Dec 25 2022, 4:35 PM

Added fixes according to comments

andreisfr added inline comments.Dec 25 2022, 4:37 PM
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
149

Fixed

466

Thank you very much, fixed this issue.

barannikov88 added inline comments.Dec 25 2022, 6:05 PM
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
466

The issue is still there, RegName references deleted memory. One of the possible way to resolve this issue is to declare RegName as std::string rather than StringRef.

BTW, what does this code do? I couldn't find any registers that contain only digits in their names (all start with a letter), so MatchRegisterName is bound to fail. Nor could I find a test that uses parenthesized numbers.

Are those some special registers not modeled in MC layer (and represented as numbers in assembly language)?
If so, you need to differentiate them from ordinary registers somehow because there is no guarantee that their numbers and / or encodings do not collide with the autogenerated ones for the registers that are being modeled.

As a side node, 'SR' variable should have a more descriptive name (or, at least, its name should be explained in a comment).

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.
andreisfr added inline comments.Dec 26 2022, 5:40 AM
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
466

Thank you for comment. The "RegName" variable is not used outside switch cases, so StringRef object should be alive at points where "RegName" actually used, is it correct? You noted correctly that we should have special registers defined by numbers and Xtensa architecture have such registers for example https://github.com/llvm/llvm-project/blob/ff25800d4ba0b577a44dc918da7a1fb3c29fdb13/llvm/lib/Target/Xtensa/XtensaRegisterInfo.td#L76 . But test for such kind of register names is absent and will be implemented in nearest patches and we will add more explanations to this function.

barannikov88 added inline comments.Dec 26 2022, 5:54 AM
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
466

RegName is alive, but the data it references is not.
StringRef does not own the data.
The data is owned by std::string, which is destroyed before the first use of RegName.

andreisfr added inline comments.Dec 26 2022, 6:22 AM
llvm/lib/Target/Xtensa/AsmParser/XtensaAsmParser.cpp
466

I will rewrite this code in next patches