This is an archive of the discontinued LLVM Phabricator instance.

Differential D100383

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

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

Details

Reviewers
SjoerdMeijer
dmgreen
sanwou01
samparker
fhahn
NickGuy
Commits
rGbf147c465352: [LSR] Fix for pre-indexed generated constant offset
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

Unit TestsFailed

TimeTest
100 msx64 debian > LLVM.CodeGen/AMDGPU::cf-loop-on-constant.ll
40 msx64 debian > LLVM.Transforms/LoopStrengthReduce/AMDGPU::atomics.ll
70 msx64 debian > LLVM.Transforms/LoopStrengthReduce/AMDGPU::different-addrspace-addressing-mode-loops.ll
130 msx64 windows > LLVM.CodeGen/AMDGPU::cf-loop-on-constant.ll
50 msx64 windows > LLVM.Transforms/LoopStrengthReduce/AMDGPU::atomics.ll
View Full Test Results (6 Failed)

Event Timeline

stelios-arm created this revision.Apr 13 2021, 6:51 AM
Herald added subscribers: arphaman, hiraditya. · View Herald TranscriptApr 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
Herald added a subscriber: llvm-commits. · View Herald Transcript
SjoerdMeijer added a comment.Apr 13 2021, 7:04 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.

Harbormaster completed remote builds in B98476: Diff 337135.Apr 13 2021, 7:50 AM
dmgreen added inline comments.Apr 13 2021, 8:42 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
3795

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

stelios-arm added a comment.Apr 13 2021, 9:00 AM
In D100383#2685704, @SjoerdMeijer wrote:

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
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 updated this revision to Diff 337396.Apr 14 2021, 4:03 AM
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.
stelios-arm added a comment.Apr 14 2021, 4:05 AM

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

Harbormaster completed remote builds in B98657: Diff 337396.Apr 14 2021, 4:37 AM
SjoerdMeijer added inline comments.Apr 14 2021, 5:08 AM
llvm/test/Transforms/LoopStrengthReduce/AArch64/pre-inc-offset-check.ll
6

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.

SjoerdMeijer added a comment.Apr 14 2021, 5:33 AM

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

stelios-arm added a comment.Apr 14 2021, 5:49 AM
In D100383#2688643, @SjoerdMeijer wrote:

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
6

I will add some in the next revision.

stelios-arm updated this revision to Diff 337730.Apr 15 2021, 5:45 AM
stelios-arm marked an inline comment as done.
  1. Added comments to the new test.
  2. Fixed the failing AMDGPU tests.
