This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Add load and store instructions
ClosedPublic

Authored by c-rhodes on Jul 7 2021, 11:32 AM.

Details

Summary

This patch adds support for following contiguous load and store
instructions:

  • LD1B, LD1H, LD1W, LD1D, LD1Q
  • ST1B, ST1H, ST1W, ST1D, ST1Q

A new register class and operand is added for the 32-bit vector select
register W12-W15. The differences in the following tests which have been
re-generated are caused by the introduction of this register class:

  • llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll
  • llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir
  • llvm/test/CodeGen/AArch64/stp-opt-with-renaming-reserved-regs.mir
  • llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir

D88663 attempts to resolve the issue with the store pair test
differences in the AArch64 load/store optimizer.

The GlobalISel differences are caused by changes in the enum values of
register classes, tests have been updated with the new values.

The reference can be found here:
https://developer.arm.com/documentation/ddi0602/2021-06

Diff Detail

Event Timeline

c-rhodes created this revision.Jul 7 2021, 11:32 AM
c-rhodes requested review of this revision.Jul 7 2021, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 11:32 AM
Matt added a subscriber: Matt.Jul 7 2021, 3:02 PM

Maybe you should add dependency of this patch with D105570 in the commit message .
The operand needed for Za horizontal and vertical are added there.

llvm/lib/Target/AArch64/SMEInstrFormats.td
360

nit: align

llvm/test/MC/AArch64/SME/st1w-diagnostics.s
3

Is it possible to create a diagnostic test for Match_InvalidMatrixTileVectorH or Match_InvalidMatrixTileVectorV?
They are added in D105570, but I don't see it being used here.

Hi @c-rhodes, I've not finished reviewing all the tests and instr format td changes yet, but here are some comments so far!

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3027

nit: Just for readability is worth putting braces {} around the outer if-block?

3713–3714

I think you can avoid the if block above entirely if you simply add an extra condition here, right? For example,

(ParseRes == MatchOperand_NoMatch && NoMatchIsError && !RegTok.getString().startswith_insensitive("za"))

That way we just fall through to the default and return MatchOperand_NoMatch, which is what you want I think? It feels like the new if block exists simply to stop the if block below from returning MatchOperand_ParseFail.

3990

