-
Notifications
You must be signed in to change notification settings - Fork 13.4k
arm neon ldrb before st2 error lead to compare fail #64696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@llvm/issue-subscribers-backend-arm |
@llvm/issue-subscribers-backend-aarch64 |
opt-bisect points to MachineScheduler? I suspect something is going wrong with alias analysis. Changing the size of "temp" from 2 to 16 seems to work around the issue. |
Seem the size of Can you give some suggestions concretely? I'm new to llvm, I want do something, but not have direction. |
I location the pass which first lead to this problem
|
There is some code in AArch64TargetLowering::getTgtMemIntrinsic that tries to specify the info for what a target intrinsic will load/store (aarch64_neon_st2lane in this case). It looks like it is saying it is accessing a much larger vector than just 2 elements, and if that is larger than the underlying object then the aliasing analysis can start going wrong. |
gmi change from
to
in the code |
According to the debug info:
SU(2) should have an another Successors SU(6) like SU(4). But not. |
If alias analysis goes wrong, how can I debug alias analysis? I have no idea. 😬 |
The scheduler ends up calling MachineInstr::mayAlias, I think, to determine what edges are necessary. |
Yes, alias analysis goes wrong in this function. for So have good guide to debug alias analysis? 🤯 |
Disable basic aa is right. https://godbolt.org/z/rnoThYxKq Below is ST2i8 %7:qq, 6, %8:gpr64sp :: (store (s128) into %ir.temp)
the diff of 2 and 16 is the size of |
Ahead, I think this case is hit the https://godbolt.org/z/xW5s7s35q In the size of |
Right, that's basically the same conclusion as #64696 (comment) : the memory operand says the store writes to 16 bytes, but the object in question is only 4 bytes, so alias analysis concludes the store can't access the object. |
So, https://godbolt.org/z/4sfT3rzWr Thx your patient @efriedma-quic . So, What can we do to fix this problem? @davemgreen @efriedma-quic |
Are you willing to put together a patch to AArch64TargetLowering::getTgtMemIntrinsic that gets the memVT more correct for aarch64_neon_st2 (and st3/st4, and maybe the loads too)? With a testcase it sounds like a sensible fix. |
I want to try it, but I'm not so familiar with llvm. Can you give me some suggestions? |
There are some details on how to write patches and contribute in https://llvm.org/docs/Contributing.html if it is helpful, and some extra details on phabricator in https://llvm.org/docs/Phabricator.html#phabricator-reviews. |
@davemgreen Hi, can you review it? https://reviews.llvm.org/D158611 |
stx lane memory size set too big lead to alias analysis goes wrong. llvm#64696
/ cherry-pick db8f6c0 |
/cherry-pick db8f6c0 |
/branch llvm/llvm-project-release-prs/issue64696 |
StN lane memory size set too big lead to alias analysis goes wrong. Fixes llvm/llvm-project#64696 Differential Revision: https://reviews.llvm.org/D158611 (cherry picked from commit db8f6c009e5a17d304be7404e50eb20b2dd0c75b)
/pull-request llvm/llvm-project-release-prs#679 |
Not merged yet. |
StN lane memory size set too big lead to alias analysis goes wrong. Fixes llvm/llvm-project#64696 Differential Revision: https://reviews.llvm.org/D158611 (cherry picked from commit db8f6c009e5a17d304be7404e50eb20b2dd0c75b)
clang: https://godbolt.org/z/xW5s7s35q
gcc: https://godbolt.org/z/P5Wv7Y4zh
code
I see the asm
The error is caused because the
temp[1]
is loaded beforest2
, so the compare fail.I switch it like :
and recompile it, it's OK.
Anyone have idea to fix it?
The text was updated successfully, but these errors were encountered: