[PATCH] D86949: [ARM] Fix so immediates and pc relative checks

[Simon Wallis via Phabricator via llvm-commits]

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