[PATCH] D76580: [X86InstPrinter] Change printPCRelImm to print the target address in hexadecimal form

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp:293
+/// value (e.g. for jumps and calls). In Intel-style these print slightly
+/// differently than normal immediates. for example, a $ is not emitted.
+///
----------------
Whilst you're reflowing this, please fix "for example" -> "For example".


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp:295
+///
+/// In a stream disassembler (e.g. llvm-objdump -d), Address is the address
+/// of the next instruction.
----------------
I don't know if it matters, but should the reference to `Address` be `\p Address`?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp:302-303
+    uint64_t Target = Address + Op.getImm();
+    if (MAI.getCodePointerSize() == 4)
+      Target &= 0xffffffff;
+    O << formatHex(Target);
----------------
This might be a dumb question, since I'm not familiar with the disassembly stuff, but why is this here?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:739-740
+    if (MI) {
+      uint64_t Addr =
+          Address.Address + (STI.getTargetTriple().isX86() ? Bytes.size() : 0);
+      IP.printInst(MI, Addr, "", STI, OS);
----------------
This is the PC bias bit, right? I thought PC bias was a thing on other platforms too?

This is probably at least deserving of a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76580