Page MenuHomePhabricator

[LSR] Fix for pre-indexed generated constant offset
ClosedPublic

Authored by stelios-arm on Apr 13 2021, 6:51 AM.

Details

Summary

This patch changed the isLegalUse check to ensure that
LSRInstance::GenerateConstantOffsetsImpl generates an
offset that results in a legal addressing mode and
formula. The check is changed to look similar to the
assert check used for illegal formulas.

Currently when building a bootstrap build with the following flags:

-DCMAKE_C_FLAGS="${CMAKE_C_FLAGS} -mllvm -lsr-preferred-addressing-mode=preindexed"  -DCMAKE_CXX_FLAGS="${CMAKE_C_FLAGS} -mllvm -lsr-preferred-addressing-mode=preindexed"

, it fails at compiling TargetLoweringBase.cpp because it generates an illegal formula.

This patch fixes this and allows to build a bootstrap build with the aforementioned flags.

Diff Detail

Event Timeline

stelios-arm created this revision.Apr 13 2021, 6:51 AM
stelios-arm requested review of this revision.Apr 13 2021, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 6:51 AM

I am afraid we need to add a test case for this because this doesn't seem to be covered by existing tests. So, I think you need to make a minimal reproducer of the input file that triggers the assertion failure.

dmgreen added inline comments.Apr 13 2021, 8:42 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
3794–3795

This looks like it's already checking isLegalUse. Should it be without the -Offset?

I am afraid we need to add a test case for this because this doesn't seem to be covered by existing tests. So, I think you need to make a minimal reproducer of the input file that triggers the assertion failure.

I will add one in the next revision. For the meantime, here is one.

stelios-arm added inline comments.Apr 13 2021, 9:02 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
3794–3795

Yes that will do. I am not sure whether such change will result in any side effects. I will test it, and if there are none I will update it in the next revision accordingly.

stelios-arm edited the summary of this revision. (Show Details)
  1. Added a test that triggers the assertion failure.
  2. Changed the isLegalUse parameters in LSRInstance::GenerateConstantOffsetsImpl to look the same as the assertion used for illegal formulas.

In this link you can see how it results in an error.

SjoerdMeijer added inline comments.Apr 14 2021, 5:08 AM
llvm/test/Transforms/LoopStrengthReduce/AArch64/pre-inc-offset-check.ll
7

Thanks for adding this, nice reproducer!

I think this tests could benefit from some comments explaining what exactly we are testing here. I.e., from the test name one might expect preindexed loads/store, but they don't appear in the codegen.

And forgot to add, and just double checking, this passes your bootstrap build and test again?

And forgot to add, and just double checking, this passes your bootstrap build and test again?

Yes this compiles the bootstrap build and the test-suite too.

llvm/test/Transforms/LoopStrengthReduce/AArch64/pre-inc-offset-check.ll
7

I will add some in the next revision.

stelios-arm marked an inline comment as done.
  1. Added comments to the new test.
  2. Fixed the failing AMDGPU tests.
SjoerdMeijer added inline comments.Apr 15 2021, 5:52 AM
llvm/test/Transforms/LoopStrengthReduce/AMDGPU/atomics.ll
95

What happened to these atomics? I think this is crucial for these tests, and we still need to match them.
Same for the atomic reads/writes in tests above and also below here.

Added back some of the atomic tests.

stelios-arm marked an inline comment as done.Apr 15 2021, 7:17 AM
stelios-arm added inline comments.
llvm/test/Transforms/LoopStrengthReduce/AMDGPU/atomics.ll
95

Oh yes. They just changed a bit, I added them back.

SjoerdMeijer accepted this revision.Apr 15 2021, 7:27 AM

Thanks, looks like a good bug fix to me.

This revision is now accepted and ready to land.Apr 15 2021, 7:27 AM
This revision was automatically updated to reflect the committed changes.
stelios-arm marked an inline comment as done.