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

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:287
+      case ARM::LEApcrel:
+        return 4;
+      case ARM::t2LEApcrel:
----------------
efriedma wrote:
> 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.
Just as insight, I only applied to the one instruction there was a problem for the moment.
Following the debug information, you can see that the offset values that were computed in this pass were 4 bytes out from the code generated. I did not investigate if it also ocured in arm mode.


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

https://reviews.llvm.org/D84923