[PATCH] D118009: [ARM] Make getInstSizeInBytes() use instruction size from InstrInfo.td
Son Tuan Vu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 25 01:57:42 PST 2022
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
More information about the llvm-commits
mailing list