This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add (Proposed) Assembler Extend Pseudo-Instructions
ClosedPublic

Authored by lenary on Dec 7 2020, 3:11 PM.

Details

Summary

There is an in-progress proposal for the following pseudo-instructions
in the assembler, to complement the existing sext.w rv64i instruction:

  • sext.b
  • sext.h
  • zext.b
  • zext.h
  • zext.w

The .b and .h variants are available with rv32i and rv64i, and zext.w is
only available with rv64i.

These are implemented primarily as pseudo-instructions, as these instructions
expand to multiple real instructions. In the case of zext.b, this expands to a
single rv32/64i instruction, so it is implemented with an InstAlias (like
sext.w is on rv64i).

The proposal is available here: https://github.com/riscv/riscv-asm-manual/pull/61

Diff Detail

Event Timeline

lenary created this revision.Dec 7 2020, 3:11 PM
lenary requested review of this revision.Dec 7 2020, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 3:11 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2294

This is indented too far

llvm/lib/Target/RISCV/RISCVInstrInfo.td
812

We two identical InstAliases to this in RISCVInstrInfoB.td when Zbb is enabled. Those can be removed now.

1061

What happens when the Zbb extension is enabled? Do its InstAliases have priority?

lenary updated this revision to Diff 310041.Dec 7 2020, 3:29 PM

Fix missing semicolon/indentation.

lenary updated this revision to Diff 310048.Dec 7 2020, 4:12 PM
lenary marked an inline comment as done.
  • Remove overlapping instruction aliases for zext.b.
  • Ensure Pseudos are only matched if Zbb not available.
lenary added a comment.Dec 7 2020, 4:17 PM

@craig.topper Thanks for the review!

llvm/lib/Target/RISCV/RISCVInstrInfo.td
812

Thanks, I have changed this to use 0xFF as that looks much more obvious.

1061

There's a description of whether an InstAlias overrides the Instruction it is an alias for, but not whether it overrides other instructions. I've updated this to ensure these definitions are only available without Zbb, so we always get the single-instruction variants if Zbb is available. This means we don't rely on the implicit behaviour of TableGen etc, which seems to be undocumented in this case.

The Zbb encoding tests *were* passing, including the Zbb tests, but I agree there's too many moving pieces and not enough coverage for us to rely on that.

This looks good to me now.

That's LGTM too, just waiting the spec merge :)

asb accepted this revision.Dec 10 2020, 5:00 AM

This needs a rebase, but otherwise looks good to me. Thanks Sam!

This revision is now accepted and ready to land.Dec 10 2020, 5:00 AM
This revision was landed with ongoing or failed builds.Dec 10 2020, 11:26 AM
This revision was automatically updated to reflect the committed changes.
dmajor added a subscriber: dmajor.Dec 10 2020, 12:00 PM

Heads up: the tests are failing on the bots http://lab.llvm.org:8011/#/builders/109/builds/4482/steps/6/logs/FAIL__LLVM__calling-conv-sext-zext_ll

(Sorry for the noise if the automation already notified you)

Printing these aliases by default has broken -fno-integrated-assembler unless the user has also updated their GNU assembler version to one that supports these aliases. For example, https://github.com/ClangBuiltLinux/linux/issues/1220 Should we back off the printing and just leave the parsing?