This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Don't merge FrameIndex accesses into [F]{LD,ST}X
ClosedPublic

Authored by xen0n on Oct 5 2022, 2:50 AM.

Details

Summary

Otherwise eliminateFrameIndex cannot figure out how to fixup the stack
offset with its stateless logic, because there wouldn't be an immediate
slot for it to trivially write to, and it may not be easy to transform
the surrounding code to make it work.

This fixes a fairly common crash when compiling moderately complex code with
Clang.

Diff Detail

Event Timeline

xen0n created this revision.Oct 5 2022, 2:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 2:50 AM
xen0n requested review of this revision.Oct 5 2022, 2:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 2:50 AM

Thanks for the fix.

I tried to write a simpler test which can reproduce the crash.

define i8 @test(i64 %i) {
  %1 = alloca ptr 
  %2 = getelementptr inbounds i8, ptr %1, i64 %i
  %3 = load i8, ptr %2
  ret i8 %3
}

BTW. Seems @wangleiat has another approach to fix this issue. @wangleiat Do you have any comments?

xen0n added a comment.Oct 7 2022, 7:43 PM

Thanks for the fix.

I tried to write a simpler test which can reproduce the crash.

define i8 @test(i64 %i) {
  %1 = alloca ptr 
  %2 = getelementptr inbounds i8, ptr %1, i64 %i
  %3 = load i8, ptr %2
  ret i8 %3
}

BTW. Seems @wangleiat has another approach to fix this issue. @wangleiat Do you have any comments?

Nice. I simply minimized my cases with bugpoint --run-llc, although I suspected alloca may be the culprit but I didn't try to remove the array structure.

As for which approach to choose for fixing this bug, I'm open to any better proposal. Maybe we can wait for @wangleiat to post his patch to compare.

Thanks for the fix.

I tried to write a simpler test which can reproduce the crash.

define i8 @test(i64 %i) {
  %1 = alloca ptr 
  %2 = getelementptr inbounds i8, ptr %1, i64 %i
  %3 = load i8, ptr %2
  ret i8 %3
}

BTW. Seems @wangleiat has another approach to fix this issue. @wangleiat Do you have any comments?

Nice. I simply minimized my cases with bugpoint --run-llc, although I suspected alloca may be the culprit but I didn't try to remove the array structure.

As for which approach to choose for fixing this bug, I'm open to any better proposal. Maybe we can wait for @wangleiat to post his patch to compare.

This would be better.

xen0n updated this revision to Diff 466247.Oct 7 2022, 9:07 PM

include @SixWeining's cleaner reproducer as an additional test case

xen0n edited the summary of this revision. (Show Details)Oct 7 2022, 9:08 PM
SixWeining accepted this revision.Oct 7 2022, 10:00 PM

LGTM except a small nit. Thanks.

llvm/test/CodeGen/LoongArch/ldx-stx-sp-1.ll
3

Nit: --mattr=+f

This revision is now accepted and ready to land.Oct 7 2022, 10:00 PM
xen0n updated this revision to Diff 466257.Oct 7 2022, 11:02 PM

address @SixWeining's comment