Page MenuHomePhabricator

[ARM] Fix so immediates and pc relative checks
ClosedPublic

Authored by simonwallis2 on Sep 1 2020, 8:11 AM.

Details

Summary

Treating an SoImm offset as a multiple of 4 between -1020 and 1020
mis-handles the second of a pair of 16-bit constants where the offset is a multiple of 2 but not a multiple of 4,
leading to an LLVM ERROR: out of range pc-relative fixup value

For 32-bit and larger (64-bit) constants, continue to treat an SoImm offset as a multiple of 4 between -1020 and 1020.
For smaller (16-bit) constants, treat an SoImm offset as a multiple of 1 between -255 and 255.

Diff Detail

Event Timeline

simonwallis2 created this revision.Sep 1 2020, 8:11 AM
simonwallis2 requested review of this revision.Sep 1 2020, 8:11 AM
efriedma added inline comments.Sep 1 2020, 12:52 PM
llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
779

Standard code style in LLVM is something like case ARM::LEApcrelJT: {

781

Does this do something sane for ARM::LEApcrelJT? I don't think the operand for that is a CPI.

789

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.

After review feedback: made some formatting changes in line with standard code style in LLVM.

Added new test variant to highlight a case raised in other review feedback.

simonwallis2 marked an inline comment as done.Sep 2 2020, 2:13 AM
simonwallis2 added inline comments.
llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
779

Thanks.
Done.

781

I have checked a number of arm::LEApcrelJT cases in the test suite
and it does appear to do something sane.

789

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.

efriedma added inline comments.Sep 2 2020, 5:42 PM
llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
781

Okay.

789

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".

simonwallis2 marked an inline comment as done.Sep 3 2020, 8:11 AM
simonwallis2 added inline comments.
llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
789

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.

789

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.

Reworded a comment. NFC.

This revision is now accepted and ready to land.Sep 11 2020, 3:52 PM
This revision was automatically updated to reflect the committed changes.