[PATCH] D86949: [ARM] Fix so immediates and pc relative checks
Simon Wallis via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 2 02:13:58 PDT 2020
simonwallis2 marked an inline comment as done.
simonwallis2 added inline comments.
================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:786
IsSoImm = true;
+ {
+ unsigned CPI = I.getOperand(op).getIndex();
----------------
efriedma wrote:
> Standard code style in LLVM is something like `case ARM::LEApcrelJT: {`
Thanks.
Done.
================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:788
+ unsigned CPI = I.getOperand(op).getIndex();
+ MachineInstr *CPEMI = CPEMIs[CPI];
+ const Align CPEAlign = getCPEAlign(CPEMI);
----------------
efriedma wrote:
> Does this do something sane for ARM::LEApcrelJT? I don't think the operand for that is a CPI.
I have checked a number of arm::LEApcrelJT cases in the test suite
and it does appear to do something sane.
================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:796
+ // which is safe for 2-byte constants with 2-byte alignment.
+ Scale = 1;
+ }
----------------
efriedma wrote:
> Instead of checking "if (LogCPEAlign >= 2)", could we just set "Scale = CPEAlign;"? I don't care that much about the quality of lowering for 16-bit constant pool entries, but the intent would be more clear.
No, we could not just set Scale = CPEAlign.
Setting Scale = CPEAlign would cause
LLVM ERROR: out of range pc-relative fixup value
for various of offsets. I have added one such case to constant-island-soimm-limit16.mir.
It would also change the behaviour for 64-bit constants.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86949/new/
https://reviews.llvm.org/D86949
More information about the llvm-commits
mailing list