[PATCH] D86949: [ARM] Fix so immediates and pc relative checks
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 2 17:42:35 PDT 2020
efriedma added inline comments.
================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:788
+ unsigned CPI = I.getOperand(op).getIndex();
+ MachineInstr *CPEMI = CPEMIs[CPI];
+ const Align CPEAlign = getCPEAlign(CPEMI);
----------------
simonwallis2 wrote:
> 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.
>
Okay.
================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:796
+ // which is safe for 2-byte constants with 2-byte alignment.
+ Scale = 1;
+ }
----------------
simonwallis2 wrote:
> 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.
>
>
Oh, I see, the right expression would be something like `Scale = 1 << (LogCPEAlign & ~1);`. Which is probably overkill.
Maybe clarify the comment a bit? The "which is safe for 2-byte constants" is a little misleading; probably better to just say "constants with any alignment".
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86949/new/
https://reviews.llvm.org/D86949
More information about the llvm-commits
mailing list