[PATCH] D84923: [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:1015
+    if (IsSoImm)
+      return (ARM_AM::getSOImmVal(Delta) != -1);
+    if (Delta <= MaxDisp)
----------------
I'm a little concerned that using getSOImmVal is going to throw off the algorithm.  The algorithm is based around there being some range of legal addresses, and moving constants into that range.  The current code doesn't generate discontinuities like this, and I'm worried adding them will cause problems for convergence.

Can we split this change off from whatever changes are necessary for correctness?  It should be conservatively correct to treat an soimm offset as "a multiple of 4 between -1020 and 1020".


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:287
+      case ARM::LEApcrel:
+        return 4;
+      case ARM::t2LEApcrel:
----------------
simonwallis2 wrote:
> dmgreen wrote:
> > simonwallis2 wrote:
> > > dmgreen wrote:
> > > > Why 4 and not 8? I think it's meant to be "2 instructions widths"
> > > Value of 4 was determined empirically.
> > You fill me with confidence. And would 8 empirically have problems too?
> Values of 0 or 8 cause:  error: out of range pc-relative fixup value
> A value of 4 did not cause failures.
> 
> Why 4 and not 8? 
> It depends on where you start measuring.  I have not pinpointed where, 
> but I believe that the base address from which the offset was calculated has already been adjusted by 4 bytes,
> to take into account the width of the load instruction,
> that is to say that it points to the address of the instruction after the load, rather than the load itself.
>From the ARM reference manual:

• When executing an ARM instruction, PC reads as the address of the current instruction plus 8.
• When executing a Thumb instruction, PC reads as the address of the current instruction plus 4.

So it sort of makes sense that we'd need to distinguish between ARM-mode and Thumb-mode... but it's suspicious that this is only getting applied to LEApcrel, and not all ARM-mode instructions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84923/new/

https://reviews.llvm.org/D84923