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

[koakuma]

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.

[jrtc27]

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

667 ↗

(On Diff #500988)

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

674 ↗

(On Diff #500988)

isUInt<32>(ImmValue)?

680 ↗

(On Diff #500988)

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

1149 ↗

(On Diff #500988)

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]

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

667 ↗

(On Diff #500988)

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]

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

[koakuma]

Ping?

[arsenm]

llvm/lib/Target/Sparc/SparcInstrInfo.td

1781–1785

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]

llvm/lib/Target/Sparc/SparcInstrInfo.td

1781–1785

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?

[koakuma]

Ping?

[barannikov88]

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

444 ↗

(On Diff #510183)

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.

1105 ↗

(On Diff #510183)

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

1113 ↗

(On Diff #510183)

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

1132 ↗

(On Diff #510183)

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.

1218 ↗

(On Diff #510183)

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

1233 ↗

(On Diff #510183)

Missing return?

1237 ↗

(On Diff #510183)

Why is LParen here?

1603 ↗

(On Diff #510183)

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

llvm/lib/Target/Sparc/SparcASITags.td

16–21 ↗

(On Diff #510183)

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

llvm/lib/Target/Sparc/SparcInstrAliases.td

513 ↗

(On Diff #510183)

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

[koakuma]

In D144936#4325027, @barannikov88 wrote:

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

444 ↗

(On Diff #510183)

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

1105 ↗

(On Diff #510183)

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

1233 ↗

(On Diff #510183)

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

1237 ↗

(On Diff #510183)

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?

1603 ↗

(On Diff #510183)

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]

Implement suggested changes.

[MaskRay]

llvm/test/MC/Sparc/sparcv9-atomic-instructions.s

18 ↗

(On Diff #530506)

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]

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

444 ↗

(On Diff #510183)

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

1105 ↗

(On Diff #510183)

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

1233 ↗

(On Diff #510183)

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?

1237 ↗

(On Diff #510183)

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]

• 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 ↗

(On Diff #530506)

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.

[barannikov88]

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

[brad]

@arsenm Ping.

[arsenm]

Code seems mostly fine but I know nothing about sparc

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

708–712 ↗

(On Diff #536530)

Initialize with ternary operator?

1242 ↗

(On Diff #536530)

needing allocation for MC operands seems unusual?

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

246 ↗

(On Diff #536530)

Single quotes

[koakuma]

Apply code change suggestions.

[brad]

@arsenm @jrtc27 Ping.

[brad]

In D144936#4525415, @brad wrote:

@arsenm @jrtc27 Ping.

@arsenm @jrtc27 Ping.

[barannikov88]

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

[arsenm]

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]

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

[barannikov88]

LGTM

[barannikov88]

Please add a disassembler test.

[koakuma]

Add disassembly tests.

[glaubitz]

@barannikov88 Can you land this change?

llvm/lib/Target/Sparc/SparcInstrInfo.td

 defm TSUBCC   : F3_12np<"tsubcc",   0b100001>;
 
 let hasSideEffects = 1 in {
   defm TADDCCTV : F3_12np<"taddcctv", 0b100010>;
   defm TSUBCCTV : F3_12np<"tsubcctv", 0b100011>;
 }
 }
 
+// Section A.11 - DONE and RETRY
+// Section A.47 - SAVED and RESTORED
+let Predicates = [HasV9], rs1 = 0, rs2 = 0 in {
+  let rd = 0 in
+    def DONE : F3_1<2, 0b111110, (outs), (ins), "done", []>;
+
+  let rd = 1 in
+    def RETRY : F3_1<2, 0b111110, (outs), (ins), "retry", []>;
+
+  let rd = 0 in
+    def SAVED : F3_1<2, 0b110001, (outs), (ins), "saved", []>;
+
+  let rd = 1 in
+    def RESTORED : F3_1<2, 0b110001, (outs), (ins), "restored", []>;
+}
+
 // Section A.42 - Prefetch Data
 let Predicates = [HasV9] in {
   def PREFETCHr : F3_1<3, 0b101101,
                    (outs), (ins (MEMrr $rs1, $rs2):$addr, shift_imm5:$rd),
                    "prefetch [$addr], $rd", []>;
   def PREFETCHi : F3_2<3, 0b101101,
                    (outs), (ins (MEMri $rs1, $simm13):$addr, shift_imm5:$rd),
                    "prefetch [$addr], $rd", []>;
 }
 
 
 
 // Section A.43 - Read Privileged Register Instructions
 let Predicates = [HasV9] in {
 let rs2 = 0 in
   def RDPR : F3_1<2, 0b101010,
                  (outs IntRegs:$rd), (ins PRRegs:$rs1),
                  "rdpr $rs1, $rd", []>;
 }
 
 // Section A.62 - Write Privileged Register Instructions
 let Predicates = [HasV9] in {
   def WRPRrr : F3_1<2, 0b110010,
                    (outs PRRegs:$rd), (ins IntRegs:$rs1, IntRegs:$rs2),
                    "wrpr $rs1, $rs2, $rd", []>;
   def WRPRri : F3_2<2, 0b110010,
                    (outs PRRegs:$rd), (ins IntRegs:$rs1, simm13Op:$simm13),
                    "wrpr $rs1, $simm13, $rd", []>;
 }
 
 //===----------------------------------------------------------------------===//
 // Non-Instruction Patterns

llvm/test/MC/Sparc/sparcv9-instructions.s

! RUN: llvm-mc %s -arch=sparcv9 -show-encoding | FileCheck %s --check-prefix=V9
         ! V8-NEXT: prefetch  [ %i1 + 0xf80 ], 1
         ! V9: prefetch  [%i1+3968], 1  ! encoding: [0xc3,0x6e,0x6f,0x80]
         prefetch  [ %i1 + 0xf80 ], 1
 
         ! V8:      error: instruction requires a CPU feature not currently enabled
         ! V8-NEXT: prefetch  [ %i1 + %i2 ], 1
         ! V9: prefetch  [%i1+%i2], 1  ! encoding: [0xc3,0x6e,0x40,0x1a]
         prefetch  [ %i1 + %i2 ], 1
+
+        ! V8:      error: instruction requires a CPU feature not currently enabled
+        ! V8-NEXT: done
+        ! V9: done      ! encoding: [0x81,0xf0,0x00,0x00]
+        done
+        ! V8:      error: instruction requires a CPU feature not currently enabled
+        ! V8-NEXT: retry
+        ! V9: retry     ! encoding: [0x83,0xf0,0x00,0x00]
+        retry
+        ! V8:      error: instruction requires a CPU feature not currently enabled
+        ! V8-NEXT: saved
+        ! V9: saved     ! encoding: [0x81,0x88,0x00,0x00]
+        saved
+        ! V8:      error: instruction requires a CPU feature not currently enabled
+        ! V8-NEXT: restored
+        ! V9: restored  ! encoding: [0x83,0x88,0x00,0x00]
+        restored