[PATCH] D80512: [MC] Changes to help improve target specific symbol disassembly

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:16
 #include "llvm/MC/MCDisassembler/MCSymbolizer.h"
+
 #include <cstdint>
----------------
Unrelated formatting change. Please revert.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1399
         if (Symbols[SI].Type == ELF::STT_AMDGPU_HSA_KERNEL) {
-          // skip amd_kernel_code_t at the begining of kernel symbol (256 bytes)
+          // skip amd_kernel_code_t at the begining of kernel symbol (256bytes)
           Start += 256;
----------------
jhenderson wrote:
> If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.
Actually, why was this line modified at all?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1408-1409
           const auto Limit = End - (std::min)(EndAlign, End - Start);
-          while (End > Limit &&
-            *reinterpret_cast<const support::ulittle32_t*>(&Bytes[End - 4]) == 0)
+          while (End > Limit && *reinterpret_cast<const support::ulittle32_t *>(
+                                    &Bytes[End - 4]) == 0)
             End -= 4;
----------------
jhenderson wrote:
> This also appears to be unrelated to this patch.
If it wasn't clear what I meant before, since this change is unrelated to this patch (it's just a pure formatting change and you aren't touching this bit of code), I think it needs reverting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80512