[PATCH] D118009: [ARM] Make getInstSizeInBytes() use instruction size from InstrInfo.td

[Son Tuan Vu via Phabricator via llvm-commits]

tyb0807 marked 2 inline comments as done.
tyb0807 added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:759
     // pseudo-instruction sizes are zero.
-    return 0;
+    return MCID.getSize();
   case TargetOpcode::BUNDLE:
----------------
nickdesaulniers wrote:
> so the Aarch64 version of this patch does the opposite. It checks the `getSize()` first, then returns early before the switch.
> https://reviews.llvm.org/D117970
> Is there a reason we can't or shouldn't be consistent between the two targets?
So in the AArch64 version, the default size of an expanded pseudo-instruction is 4 bytes, while for ARM, it is more complicated to define such a default size: Thumb1 instructions are 2 bytes, Thumb2 instructions are 2-4 bytes, and ARM instructions are 4 bytes. As a result, we return 0 for pseudo-instructions that we don't have any information about the size.

I'll move the AArch64 code into the default case as well, to make the comparison easier, and add a comment explaining this


================
Comment at: llvm/unittests/Target/ARM/CMakeLists.txt:13
   MC
+  MIRParser
   SelectionDAG
----------------
nickdesaulniers wrote:
> should this be a separate change?
Do you mean the test should be in a separate change? Any reason for doing this?


================
Comment at: llvm/utils/gn/secondary/llvm/unittests/Target/ARM/BUILD.gn:5
   deps = [
+    "//llvm/lib/CodeGen/MIRParser",
     "//llvm/lib/MC",
----------------
nickdesaulniers wrote:
> different change?
See reply above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118009