-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[BasicAA][TLI] Treat local-linkage globals or known environments as not aliasing errno #170290
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
base: main
Are you sure you want to change the base?
[BasicAA][TLI] Treat local-linkage globals or known environments as not aliasing errno #170290
Conversation
…s errno Errno cannot alias global variables with internal/private-linkage, neither can it alias external ones unless known to be errno. Likewise, a non-thread-local global cannot alias errno when known to be implemented as a thread-local variable.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
nikic
left a comment
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.
From some quick research, it seems like all the mainstream libc implementations (glibc, musl, bionic, newlib, msvc) define errno as some variant of *__errno(). So I wonder whether whether the name based check here really makes sense at all, and we shouldn't have just:
- Internal is never errno (also for unknown environment)
- If we know the environment (gnu, musl etc) then assume that errno never aliases globals.
| return AliasResult::NoAlias; | ||
|
|
||
| // A non-thread-local global cannot alias a thread-local errno. | ||
| if (!GV->isThreadLocal() && TLI.isErrnoThreadLocal()) |
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.
I wonder whether this logic shouldn't be inside mayBeErrnoGlobal?
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.
Sure thing. I wonder though if it is still reasonable to keep it separate and have IsErrnoThreadLocal.
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Antonio Frighetto (antoniofrighetto) ChangesErrno cannot alias global variables with internal/private-linkage, neither can it alias external ones unless known to be errno. Likewise, a non-thread-local global cannot alias errno when known to be implemented as a thread-local variable. Full diff: https://github.com/llvm/llvm-project/pull/170290.diff 6 Files Affected:
diff --git a/llvm/include/llvm/Analysis/TargetLibraryInfo.h b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
index 0f98af69f12c6..b9ccda022bf2f 100644
--- a/llvm/include/llvm/Analysis/TargetLibraryInfo.h
+++ b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
@@ -89,6 +89,8 @@ class TargetLibraryInfoImpl {
#include "llvm/Analysis/TargetLibraryInfo.inc"
bool ShouldExtI32Param, ShouldExtI32Return, ShouldSignExtI32Param, ShouldSignExtI32Return;
unsigned SizeOfInt;
+ bool IsErrnoFunctionCall;
+ bool IsErrnoThreadLocal;
enum AvailabilityState {
StandardName = 3, // (memset to all ones)
@@ -256,6 +258,8 @@ class TargetLibraryInfoImpl {
/// conventions.
LLVM_ABI static bool isCallingConvCCompatible(CallBase *CI);
LLVM_ABI static bool isCallingConvCCompatible(Function *Callee);
+
+ LLVM_ABI bool mayBeErrnoGlobal(const GlobalVariable *GV) const;
};
/// Provides information about what library functions are available for
@@ -601,6 +605,12 @@ class TargetLibraryInfo {
bool isKnownVectorFunctionInLibrary(StringRef F) const {
return this->isFunctionVectorizable(F);
}
+
+ /// Returns whether the name of the global variable is associated to the
+ /// representation of the errno storage. Returns true if could not determined.
+ bool mayBeErrnoGlobal(const GlobalVariable *GV) const {
+ return Impl->mayBeErrnoGlobal(GV);
+ }
};
/// Analysis pass providing the \c TargetLibraryInfo.
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index f812809e5e0b5..381bb285f1f79 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1870,8 +1870,21 @@ AliasResult BasicAAResult::aliasErrno(const MemoryLocation &Loc,
Loc.Size.getValue().getKnownMinValue() * 8 > TLI.getIntSize())
return AliasResult::NoAlias;
- if (isIdentifiedFunctionLocal(getUnderlyingObject(Loc.Ptr)))
+ const Value *Object = getUnderlyingObject(Loc.Ptr);
+ if (isIdentifiedFunctionLocal(Object))
return AliasResult::NoAlias;
+
+ if (auto *GV = dyn_cast<GlobalVariable>(Object)) {
+ // Errno cannot alias internal/private globals.
+ if (GV->hasLocalLinkage())
+ return AliasResult::NoAlias;
+
+ // Neither can it alias globals where environments define it as a function
+ // call, nor can a non-thread-local global alias a thread-local errno.
+ if (!TLI.mayBeErrnoGlobal(GV))
+ return AliasResult::NoAlias;
+ }
+
return AliasResult::MayAlias;
}
diff --git a/llvm/lib/Analysis/TargetLibraryInfo.cpp b/llvm/lib/Analysis/TargetLibraryInfo.cpp
index 51b1f5874bcb6..be000f6f9c845 100644
--- a/llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ b/llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -905,8 +905,26 @@ static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
initializeLibCalls(TLI, T, StandardNames, VecLib);
}
+static bool initializeIsErrnoThreadLocal(const Triple &T) {
+ // Assume errno has thread-local storage for non-baremetal environments.
+ // TODO: Could refine known OSes.
+ return T.isOSDarwin() || T.isOSFreeBSD() || T.isOSLinux() || T.isOSWindows();
+}
+
+static bool initializeIsErrnoFunctionCall(const Triple &T) {
+ // Assume errno is implemented as a function call on the following
+ // environments.
+ // TODO: Could refine known environments.
+ return T.isAndroid() || T.isGNUEnvironment() || T.isMusl() ||
+ T.getEnvironment() == Triple::LLVM ||
+ T.getEnvironment() == Triple::Mlibc ||
+ T.getEnvironment() == Triple::MSVC;
+}
+
TargetLibraryInfoImpl::TargetLibraryInfoImpl(const Triple &T,
- VectorLibrary VecLib) {
+ VectorLibrary VecLib)
+ : IsErrnoFunctionCall(initializeIsErrnoFunctionCall(T)),
+ IsErrnoThreadLocal(initializeIsErrnoThreadLocal(T)) {
// Default to everything being available.
memset(AvailableArray, -1, sizeof(AvailableArray));
@@ -918,7 +936,8 @@ TargetLibraryInfoImpl::TargetLibraryInfoImpl(const TargetLibraryInfoImpl &TLI)
ShouldExtI32Return(TLI.ShouldExtI32Return),
ShouldSignExtI32Param(TLI.ShouldSignExtI32Param),
ShouldSignExtI32Return(TLI.ShouldSignExtI32Return),
- SizeOfInt(TLI.SizeOfInt) {
+ SizeOfInt(TLI.SizeOfInt), IsErrnoFunctionCall(TLI.IsErrnoFunctionCall),
+ IsErrnoThreadLocal(TLI.IsErrnoThreadLocal) {
memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
VectorDescs = TLI.VectorDescs;
ScalarDescs = TLI.ScalarDescs;
@@ -930,7 +949,8 @@ TargetLibraryInfoImpl::TargetLibraryInfoImpl(TargetLibraryInfoImpl &&TLI)
ShouldExtI32Return(TLI.ShouldExtI32Return),
ShouldSignExtI32Param(TLI.ShouldSignExtI32Param),
ShouldSignExtI32Return(TLI.ShouldSignExtI32Return),
- SizeOfInt(TLI.SizeOfInt) {
+ SizeOfInt(TLI.SizeOfInt), IsErrnoFunctionCall(TLI.IsErrnoFunctionCall),
+ IsErrnoThreadLocal(TLI.IsErrnoThreadLocal) {
std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
AvailableArray);
VectorDescs = TLI.VectorDescs;
@@ -944,6 +964,8 @@ TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(const TargetLibraryInfoI
ShouldSignExtI32Param = TLI.ShouldSignExtI32Param;
ShouldSignExtI32Return = TLI.ShouldSignExtI32Return;
SizeOfInt = TLI.SizeOfInt;
+ IsErrnoFunctionCall = TLI.IsErrnoFunctionCall;
+ IsErrnoThreadLocal = TLI.IsErrnoThreadLocal;
memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
return *this;
}
@@ -955,6 +977,8 @@ TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(TargetLibraryInfoImpl &&
ShouldSignExtI32Param = TLI.ShouldSignExtI32Param;
ShouldSignExtI32Return = TLI.ShouldSignExtI32Return;
SizeOfInt = TLI.SizeOfInt;
+ IsErrnoFunctionCall = TLI.IsErrnoFunctionCall;
+ IsErrnoThreadLocal = TLI.IsErrnoThreadLocal;
std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
AvailableArray);
return *this;
@@ -1468,6 +1492,15 @@ unsigned TargetLibraryInfoImpl::getSizeTSize(const Module &M) const {
return M.getDataLayout().getIndexSizeInBits(/*AddressSpace=*/0);
}
+bool TargetLibraryInfoImpl::mayBeErrnoGlobal(const GlobalVariable *GV) const {
+ assert(GV && "Expecting existing GlobalVariable.");
+ if (IsErrnoFunctionCall)
+ return false;
+ if (!GV->isThreadLocal() && IsErrnoThreadLocal)
+ return false;
+ return true;
+}
+
TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass()
: ImmutablePass(ID), TLA(TargetLibraryInfoImpl(Triple())) {}
diff --git a/llvm/test/Transforms/InstCombine/may-alias-errno.ll b/llvm/test/Transforms/InstCombine/may-alias-errno.ll
index 40fab8024b362..a9ceae6903239 100644
--- a/llvm/test/Transforms/InstCombine/may-alias-errno.ll
+++ b/llvm/test/Transforms/InstCombine/may-alias-errno.ll
@@ -1,5 +1,5 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+; RUN: opt -S -passes=instcombine -mtriple=aarch64-linux-gnu < %s | FileCheck %s
; sinf clobbering errno, but %p cannot alias errno per C/C++ strict aliasing rules via TBAA.
; Can do constant store-to-load forwarding.
@@ -149,6 +149,43 @@ entry:
ret <vscale x 4 x i32> %v
}
+@internal_g = internal global i32 0
+
+; errno cannot alias an internal global variable, can do constant store-to-load forwarding.
+define i32 @does_not_alias_errno_internal_global(float %f) {
+; CHECK-LABEL: define i32 @does_not_alias_errno_internal_global(
+; CHECK-SAME: float [[F:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: store i32 42, ptr @internal_g, align 4
+; CHECK-NEXT: [[CALL:%.*]] = call float @sinf(float [[F]])
+; CHECK-NEXT: ret i32 42
+;
+entry:
+ store i32 42, ptr @internal_g, align 4
+ %call = call float @sinf(float %f)
+ %v = load i32, ptr @internal_g, align 4
+ ret i32 %v
+}
+
+@external_g = external global i32
+
+; errno cannot alias an external global variable in GNU environment,
+; can do constant store-to-load forwarding.
+define i32 @does_not_alias_errno_external_known_environment(float %f) {
+; CHECK-LABEL: define i32 @does_not_alias_errno_external_known_environment(
+; CHECK-SAME: float [[F:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: store i32 42, ptr @external_g, align 4
+; CHECK-NEXT: [[CALL:%.*]] = call float @sinf(float [[F]])
+; CHECK-NEXT: ret i32 42
+;
+entry:
+ store i32 42, ptr @external_g, align 4
+ %call = call float @sinf(float %f)
+ %v = load i32, ptr @external_g, align 4
+ ret i32 %v
+}
+
declare float @sinf(float) memory(errnomem: write)
declare float @read_errno(ptr) memory(argmem: write, errnomem: read)
declare void @escape(ptr %p)
diff --git a/llvm/test/Transforms/InstCombine/non-tls-does-not-alias-errno.ll b/llvm/test/Transforms/InstCombine/non-tls-does-not-alias-errno.ll
new file mode 100644
index 0000000000000..95178a9785af1
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/non-tls-does-not-alias-errno.ll
@@ -0,0 +1,23 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -mtriple=arm64-apple-macos -passes=instcombine < %s | FileCheck %s
+
+@errno = external global i32
+
+; non-tls errno global variable does not alias errno when known to be a thread-local variable,
+; can do constant store-to-load forwarding.
+define i32 @does_not_alias_errno_global_known_tls(float %f) {
+; CHECK-LABEL: define i32 @does_not_alias_errno_global_known_tls(
+; CHECK-SAME: float [[F:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: store i32 42, ptr @errno, align 4
+; CHECK-NEXT: [[CALL:%.*]] = call float @sinf(float [[F]])
+; CHECK-NEXT: ret i32 42
+;
+entry:
+ store i32 42, ptr @errno, align 4
+ %call = call float @sinf(float %f)
+ %v = load i32, ptr @errno, align 4
+ ret i32 %v
+}
+
+declare float @sinf(float) memory(errnomem: write)
diff --git a/llvm/unittests/Analysis/TargetLibraryInfoTest.cpp b/llvm/unittests/Analysis/TargetLibraryInfoTest.cpp
index 3029f10b1bb29..796187b6f3c4f 100644
--- a/llvm/unittests/Analysis/TargetLibraryInfoTest.cpp
+++ b/llvm/unittests/Analysis/TargetLibraryInfoTest.cpp
@@ -703,6 +703,28 @@ TEST_F(TargetLibraryInfoTest, ValidProto) {
}
}
+TEST_F(TargetLibraryInfoTest, IsErrnoGlobal) {
+ using TLII = TargetLibraryInfoImpl;
+ parseAssembly(R"(
+ @global = external global i32
+ )");
+ auto *GV = M->getNamedGlobal("global");
+
+ // Errno is not a global on the following environments.
+ EXPECT_FALSE(TLII(Triple("x86_64-unknown-linux-gnu")).mayBeErrnoGlobal(GV));
+ EXPECT_FALSE(TLII(Triple("x86_64-pc-windows-msvc")).mayBeErrnoGlobal(GV));
+
+ // Errno is thread-local on the following OSes.
+ EXPECT_FALSE(TLII(Triple("arm64-apple-macosx")).mayBeErrnoGlobal(GV));
+ EXPECT_FALSE(TLII(Triple("x86_64-unknown-freebsd")).mayBeErrnoGlobal(GV));
+
+ // Unknown.
+ EXPECT_TRUE(TLII(Triple("arm-none-eabi")).mayBeErrnoGlobal(GV));
+ EXPECT_TRUE(TLII(Triple("aarch64-unknown-unknown")).mayBeErrnoGlobal(GV));
+ GV->setThreadLocalMode(GlobalVariable::GeneralDynamicTLSModel);
+ EXPECT_TRUE(TLII(Triple("x86_64-pc-linux")).mayBeErrnoGlobal(GV));
+}
+
namespace {
/// Creates TLI for AArch64 and uses it to get the LibFunc names for the given
|
|
@efriedma-quic Agree on modern multithreaded systems I'm not sure if this is also the case for old libc version or alternative environments, though I wonder if we should keep |
|
I guess if everyone uses a call, that's fine... Unusual/baremetal environments usually end up using -ffreestanding for various reasons. So we don't really care what happens there, as long as we don't do errno checks with -ffreestanding. I'm not sure we actually suppress the checks currently, though. |
| if (isIdentifiedFunctionLocal(Object)) | ||
| return AliasResult::NoAlias; | ||
|
|
||
| if (auto *GV = dyn_cast<GlobalVariable>(Object)) { |
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.
Do we need additional logic for GlobalAlias? Or has it been handled by other places?
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.
Don't believe so - if a GlobalAlias turns out to be a GlobalVariable, getUnderlyingObject() should tell us that.
|
Updated by dropping IsErrnoThreadLocal.
I believe the only way to infer -ffreestanding is from the target triple. From https://clang.llvm.org/docs/CrossCompilation.html:
Triple.cpp also considers none as environment to model freestanding: llvm-project/llvm/lib/TargetParser/Triple.cpp Lines 1435 to 1438 in 7ac2900
Not doing any assumptions on 1) unknown environments unless Darwin 2) "none" OS. |
Can you not just check for the |
Done, thanks, hopefully should be on track now. |
| if ((ErrnoMR | Result) != Result) { | ||
| if (AAQI.AAR.aliasErrno(Loc, Call->getModule()) != AliasResult::NoAlias) { | ||
| // Do not make any assumptions about errno on freestanding environments. | ||
| bool IsFreestanding = Call->getFunction()->hasFnAttribute("no-builtins"); |
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.
I don't think this check belongs here. I'd expect freestanding to only be relevant for isErrnoFunctionCall.
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 rework of aliasErrno signatures sort of prevented me from checking it closer to isErrnoFunctionCall, though agree it's best suitable there.
Errno cannot alias global variables with internal/private-linkage, neither can it aliases globals on known environments, where errno is known to be defined as a function call.