[Attributor] Fix an issue that an access is skipped by mistake #101862
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-transforms Author: Shilei Tian (shiltian) ChangesWhen we check if an access can be skipped, there is a case that an Full diff: https://github.com/llvm/llvm-project/pull/101862.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index cd31c4be1c1da..1a551faec6dff 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -1331,7 +1331,7 @@ struct AAPointerInfoImpl
// itself either.
bool Inserted = ExclusionSet.insert(&I).second;
- if (!FnReachabilityAA ||
+ if (FnReachabilityAA &&
!FnReachabilityAA->instructionCanReach(
A, *LeastDominatingWriteInst,
*Acc.getRemoteInst()->getFunction(), &ExclusionSet))
|
Well, the thing is, this can only be exposed in an Attributor use that doesn't have What I can do I guess is still to include the test case in this PR and the enabling of |
shiltian
force-pushed
the
users/shiltian/dont-skip-if-no-aa
branch
from
70d02a3 to
8c53a8b
Compare
|
ping |
Pre-commit the test that would break in the next patch in its working state? |
Hmm, I didn't follow. |
I'm not very familiar with these APIs, but is this something that would be easier to test with a C++ unit test? I see there are already some attributor unit tests. |
Yes, that's a good point. However, this issue requires a target where a stack variable is not accessible by other threads (effectively a GPU). I can't add target triple in the unit test because it will break a build if the target is not enabled. |
shiltian
force-pushed
the
users/shiltian/dont-skip-if-no-aa
branch
from
8c53a8b to
0dd6941
Compare
The tests kept being based on std::string instead of std::string_view to allow testing with older C++ dialects. Adds tests for: - LWG3112 system_error and filesystem_error constructors taking a string may not be able to meet their postconditions
shiltian
force-pushed
the
users/shiltian/dont-skip-if-no-aa
branch
from
0dd6941 to
fdc014a
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/3243 Here is the relevant piece of the build log for the reference: |
|
Hmm, I can't reproduce this issue on gfx1030 as well as gfx90a. |
Was a fluke: https://lab.llvm.org/buildbot/#/builders/73/builds/3244 |
When we check if an access can be skipped, there is a case that an
inter-procedural interference access exists after a dominant write. Currently we
rely on
AAInterFnReachabilityto tell if the access can be reachable. If it isnot, we can safely skip the access. However, it is based on an assumption that
the AA exists. It is possible that the AA doesn't exist. In this case, we can't
safely assume the acess can be skipped because we have to assume the access can
reach. This can happen when
AAInterFnReachabilityis not in the allowed AAlist when creating the attributor, such as AMDGPUAttributor.