This is an archive of the discontinued LLVM Phabricator instance.

Sparc: Add the alternate address space load/store instructions.
ClosedPublic

Authored by jyknight on Apr 8 2015, 1:09 PM.

Details

Summary

Sparc: Add the "alternate address space" load/store instructions.

  • Adds support for the asm syntax, which has an immediate integer "ASI" (address space identifier) appearing after an address, before a comma.
  • Adds the various-width load, store, and swap in alternate address space instructions. (ldsba, ldsha, lduba, lduha, lda, stba, stha, sta, swapa)

This does not attempt to hook these instructions up to pointer address
spaces in LLVM, although that would probably be a reasonable thing to
do in the future.

Diff Detail

Event Timeline

jyknight updated this revision to Diff 23437.Apr 8 2015, 1:09 PM
jyknight retitled this revision from to Sparc: Add the alternate address space load/store instructions..
jyknight updated this object.
jyknight edited the test plan for this revision. (Show Details)
jyknight added a subscriber: Unknown Object (MLST).

Some inline comments. Can you elaborate on the patch description as to what you're doing and why here?

Thanks!

-eric

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

Comments are complete sentences with periods.

lib/Target/Sparc/Disassembler/SparcDisassembler.cpp
311

No blocks around single stmt if conditionals.

492

Ditto on the single block stmts after conditionals.

lib/Target/Sparc/SparcInstr64Bit.td
489–490

Can you comment on the asi magic value here and why you put it as part of the let stmt?

jyknight updated this revision to Diff 25092.May 6 2015, 2:52 PM

Updated per echristo's review comments.

jyknight updated this object.May 6 2015, 2:53 PM
jyknight updated this object.May 6 2015, 3:03 PM
jyknight added inline comments.May 6 2015, 3:06 PM
lib/Target/Sparc/SparcInstr64Bit.td
489–490

That magic value was a "template" parameter to F3_1_asi before, but I wanted it to be a passed-in immediate, from the asm, for the other new uses of F3_1_asi.

echristo accepted this revision.May 13 2015, 3:11 PM
echristo added a reviewer: echristo.

A few comments inline, but at least for assembly this is probably ok. You'll need to add some comments/code generation options to the patterns, but for the first part it's ok. Don't worry about getting a fresh LGTM with the comments, I'll post-commit review those.

-eric

lib/Target/Sparc/SparcInstr64Bit.td
489

What I meant in my request is "what's this magic number? Can you add an appropriate comment as to what it is?" Feel free to do that after since you're just moving an existing magic number around.

lib/Target/Sparc/SparcInstrInfo.td
292

You probably want a pattern if you expect to match anything during code generation. If you don't plan on adding that now then having a couple of block comments would be good.

This revision is now accepted and ready to land.May 13 2015, 3:11 PM
This revision was automatically updated to reflect the committed changes.