Herald added subscribers: kerbowa, jfb, nhaehnle, jvesely. · View Herald TranscriptApr 15 2021, 5:45 AM
SjoerdMeijer added inline comments.Apr 15 2021, 5:52 AM
llvm/test/Transforms/LoopStrengthReduce/AMDGPU/atomics.ll
95 ↗(On Diff #337730)

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.

Harbormaster completed remote builds in B98893: Diff 337730.Apr 15 2021, 7:08 AM
stelios-arm updated this revision to Diff 337758.Apr 15 2021, 7:16 AM

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 ↗(On Diff #337730)

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
Harbormaster completed remote builds in B98911: Diff 337758.Apr 15 2021, 8:14 AM
Closed by commit rGbf147c465352: [LSR] Fix for pre-indexed generated constant offset (authored by stelios-arm). · Explain WhyApr 15 2021, 8:49 AM
This revision was automatically updated to reflect the committed changes.
stelios-arm marked an inline comment as done.
stelios-arm added a commit: rGbf147c465352: [LSR] Fix for pre-indexed generated constant offset.

Revision Contents

PathSize
llvm/
lib/
Transforms/
Scalar/
2 lines
test/
Transforms/
LoopStrengthReduce/
AArch64/
41 lines

Diff 337396

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

Show First 20 Lines • Show All 3,786 Lines • ▼ Show 20 Lines
void LSRInstance::GenerateConstantOffsetsImpl( void LSRInstance::GenerateConstantOffsetsImpl(
LSRUse &LU, unsigned LUIdx, const Formula &Base, LSRUse &LU, unsigned LUIdx, const Formula &Base,
const SmallVectorImpl<int64_t> &Worklist, size_t Idx, bool IsScaledReg) { const SmallVectorImpl<int64_t> &Worklist, size_t Idx, bool IsScaledReg) {
auto GenerateOffset = [&](const SCEV *G, int64_t Offset) { auto GenerateOffset = [&](const SCEV *G, int64_t Offset) {
Formula F = Base; Formula F = Base;
F.BaseOffset = (uint64_t)Base.BaseOffset - Offset; F.BaseOffset = (uint64_t)Base.BaseOffset - Offset;
if (isLegalUse(TTI, LU.MinOffset - Offset, LU.MaxOffset - Offset, LU.Kind, if (isLegalUse(TTI, LU.MinOffset, LU.MaxOffset, LU.Kind,
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-    if (isLegalUse(TTI, LU.MinOffset, LU.MaxOffset, LU.Kind,
-                   LU.AccessTy, F)) {
+    if (isLegalUse(TTI, LU.MinOffset, LU.MaxOffset, LU.Kind, LU.AccessTy, F)) {
Lint: Pre-merge checks: clang-format: please reformat the code ``` - if (isLegalUse(TTI, LU.MinOffset, LU.MaxOffset…
dmgreenUnsubmitted
Not Done
ReplyInline Actions

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

dmgreen: This looks like it's already checking isLegalUse. Should it be without the -Offset?
stelios-armAuthorUnsubmitted
Done
ReplyInline Actions

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: Yes that will do. I am not sure whether such change will result in any side effects. I will…
LU.AccessTy, F)) { LU.AccessTy, F)) {
// Add the offset to the base register. // Add the offset to the base register.
const SCEV *NewG = SE.getAddExpr(SE.getConstant(G->getType(), Offset), G); const SCEV *NewG = SE.getAddExpr(SE.getConstant(G->getType(), Offset), G);
// If it cancelled out, drop the base register, otherwise update it. // If it cancelled out, drop the base register, otherwise update it.
if (NewG->isZero()) { if (NewG->isZero()) {
if (IsScaledReg) { if (IsScaledReg) {
F.Scale = 0; F.Scale = 0;
F.ScaledReg = nullptr; F.ScaledReg = nullptr;
▲ Show 20 Lines • Show All 2,203 Lines • Show Last 20 Lines

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

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=aarch64-none-eabi -lsr-preferred-addressing-mode=preindexed %s -o - | FileCheck %s
%"Type" = type <{[166 x [338 x i8]]}>
define void @test_pre_inc(%"Type"* %p) {
SjoerdMeijerUnsubmitted
Done
ReplyInline Actions

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.

SjoerdMeijer: Thanks for adding this, nice reproducer! I think this tests could benefit from some comments…
stelios-armAuthorUnsubmitted
Done
ReplyInline Actions

I will add some in the next revision.

stelios-arm: I will add some in the next revision.
; CHECK-LABEL: test_pre_inc:
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: add x8, x0, #340 // =340
; CHECK-NEXT: mov w9, #165
; CHECK-NEXT: mov w10, #2
; CHECK-NEXT: .LBB0_1: // %main
; CHECK-NEXT: // =>This Inner Loop Header: Depth=1
; CHECK-NEXT: stur wzr, [x8, #-1]
; CHECK-NEXT: strb w10, [x8]
; CHECK-NEXT: subs x9, x9, #1 // =1
; CHECK-NEXT: add x8, x8, #338 // =338
; CHECK-NEXT: b.ne .LBB0_1
; CHECK-NEXT: // %bb.2: // %exit
; CHECK-NEXT: ret
entry:
br label %main
exit:
ret void
if.then:
%arrayidx.i = getelementptr inbounds %"Type", %"Type"* %p, i64 0, i32 0, i64 %indvars, i64 1
%0 = bitcast i8* %arrayidx.i to i32*
store i32 0, i32* %0, align 1
br label %if.end
if.end:
%arrayidx.p = getelementptr inbounds %"Type", %"Type"* %p, i64 0, i32 0, i64 %indvars, i64 2
store i8 2, i8* %arrayidx.p, align 1
%indvars.iv.next = add nuw nsw i64 %indvars, 1
%add.i = add nuw i8 %begin, 1
%cmp.i.not = icmp eq i64 %indvars.iv.next, 166
br i1 %cmp.i.not, label %exit, label %main
main:
%begin = phi i8 [ 1, %entry ], [ %add.i, %if.end ]
%indvars = phi i64 [ 1, %entry ], [ %indvars.iv.next, %if.end ]
br label %if.then
}