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

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

In D118009#3267198 <https://reviews.llvm.org/D118009#3267198>, @nickdesaulniers wrote:

> 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.

If these sizes are wrong, then this patch doesn't make that worse (but this is a good opportunity to verify that the sizes are correct). This override tends to only be required for pseudos that are expanded very late (usually in the Asm Printer), and problems with it tend to affect branch relaxation, iirc.



================
Comment at: llvm/unittests/Target/ARM/CMakeLists.txt:13
   MC
+  MIRParser
   SelectionDAG
----------------
tyb0807 wrote:
> nickdesaulniers wrote:
> > should this be a separate change?
> Do you mean the test should be in a separate change? Any reason for doing this?
Adding tests with these changes seems like the right thing to do, and these tests need a MIR parser, so this change is required.


================
Comment at: llvm/unittests/Target/ARM/InstSizes.cpp:87
+  };
+
+  runChecks(TM.get(), II, "",
----------------
Can you add tests for the instructions remaining in the switch block, each of which has more complex logic for calculating its size? Right now you're only testing the things you moved out of the switch block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118009