This is an archive of the discontinued LLVM Phabricator instance.

[SPARC][IAS] Add support for v9 DONE, RETRY, SAVED, & RESTORED
ClosedPublic

Authored by koakuma on Feb 27 2023, 5:58 PM.

Details

Summary

Add support for DONE, RETRY, SAVED, and RESTORED (v9 §A.11 & §A.47).
Those instructions are used for low-level interrupt handling and register window management by OS kernels.

Diff Detail

Event Timeline

koakuma created this revision.Feb 27 2023, 5:58 PM
koakuma requested review of this revision.Feb 27 2023, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 5:58 PM
jrtc27 added inline comments.Mar 1 2023, 9:10 AM
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
687

Shouldn't this be impossible due to the predicate, and thus be an assert?

694

isUInt<32>(ImmValue)?

700

Use MCInstBuilder throughout? Avoids all the { ... } below, too...

1134

Use a TableGen GenericTable for this to avoid having to define the forward mapping here and the backward mapping later? (See RISCVSystemOperands.td for an example).

koakuma added inline comments.Mar 31 2023, 11:03 PM
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
687

Probably just me misunderstanding it but during testing I found out that even with 32-bit -mtriple=sparc it still recognizes SETX anyway, that's why I'm putting the condition as a normal error instead of an assert.

koakuma updated this revision to Diff 510183.Mar 31 2023, 11:03 PM

Implement review suggestions:

  • Use isUInt<32>(ImmValue) instead of manually checking ranges
  • Use MCInstBuilder to expand setx pseudoinstruction
  • Create a table to hold ASI symbolic names
arsenm added inline comments.Apr 24 2023, 10:07 AM
llvm/lib/Target/Sparc/SparcInstrInfo.td
1834–1838

I'm confused by what this comment means, as you're defining a new instruction. Is the intent to define an assembler alias only?

koakuma added inline comments.Apr 24 2023, 6:55 PM
llvm/lib/Target/Sparc/SparcInstrInfo.td
1834–1838

Yeah, my intent is to define a rdpr %fq, $rd alias, but the encoding of %fq is different between v8 and v9 versions of the ISA, so I had to do that to convince the assembler to encode it properly when emitting code for v9.
Should I do it in a different way?

It looks like there are several independent changes. A few smaller diffs would be much easier to review.

llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
454

It should have its own Kind and union member. Not sure if tablegen provides you with an enum, but if it does, the enum could be used.
This is "safer" and allows you to avoid converting to MCExpr and back.

1123

Why not just getTok().is(AsmToken::Integer) and getTok().getIntVal() on success?

1131

Is space after '#' allowed? If not, getLexer().peekToken(false) should be used.

1150

The location is not precise. It points before the last whitespace (there may be several).
Ideally, you would getTok().getEndLoc() before Lex()ing the indentifier or the number.

1234

This should return MatchOperand_ParseFail. NoMatch implies no tokens were consumed.

1249

Missing return?

1255

Why is LParen here?

1621–1684

Do you know why Sparc does not use autogenerated MatchRegisterName? (ShouldEmitMatchRegisterName = 0 in Sparc.td.)

llvm/lib/Target/Sparc/SparcASITags.td
17–22

It looks like all tags have an alternative name, just pass it as template parameter.

llvm/lib/Target/Sparc/SparcInstrAliases.td
513

It should be simm13Op, though it does not currently matter that much as it is missing AsmOperandClass, so no overflow checking is done.

koakuma marked 14 inline comments as done.Jun 12 2023, 7:41 AM

It looks like there are several independent changes. A few smaller diffs would be much easier to review.

Any advice on how to split it? This is basically just a bunch of instruction definitions, the complication mostly arises since some of the parsing needs to be handled on the C++ side.

llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
454

Not entirely sure if I can/how to do this, any example code I can refer to?

1123

Same reason with the LParen matcher, this is to handle constant expressions for ASI identifier.
We want to evaluate those expressions before proceeding further.

1249

Should be no problem since it'll fall through the if check below it, but will anyway for clarity, thanks.

1255

Because using a constant expression for ASI identifier value (for example ldxa [%o0] (0x80 + 0x08), %o1) is also legal.
Is there a better way to detect/process it?

