[PATCH] D92793: [RISCV] Add (Proposed) Assembler Extend Pseudo-Instructions

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

@craig.topper Thanks for the review!



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:806
 
+def : InstAlias<"zext.b $rd, $rs", (ANDI GPR:$rd, GPR:$rs, 255)>;
+
----------------
craig.topper wrote:
> We two identical InstAliases to this in RISCVInstrInfoB.td when Zbb is enabled. Those can be removed now.
Thanks, I have changed this to use `0xFF` as that looks much more obvious.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1055
+
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0, isCodeGenOnly = 0,
+    isAsmParserOnly = 1 in {
----------------
craig.topper wrote:
> What happens when the Zbb extension is enabled? Do its InstAliases have priority?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92793/new/

https://reviews.llvm.org/D92793