[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:796
+                // which is safe for 2-byte constants with 2-byte alignment.
+                Scale = 1;
+            }
----------------
efriedma wrote:
> 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".
> Oh, I see, the right expression would be something like `Scale = 1 << (LogCPEAlign & ~1);`.  Which is probably overkill.

In order to represent a reachable range of contiguous offsets, the value of Scale must be all of:
  - An even power of 2 for a valid SoImm encoding.
  - Not less than the alignment of the constant.
  - Not more than the instruction width in bytes (4).

So the only permitted values are 1 or 4.

Computing 1<<(LogCPEAlign & ~1) would violate this if constants with alignment > 8 bytes were added.


================
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:
> > 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".
> > Oh, I see, the right expression would be something like `Scale = 1 << (LogCPEAlign & ~1);`.  Which is probably overkill.
> 
> In order to represent a reachable range of contiguous offsets, the value of Scale must be all of:
>   - An even power of 2 for a valid SoImm encoding.
>   - Not less than the alignment of the constant.
>   - Not more than the instruction width in bytes (4).
> 
> So the only permitted values are 1 or 4.
> 
> Computing 1<<(LogCPEAlign & ~1) would violate this if constants with alignment > 8 bytes were added.
> 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".

How about changing
   // We'll pretend the maximum offset is 255 * 1,
   // which is safe for 2-byte constants with 2-byte alignment.
to
   // We'll pretend the maximum offset is 255 * 1,
   // which is safe for constants with less than 4-byte alignment.



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

https://reviews.llvm.org/D86949