[PATCH] D108961: [RISCV] MC relaxation for out-of-range conditional branch.

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:191
+  case RISCV::PseudoLongBEQ:
+    if (UseCompressedBr)
+      return RISCV::C_BNEZ;
----------------
Is this right?  BEQ is not the same as BEQZ?  Isn't there a missing zero check here?

Ah, no, that's checked in the caller.  Could you adjust naming or comments to make that clear please?  In fact, so much of the caller differs for this case, maybe it should just be an if/else block there?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:221
+
+  if (IsEqTest && SrcReg1.getReg() == RISCV::X0)
+    std::swap(SrcReg1, SrcReg2);
----------------
Doing this when we're not going to use the compressed instruction seems like an odd canonicalization from the assembler.  Not objecting, just wondering why.  


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:265
   unsigned Size = Desc.getSize();
+  unsigned Opcode = MI.getOpcode();
 
----------------
Can you precommit a change which pulls this out and converts these branches to a switch?  Doing so reduces the delta.  


================
Comment at: llvm/test/MC/RISCV/long-conditional-jump.s:6
+# RUN:     | llvm-objdump -d -M no-aliases - \
+# RUN:     | FileCheck --check-prefix=CHECK-INST-C %s
+
----------------
Use check-prefixes to common all but the couple which actually differ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108961