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

[Simon Wallis via Phabricator via llvm-commits]

simonwallis2 added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:1015
+    if (IsSoImm)
+      return (ARM_AM::getSOImmVal(Delta) != -1);
+    if (Delta <= MaxDisp)
----------------
efriedma wrote:
> 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".
> 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?

I take on board your observations about a range of legal addresses.
Although I have not seen evidence of any problems for convergence with non-contiguous offsets,
they are not necessary for this bugfix.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:1015
+    if (IsSoImm)
+      return (ARM_AM::getSOImmVal(Delta) != -1);
+    if (Delta <= MaxDisp)
----------------
simonwallis2 wrote:
> efriedma wrote:
> > 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".
> > 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?
> 
> I take on board your observations about a range of legal addresses.
> Although I have not seen evidence of any problems for convergence with non-contiguous offsets,
> they are not necessary for this bugfix.
> It should be conservatively correct to treat an soimm offset as "a multiple of 4 between -1020 and 1020".

The current treatment of an SoImm offset as "a multiple of 4 between -1020 and 1020" is part of the cause of this bug.

I will split this patch into smaller parts and resubmit.


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

https://reviews.llvm.org/D84923