Should we be checking for '{' first and reporting an error if it's not? It could be '(', for example.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp
527

Is it worth asserting that RegOpnd >= AArch64::W12 here?

llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir
78

I realise adding a new class might have an effect on the other test files, but I'm not sure why this has changed from FPR64 to WSeqPairsClass_with_sube32_in_MatrixIndexGPR32_12_15 here?

david-arm added inline comments.Jul 9 2021, 7:38 AM
llvm/test/MC/AArch64/SME/ld1b-diagnostics.s
5

nit: Does 'za0' refer to what the tile should be? At first I thought za0 was just typed wrong and should be za1.

c-rhodes updated this revision to Diff 358256.Jul 13 2021, 7:08 AM
c-rhodes edited the summary of this revision. (Show Details)
  • Address comments.
  • Update sed lines in tests to fix failing Windows precommit.
c-rhodes marked 4 inline comments as done.Jul 13 2021, 7:30 AM
c-rhodes added a subscriber: aemerson.
c-rhodes added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3990

Should we be checking for '{' first and reporting an error if it's not? It could be '(', for example.

Not sure what you mean here, if the current token isn't { then we wouldn't be here since the case is case AsmToken::LCurly:.

llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir
78

I realise adding a new class might have an effect on the other test files, but I'm not sure why this has changed from FPR64 to WSeqPairsClass_with_sube32_in_MatrixIndexGPR32_12_15 here?

Not entirely sure myself to be honest, not being familiar with GlobalISel. @aemerson any thoughts on the difference here?

llvm/test/MC/AArch64/SME/ld1b-diagnostics.s
5

nit: Does 'za0' refer to what the tile should be? At first I thought za0 was just typed wrong and should be za1.

Yeah za0 is what's valid, fair point though I can see how that could be confusing, I've clarified the comments by prefixing 'expected: '.

llvm/test/MC/AArch64/SME/st1w-diagnostics.s
3

Is it possible to create a diagnostic test for Match_InvalidMatrixTileVectorH or Match_InvalidMatrixTileVectorV?
They are added in D105570, but I don't see it being used here.

The diagnostics for the matrix operands need some work to be honest, at the moment none of the invalid match types are triggered. Ideally the invalid tile test below would trigger:

case Match_InvalidMatrixTileVectorH32:
  return Error(Loc, "invalid matrix operand, expected za[0-3]h.s");

but za4h.s isn't in matchMatrixRegName so tryParseMatrixRegister returns no match and za4h.s gets parsed as a token, at which point the operand parsing loop in ParseIntruction tries to parse a comma before the next operand, and since there isn't one separating the matrix operand and index, it errors, emitting: 'unexpected token in argument list'.

I suppose we could treat matchMatrixRegName as an error and return MatchOperand_ParseFail. The diagnostic wouldn't be as accurate as what's added in D105570, but it would be an improvement over the current diagnostic which is emitted for the next operand ([).

aemerson added inline comments.Jul 13 2021, 11:43 AM
llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir
78

I *think*, and please verify my theory here, that this is just because in this test we're hard coding an enum integer 1245194 for FPR64, as the operand for the INLINEASM instruction. If you add a new regclass that could be shifting the entries in the enum so 1245194 is no longer referencing FPR64. You should be able to just find the new integer value for FPR64 and change the test to use that.

c-rhodes updated this revision to Diff 358561.Jul 14 2021, 4:52 AM
c-rhodes edited the summary of this revision. (Show Details)
  • Update enum value of FPR64 regclass in llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir, since the old value is now referencing WSeqPairsClass_with_sube32_in_MatrixIndexGPR32_12_15.
  • Tweaked isMatrixRegOperand to return NearMatch to improve diagnostics.
  • Added diagnostics tests for load/store instructions where the matrix operand is valid (i.e. it can be parsed) but doesn't match what's expected for the instruction.
c-rhodes added inline comments.Jul 14 2021, 4:59 AM
llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir
78

I *think*, and please verify my theory here, that this is just because in this test we're hard coding an enum integer 1245194 for FPR64, as the operand for the INLINEASM instruction. If you add a new regclass that could be shifting the entries in the enum so 1245194 is no longer referencing FPR64. You should be able to just find the new integer value for FPR64 and change the test to use that.

that seems to be the case, cheers @aemerson! I've took the new enum value for FPR64 from llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll and updated it here.

llvm/test/MC/AArch64/SME/st1w-diagnostics.s
3

Is it possible to create a diagnostic test for Match_InvalidMatrixTileVectorH or Match_InvalidMatrixTileVectorV?
They are added in D105570, but I don't see it being used here.

The diagnostics for the matrix operands need some work to be honest, at the moment none of the invalid match types are triggered. Ideally the invalid tile test below would trigger:

case Match_InvalidMatrixTileVectorH32:
  return Error(Loc, "invalid matrix operand, expected za[0-3]h.s");

but za4h.s isn't in matchMatrixRegName so tryParseMatrixRegister returns no match and za4h.s gets parsed as a token, at which point the operand parsing loop in ParseIntruction tries to parse a comma before the next operand, and since there isn't one separating the matrix operand and index, it errors, emitting: 'unexpected token in argument list'.

I suppose we could treat matchMatrixRegName as an error and return MatchOperand_ParseFail. The diagnostic wouldn't be as accurate as what's added in D105570, but it would be an improvement over the current diagnostic which is emitted for the next operand ([).

Returning MatchOperand_ParseFail broke a bunch of unrelated tests so isn't really an option, but I've tweaked the isMatrixRegOperand predicate to return near match. The diagnostics added in D105570 are now used instead of invalid operand for instruction and I've added more diagnostics tests.

CarolineConcatto accepted this revision.Jul 15 2021, 1:51 AM

Hey @c-rhodes
For me the patch looks ok now.
Just before submit fix the formatting.

This revision is now accepted and ready to land.Jul 15 2021, 1:51 AM
This revision was landed with ongoing or failed builds.Jul 16 2021, 3:11 AM
This revision was automatically updated to reflect the committed changes.