[PATCH] D154899: [SystemZ] Allow symbols in immediate asm operands

Thu Jul 20 07:25:50 PDT 2023

uweigand added a comment.

Thanks for the updates, looking really good now.   Just a few cosmetic comments inline.

================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp:69
+  template <SystemZ::FixupKind Kind>
   uint64_t getDispOpValue(const MCInst &MI, unsigned OpNum,
                           SmallVectorImpl<MCFixup> &Fixups,
----------------
Can we rename this to something like `getImmOpValue` - it's no longer just displacements.  Also the comment should be updated.

================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCFixups.h:25
   FK_390_12,
   FK_390_20,

----------------
Can we integrate the handling of displacements into the new scheme and use `FK_390_U12Imm` and `FK_390_S20Imm` here (and elsewhere)?  In generals, displacements should just be a type of immediates.

================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp:60
   case SystemZ::FK_390_12: return ELF::R_390_12;
   case SystemZ::FK_390_20: return ELF::R_390_20;
   }
----------------
Would be nicer to reformat those cases as well so that all use the same style.

================
Comment at: llvm/test/MC/SystemZ/fixups.s:8
+# RUN: llvm-mc -triple s390x-unknown-unknown -mcpu=z13 -filetype=obj %s | \
+# RUN: llvm-objdump -d - | FileCheck %s -check-prefix=CHECK-DIS
+
----------------
Would it make sense to add `CHECK-DIS` lines to the existing tests as well?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154899