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

Nick Desaulniers via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 24 12:13:53 PST 2022


nickdesaulniers added a comment.

Please appease the linter for the newly added tests.

Other reviewers should be careful with review here. My major concern that I have not yet verified is whether these pseudo's always expand to the same number of instructions or not, which would affect their corresponding size.



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:759
     // pseudo-instruction sizes are zero.
-    return 0;
+    return MCID.getSize();
   case TargetOpcode::BUNDLE:
----------------
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?


================
Comment at: llvm/unittests/Target/ARM/CMakeLists.txt:13
   MC
+  MIRParser
   SelectionDAG
----------------
should this be a separate change?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118009



More information about the llvm-commits mailing list