Page MenuHomePhabricator

[Scalarizer] ExtractElement handling w/ constant extract index
ClosedPublic

Authored by lebedev.ri on Jul 2 2020, 3:30 PM.

Details

Summary

It appears to be better IR-wise to aggressively scalarize it,
rather than relying on gathering it, and leaving it as-is.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jul 2 2020, 3:30 PM
lebedev.ri marked an inline comment as done.Jul 2 2020, 3:33 PM
lebedev.ri added inline comments.
llvm/test/Transforms/Scalarizer/basic.ll
611 ↗(On Diff #275249)

Previously we'd end up with https://godbolt.org/z/HDKdqh

%val0 = load <4 x i32>, <4 x i32>* %src, align 16
%val0.i0 = extractelement <4 x i32> %val0, i32 0
%val1.i0 = shl i32 1, %val0.i0
%val0.i1 = extractelement <4 x i32> %val0, i32 1
%val1.i1 = shl i32 2, %val0.i1
%val0.i2 = extractelement <4 x i32> %val0, i32 2
%val1.i2 = shl i32 3, %val0.i2
%val0.i3 = extractelement <4 x i32> %val0, i32 3
%val1.i3 = shl i32 4, %val0.i3
%val1.upto0 = insertelement <4 x i32> undef, i32 %val1.i0, i32 0
%val1.upto1 = insertelement <4 x i32> %val1.upto0, i32 %val1.i1, i32 1
%val1.upto2 = insertelement <4 x i32> %val1.upto1, i32 %val1.i2, i32 2
%val1 = insertelement <4 x i32> %val1.upto2, i32 %val1.i3, i32 3
%val2 = extractelement <4 x i32> %val1, i32 3
ret i32 %val2
jdoerfert accepted this revision.Jul 2 2020, 3:36 PM

LGTM.

This revision is now accepted and ready to land.Jul 2 2020, 3:36 PM
lebedev.ri updated this revision to Diff 275262.Jul 2 2020, 4:55 PM
lebedev.ri retitled this revision from [Scalarizer] Constant ExtractElement Hanlding to [Scalarizer] ExtractElement handling w/ constant extract index.

Reordered, precommitted tests.

This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.Jul 6 2020, 7:26 AM

@lebedev.ri this is causing assertion failures and verification failures in some of our downstream tests. Here's a test case:

$ cat reduced.ll
define void @main(<3 x i32> inreg %w) {
entry:
  %a = extractelement <3 x i32> undef, i32 0
  %b = extractelement <3 x i32> undef, i32 1
  %x = extractelement <3 x i32> %w, i32 2
  %y = insertelement <4 x i32> undef, i32 %x, i32 2
  %z = insertelement <4 x i32> %y, i32 undef, i32 3
  store <4 x i32> %z, <4 x i32> addrspace(7)* undef, align 16
  ret void
}
$ ~/llvm-debug/bin/opt -scalarizer -o /dev/null reduced.ll
Instruction does not dominate all uses!
  <badref> = extractelement [145938144 x half] <badref>, i32 undef
  %z.upto2 = insertelement <4 x i32> undef, i32 <badref>, i32 2
in function main
LLVM ERROR: Broken function found, compilation aborted!

@lebedev.ri this is causing assertion failures and verification failures in some of our downstream tests. Here's a test case:

$ cat reduced.ll
define void @main(<3 x i32> inreg %w) {
entry:
  %a = extractelement <3 x i32> undef, i32 0
  %b = extractelement <3 x i32> undef, i32 1
  %x = extractelement <3 x i32> %w, i32 2
  %y = insertelement <4 x i32> undef, i32 %x, i32 2
  %z = insertelement <4 x i32> %y, i32 undef, i32 3
  store <4 x i32> %z, <4 x i32> addrspace(7)* undef, align 16
  ret void
}
$ ~/llvm-debug/bin/opt -scalarizer -o /dev/null reduced.ll
Instruction does not dominate all uses!
  <badref> = extractelement [145938144 x half] <badref>, i32 undef
  %z.upto2 = insertelement <4 x i32> undef, i32 <badref>, i32 2
in function main
LLVM ERROR: Broken function found, compilation aborted!

Thanks for test case, looking.

@lebedev.ri this is causing assertion failures and verification failures in some of our downstream tests. Here's a test case:

$ cat reduced.ll
define void @main(<3 x i32> inreg %w) {
entry:
  %a = extractelement <3 x i32> undef, i32 0
  %b = extractelement <3 x i32> undef, i32 1
  %x = extractelement <3 x i32> %w, i32 2
  %y = insertelement <4 x i32> undef, i32 %x, i32 2
  %z = insertelement <4 x i32> %y, i32 undef, i32 3
  store <4 x i32> %z, <4 x i32> addrspace(7)* undef, align 16
  ret void
}
$ ~/llvm-debug/bin/opt -scalarizer -o /dev/null reduced.ll
Instruction does not dominate all uses!
  <badref> = extractelement [145938144 x half] <badref>, i32 undef
  %z.upto2 = insertelement <4 x i32> undef, i32 <badref>, i32 2
in function main
LLVM ERROR: Broken function found, compilation aborted!

Thanks for test case, looking.

Thank you for a great reproducer! Fixed in rGdb05f2e34a5e9380ddcc199d6687531108d795e4.

foad added a comment.Jul 7 2020, 1:10 AM

Thanks. I can confirm that it fixes all the failures I was seeing.

bjope added a comment.Jul 7 2020, 6:32 AM

I unfortunately still see some problems (related to the Scalarizer changes, and probably this patch):

> cat scalarizer-bug.ll
; RUN: opt < %s -scalarizer -S -o -

define void @foo() {
vector.ph:
  br label %vector.body115

vector.body115:                                   ; preds = %vector.body115, %vector.ph
  %vector.recur = phi <4 x i16> [ undef, %vector.ph ], [ %wide.load125, %vector.body115 ]
  %wide.load125 = load <4 x i16>, <4 x i16>* undef, align 1
  br i1 undef, label %middle.block113, label %vector.body115

middle.block113:                                  ; preds = %vector.body115
  ret void
}


-----------------------------------------------------

> ~/opt.master -scalarizer scalarizer-bug.ll -S
opt.master: ../lib/IR/Value.cpp:458: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/uabbpet/opt.master -scalarizer scalarizer-bug.ll -S 
1.      Running pass 'Function Pass Manager' on module 'scalarizer-bug.ll'.
2.      Running pass 'Scalarize vector operations' on function '@foo'
Abort
bjope added a subscriber: uabelho.Jul 7 2020, 6:35 AM

I unfortunately still see some problems (related to the Scalarizer changes, and probably this patch):

> cat scalarizer-bug.ll
; RUN: opt < %s -scalarizer -S -o -

define void @foo() {
vector.ph:
  br label %vector.body115

vector.body115:                                   ; preds = %vector.body115, %vector.ph
  %vector.recur = phi <4 x i16> [ undef, %vector.ph ], [ %wide.load125, %vector.body115 ]
  %wide.load125 = load <4 x i16>, <4 x i16>* undef, align 1
  br i1 undef, label %middle.block113, label %vector.body115

middle.block113:                                  ; preds = %vector.body115
  ret void
}


-----------------------------------------------------

> ~/opt.master -scalarizer scalarizer-bug.ll -S
opt.master: ../lib/IR/Value.cpp:458: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/uabbpet/opt.master -scalarizer scalarizer-bug.ll -S 
1.      Running pass 'Function Pass Manager' on module 'scalarizer-bug.ll'.
2.      Running pass 'Scalarize vector operations' on function '@foo'
Abort

Acknowledged, looking.

I unfortunately still see some problems (related to the Scalarizer changes, and probably this patch):

> cat scalarizer-bug.ll
; RUN: opt < %s -scalarizer -S -o -

define void @foo() {
vector.ph:
  br label %vector.body115

vector.body115:                                   ; preds = %vector.body115, %vector.ph
  %vector.recur = phi <4 x i16> [ undef, %vector.ph ], [ %wide.load125, %vector.body115 ]
  %wide.load125 = load <4 x i16>, <4 x i16>* undef, align 1
  br i1 undef, label %middle.block113, label %vector.body115

middle.block113:                                  ; preds = %vector.body115
  ret void
}


-----------------------------------------------------

> ~/opt.master -scalarizer scalarizer-bug.ll -S
opt.master: ../lib/IR/Value.cpp:458: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/uabbpet/opt.master -scalarizer scalarizer-bug.ll -S 
1.      Running pass 'Function Pass Manager' on module 'scalarizer-bug.ll'.
2.      Running pass 'Scalarize vector operations' on function '@foo'
Abort

Acknowledged, looking.

Hm, this is saddening. I've fixed it in rG16266e63963ad6ee27ad21983a9366ab313dfd03, but are there more?

Forgot to say, thank you for the reduced reproducer!

bjope added a comment.Jul 7 2020, 7:26 AM

I unfortunately still see some problems (related to the Scalarizer changes, and probably this patch):

> cat scalarizer-bug.ll
; RUN: opt < %s -scalarizer -S -o -

define void @foo() {
vector.ph:
  br label %vector.body115

vector.body115:                                   ; preds = %vector.body115, %vector.ph
  %vector.recur = phi <4 x i16> [ undef, %vector.ph ], [ %wide.load125, %vector.body115 ]
  %wide.load125 = load <4 x i16>, <4 x i16>* undef, align 1
  br i1 undef, label %middle.block113, label %vector.body115

middle.block113:                                  ; preds = %vector.body115
  ret void
}


-----------------------------------------------------

> ~/opt.master -scalarizer scalarizer-bug.ll -S
opt.master: ../lib/IR/Value.cpp:458: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/uabbpet/opt.master -scalarizer scalarizer-bug.ll -S 
1.      Running pass 'Function Pass Manager' on module 'scalarizer-bug.ll'.
2.      Running pass 'Scalarize vector operations' on function '@foo'
Abort

Acknowledged, looking.

Hm, this is saddening. I've fixed it in rG16266e63963ad6ee27ad21983a9366ab313dfd03, but are there more?

Ah, maybe I was so occupied reducing the fault so I missed that there was another fix. I actually did take a look in github just to avoid reporting a problem that had been fixed, but must have done it just before rG16266e63963ad6ee27ad21983a9366ab313dfd03 landed.

I'll fetch, rebuild, and test. Let you know if it didn't help.

I unfortunately still see some problems (related to the Scalarizer changes, and probably this patch):

> cat scalarizer-bug.ll
; RUN: opt < %s -scalarizer -S -o -

define void @foo() {
vector.ph:
  br label %vector.body115

vector.body115:                                   ; preds = %vector.body115, %vector.ph
  %vector.recur = phi <4 x i16> [ undef, %vector.ph ], [ %wide.load125, %vector.body115 ]
  %wide.load125 = load <4 x i16>, <4 x i16>* undef, align 1
  br i1 undef, label %middle.block113, label %vector.body115

middle.block113:                                  ; preds = %vector.body115
  ret void
}


-----------------------------------------------------

> ~/opt.master -scalarizer scalarizer-bug.ll -S
opt.master: ../lib/IR/Value.cpp:458: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/uabbpet/opt.master -scalarizer scalarizer-bug.ll -S 
1.      Running pass 'Function Pass Manager' on module 'scalarizer-bug.ll'.
2.      Running pass 'Scalarize vector operations' on function '@foo'
Abort

Acknowledged, looking.

Hm, this is saddening. I've fixed it in rG16266e63963ad6ee27ad21983a9366ab313dfd03, but are there more?

Ah, maybe I was so occupied reducing the fault so I missed that there was another fix. I actually did take a look in github just to avoid reporting a problem that had been fixed, but must have done it just before rG16266e63963ad6ee27ad21983a9366ab313dfd03 landed.

Nono, rG16266e63963ad6ee27ad21983a9366ab313dfd03 is the fix as a reaction to the bug you reported, your report wasn't a duplicate, sorry for confusion.

I'll fetch, rebuild, and test. Let you know if it didn't help.

Yes, please, thank you!

bjope added a comment.Jul 7 2020, 7:42 AM

Ah, maybe I was so occupied reducing the fault so I missed that there was another fix. I actually did take a look in github just to avoid reporting a problem that had been fixed, but must have done it just before rG16266e63963ad6ee27ad21983a9366ab313dfd03 landed.

Nono, rG16266e63963ad6ee27ad21983a9366ab313dfd03 is the fix as a reaction to the bug you reported, your report wasn't a duplicate, sorry for confusion.

Ah, thanks! (That was such a quick fix/response so I thought someone else had discovered the same problem.)

I'll fetch, rebuild, and test. Let you know if it didn't help.

Yes, please, thank you!

The fix works fine. So I don't know about more problems right now.

materi added a subscriber: materi.Aug 18 2020, 6:56 AM

We have seen some issues with missing symbols during linking that I think are caused by this patch. The origin of this is downstream fuzz testing.

The global variable aglobal is renamed:

$ cat global.ll
@aglobal = dso_local global i16 0, align 1
@b = dso_local local_unnamed_addr global i16 0, align 1

define dso_local void @c() local_unnamed_addr {
entry:
  %d.sroa.0.1.vec.extract = extractelement <4 x i16*> <i16* @aglobal, i16* @aglobal, i16* @aglobal, i16* @aglobal>, i32 1
  %0 = ptrtoint i16* %d.sroa.0.1.vec.extract to i16
  store i16 %0, i16* @b, align 1
  ret void
}

$ opt -scalarizer global.ll -S
; ModuleID = 'global.ll'
source_filename = "global.ll"

@d.sroa.0.1.vec.extract = dso_local global i16 0, align 1
@b = dso_local local_unnamed_addr global i16 0, align 1

define dso_local void @c() local_unnamed_addr {
entry:
  %0 = ptrtoint i16* @d.sroa.0.1.vec.extract to i16
  store i16 %0, i16* @b, align 1
  ret void
}

We have seen some issues with missing symbols during linking that I think are caused by this patch. The origin of this is downstream fuzz testing.

The global variable aglobal is renamed:

$ cat global.ll
@aglobal = dso_local global i16 0, align 1
@b = dso_local local_unnamed_addr global i16 0, align 1

define dso_local void @c() local_unnamed_addr {
entry:
  %d.sroa.0.1.vec.extract = extractelement <4 x i16*> <i16* @aglobal, i16* @aglobal, i16* @aglobal, i16* @aglobal>, i32 1
  %0 = ptrtoint i16* %d.sroa.0.1.vec.extract to i16
  store i16 %0, i16* @b, align 1
  ret void
}

$ opt -scalarizer global.ll -S
; ModuleID = 'global.ll'
source_filename = "global.ll"

@d.sroa.0.1.vec.extract = dso_local global i16 0, align 1
@b = dso_local local_unnamed_addr global i16 0, align 1

define dso_local void @c() local_unnamed_addr {
entry:
  %0 = ptrtoint i16* @d.sroa.0.1.vec.extract to i16
  store i16 %0, i16* @b, align 1
  ret void
}

I see that the global is renamed, https://godbolt.org/z/PM95se, it's not really intentional.
But i think something else is missing in this test - what's the failure? -verify passes

We have seen some issues with missing symbols during linking that I think are caused by this patch. The origin of this is downstream fuzz testing.

The global variable aglobal is renamed:

$ cat global.ll
@aglobal = dso_local global i16 0, align 1
@b = dso_local local_unnamed_addr global i16 0, align 1

define dso_local void @c() local_unnamed_addr {
entry:
  %d.sroa.0.1.vec.extract = extractelement <4 x i16*> <i16* @aglobal, i16* @aglobal, i16* @aglobal, i16* @aglobal>, i32 1
  %0 = ptrtoint i16* %d.sroa.0.1.vec.extract to i16
  store i16 %0, i16* @b, align 1
  ret void
}

$ opt -scalarizer global.ll -S
; ModuleID = 'global.ll'
source_filename = "global.ll"

@d.sroa.0.1.vec.extract = dso_local global i16 0, align 1
@b = dso_local local_unnamed_addr global i16 0, align 1

define dso_local void @c() local_unnamed_addr {
entry:
  %0 = ptrtoint i16* @d.sroa.0.1.vec.extract to i16
  store i16 %0, i16* @b, align 1
  ret void
}

I see that the global is renamed, https://godbolt.org/z/PM95se, it's not really intentional.
But i think something else is missing in this test - what's the failure? -verify passes

I don't think there is a verifier that points out this issue. But consider if there is another .ll file which has an external reference to the global:

@aglobal = external dso_local local_unnamed_addr global i16, align 1
define dso_local i16 @main() local_unnamed_addr #0 {
entry:
  %0 = load i16, i16* @aglobal, align 1
  ret i16 %0
}

In this case you get link error when @aglobal has been renamed in global.ll.

Proposed fix for the problem described by @materi : https://reviews.llvm.org/D86472