1621–1684

Unfortunately no. I am still new to the backend so for these parts of the code I'm just following what the original code already does.
I guess I'll try it out and see if I could use autogenerated MatchRegisterName.
(But it's probably better to do it in a separate patch? I'm not sure)

koakuma updated this revision to Diff 530506.Jun 12 2023, 7:42 AM
koakuma marked 4 inline comments as done.

Implement suggested changes.

MaskRay added inline comments.Jun 12 2023, 2:53 PM
llvm/test/MC/Sparc/sparcv9-atomic-instructions.s
18

The indentation seems unnecessary. It seems that we can unindent all instructions. That's the more common style for instruction encoding in other targets' testing as well.

barannikov88 added inline comments.Jun 12 2023, 3:50 PM
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
454

Roughly:

  • Add a new enum member to KindTy, e.g. k_ASITag
  • Make isASITag return true if the kind is k_ASITag
  • Add a new member to the anonymous union in SparcOperand that will hold the ASI tag number (unsigned integer probably)
  • Add SparcOperand::CreateASITag accepting ASI tag number, use this method in parseASITag instead of CreateImm
  • Update SparcOperand::print and addASITagOperands accordingly
1123

I'd personally prefer parseExpression() instead of parseSparcAsmOperand() if the first token is not '#'. Up to you.

1249

Actually there was a problem because parseSparcAsmOperand consumes tokens :)
I think there is another issue now -- what happens if there was a '%' but parseSparcAsmOperand didn't return success?

1255

Because using a constant expression for ASI identifier value (for example ldxa [%o0] (0x80 + 0x08), %o1) is also legal.

That's weird. Can you add a couple of testcases using this syntax?

Is there a better way to detect/process it?

I think not.

koakuma updated this revision to Diff 536530.Jul 1 2023, 8:24 AM
koakuma marked 5 inline comments as done.
  • Implement formatting suggestions for new testfile
  • Add constant expression forms of tests
  • Fix return value for CAS operands parsing
  • Add separate enum kind for ASI tags
llvm/test/MC/Sparc/sparcv9-atomic-instructions.s
18

I can do that with new testfiles, but for old ones I feel like it's better to follow old formattings?
At least I feel like for those files it's better to reformat in a new patch instead of here.

Looks fine to me. It would still be better to create separate revisions for independent changes.

Code seems mostly fine but I know nothing about sparc

llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
708–712

Initialize with ternary operator?

1242

needing allocation for MC operands seems unusual?

llvm/lib/Target/Sparc/MCTargetDesc/SparcInstPrinter.cpp
246

Single quotes

koakuma updated this revision to Diff 543151.Jul 21 2023, 11:34 PM
koakuma marked 3 inline comments as done.

Apply code change suggestions.

I still think the best way forward is to split this set of changes into several. It would be much easier to review, ensure test coverage is sufficient and commit messages would be more helpful.
I can see several independent changes here (I may have missed something):

  • Adding DONE/RETRY/SAVED/RESTORED instructions
  • Adding RDFQ variant
  • Adding CAS variants
  • Adding SETX pseudo instruction
  • Adding reg+imm mode to some memory instructions
  • Adding new mnemonics to control registers
  • Supporting ASI tags
  • Other minor changes

I think the code looks fine but I know nothing about SPARC. Don't really care about splitting this up or not, though I generally prefer minimal patches

koakuma updated this revision to Diff 547577.Aug 6 2023, 7:21 AM
koakuma retitled this revision from [SPARC][IAS] Recognize more SPARCv9 instructions/pseudoinstructions to [SPARC][IAS] Add support for v9 DONE, RETRY, SAVED, & RESTORED.
koakuma edited the summary of this revision. (Show Details)

Split other changes into D157230, D157231, D157232, D157233, D157234, D157235, and D157236.

This revision is now accepted and ready to land.Aug 6 2023, 12:20 PM

Please add a disassembler test.

koakuma updated this revision to Diff 548210.Aug 8 2023, 7:21 AM

Add disassembly tests.

barannikov88 accepted this revision.Aug 8 2023, 9:35 AM

@barannikov88 Can you land this change?

This revision was automatically updated to reflect the committed changes.