Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[SROA]
isVectorPromotionViable()
: memory intrinsics operate on vect…
…ors of bytes Now, there's a big caveat here - these bytes are abstract bytes, not the i8 we have in LLVM, so strictly speaking this is not exactly legal, see e.g. AliveToolkit/alive2#860 ^ the "bytes" "could" have been a pointer, and loading it as an integer inserts an implicit ptrtoint. But at the same time, InstCombine's `InstCombinerImpl::SimplifyAnyMemTransfer()` would expand a memtransfer of 1/2/4/8 bytes into integer-typed load+store, so this isn't exactly a new problem. Note that in memory, poison is byte-wise, so we really can't widen elements, but SROA seems to be inconsistent here. Fixes #59116.
- Loading branch information
Showing
17 changed files
with
216 additions
and
268 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit caused two regressions for me. One case where compilation triggers failed asserts, one case where compilation time ballooned from <2 seconds to >100 seconds.
The failed assert is reproducible with a reduced testcase in https://martin.st/temp/sroa-assert.c like this:
The full original unreduced testcase is available in https://martin.st/temp/placebo-preproc.c - reproducible in the same way, but this case requires building with
-target aarch64-w64-mingw32
.The compilation time regression can be reproduced with https://martin.st/temp/h264_slice-preproc.c, with
clang -target aarch64-linux-gnu -w -c h264_slice-preproc.c -O2
.cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted this for now in 5cfc22c. I'm still seeing crashes after 655d857.
Let me know if you need another test case.
cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revert.
It sounds like @d0k's assertion is different, so it may be good to have a repro for it.
cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit also broke PPC64 2-stage builders (confirmed as fixed by the revert), with an apparent miscompile of llvm-tblgen causing them to produce invalid source-code.
https://lab.llvm.org/buildbot/#/builders/121/builds/25465
https://lab.llvm.org/buildbot/#/builders/36/builds/27549
cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also had issues on one of our 32 bit bots https://lab.llvm.org/buildbot/#/builders/182/builds/4502. Instead of asserts we just got timeouts, maybe if I'd left it it would have eventually asserted. Was fixed by the revert.
For whatever reason it got stuck on
lib/Target/AMDGPU/CMakeFiles/LLVMAMDGPUCodeGen.dir/SIInsertWaitcnts.cpp
. If this turns out to be a different issue I can get you a reproducer for it.cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took creduce 5h to produce it, but here's another reproducer: https://gist.github.com/d0k/2bf4e6a4e5642d053c5d84fc66a33c5b
cf624b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, PPC miscompile is still there, re-reverted. Help wanted.