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

[Eli Friedman via Phabricator via llvm-commits]

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