[PATCH] D33774: [CodeGen] Make __attribute__(const) calls speculatable

[Tobias Grosser via Phabricator]

grosser created this revision.
Herald added subscribers: javed.absar, wdng.

Today 'const' functions are only marked 'readnone' and 'nounwind', but lack the
'speculatable' attribute for historic reasons. As 'const' functions are known to
not have any side effects and are intended to enable loop optimizations, they
are indeed 'speculatable' and consequently should be marked as such.

Some history: Back before r44273 (long time ago) readnone was indeed called
const and LLVM was assuming that readnone functions do not contain infinite
loops and can be hoisted. This worked at the beginning, but LLVM learned over
time to infer the readnone attribute from function definitions. As a result,
infinite functions that do not touch memory were marked as readnone, incorrectly
also stating that they are free of infinite loops, because different LLVM passes
still assumed a one-to-one correspondence between '__attribute(const)' and
LLVM's readnone attribute. Over time, we learned that 'readnone' must not imply
absence of infinite loops and other side effects to allow us to derive this
attribute automatically. Hence, the definition of readnone was changed to not
give information about the termination of functions. To still provide
information about side effects outside of memory effects LLVM recently learned
about speculatable function attributes: (https://reviews.llvm.org/D20116)

With 'speculatable' now available, we can pass information about the absence
of non-memory side effects to LLVM-IR.

This idea was taken from earlier discussions where for example Chris suggested
this solution:

"This really only matters when the compiler is able to infer readnone/readonly,
which typically doesn't include cases with indirect calls. Per #2, I think it
could be handled by making the GCC-style pure/const attributes imply both
readonly/readnone *and* halting. :


https://reviews.llvm.org/D33774

Files:
lib/CodeGen/CGCall.cpp
test/CodeGen/function-attributes.c
test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp


Index: test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
===================================================================
--- test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
+++ test/CodeGenCXX/2009-05-04-PureConstNounwind.cpp
@@ -5,18 +5,18 @@

// CHECK: define i32 @_Z1fv() [[TF:#[0-9]+]] {
int f(void) {
- // CHECK: call i32 @_Z1cv() [[NUW_RN_CALL:#[0-9]+]]
+ // CHECK: call i32 @_Z1cv() [[NUW_RN_SP_CALL:#[0-9]+]]
// CHECK: call i32 @_Z1pv() [[NUW_RO_CALL:#[0-9]+]]
return c() + p() + t();
}

-// CHECK: declare i32 @_Z1cv() [[NUW_RN:#[0-9]+]]
+// CHECK: declare i32 @_Z1cv() [[NUW_RN_SP:#[0-9]+]]
// CHECK: declare i32 @_Z1pv() [[NUW_RO:#[0-9]+]]
// CHECK: declare i32 @_Z1tv() [[TF2:#[0-9]+]]

// CHECK: attributes [[TF]] = { {{.*}} }
-// CHECK: attributes [[NUW_RN]] = { nounwind readnone{{.*}} }
+// CHECK: attributes [[NUW_RN_SP]] = { nounwind readnone speculatable{{.*}} }
// CHECK: attributes [[NUW_RO]] = { nounwind readonly{{.*}} }
// CHECK: attributes [[TF2]] = { {{.*}} }
-// CHECK: attributes [[NUW_RN_CALL]] = { nounwind readnone }
+// CHECK: attributes [[NUW_RN_SP_CALL]] = { nounwind readnone speculatable }
// CHECK: attributes [[NUW_RO_CALL]] = { nounwind readonly }
Index: test/CodeGen/function-attributes.c
===================================================================
--- test/CodeGen/function-attributes.c
+++ test/CodeGen/function-attributes.c
@@ -44,7 +44,7 @@

// FIXME: We should be setting nounwind on calls.
// CHECK: call i32 @f10_t()
-// CHECK: [[NUW_RN:#[0-9]+]]
+// CHECK: [[NUW_RN_SP:#[0-9]+]]
// CHECK: {
int __attribute__((const)) f10_t(void);
int f10(void) { return f10_t(); }
@@ -114,5 +114,5 @@
// CHECK: attributes [[ALIGN]] = { nounwind optsize alignstack=16{{.*}} }
// CHECK: attributes [[RT]] = { nounwind optsize returns_twice{{.*}} }
// CHECK: attributes [[NR]] = { noreturn optsize }
-// CHECK: attributes [[NUW_RN]] = { nounwind optsize readnone }
+// CHECK: attributes [[NUW_RN_SP]] = { nounwind optsize readnone speculatable }
// CHECK: attributes [[RT_CALL]] = { optsize returns_twice }
Index: lib/CodeGen/CGCall.cpp
===================================================================
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1804,6 +1804,7 @@
if (TargetDecl->hasAttr<ConstAttr>()) {
FuncAttrs.addAttribute(llvm::Attribute::ReadNone);
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
+ FuncAttrs.addAttribute(llvm::Attribute::Speculatable);
} else if (TargetDecl->hasAttr<PureAttr>()) {
FuncAttrs.addAttribute(llvm::Attribute::ReadOnly);
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);

[Michael Kruse via Phabricator]

Meinersbur added a comment.

Definition of `__attibute__((const))` from https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

> Many functions do not examine any values except their arguments, and have no effects except the return value. Basically this is just slightly more strict class than the pure attribute below, since function is not allowed to read global memory.
> Note that a function that has pointer arguments and examines the data pointed to must not be declared const. Likewise, a function that calls a non-const function usually must not be const. It does not make sense for a const function to return void.

Definition of `speculatable` from http://llvm.org/docs/LangRef.html#function-attributes

> This function attribute indicates that the function does not have any effects besides calculating its result and does not have undefined behavior.


https://reviews.llvm.org/D33774

[Eli Friedman via Phabricator]

efriedma added a comment.

Consider something like this:

define i32 @div(i32 %x, i32 %y) {
entry:
%div = sdiv i32 %x, %y
ret i32 %div
}

We can mark this function readnone, but not speculatable: it doesn't read or write memory, but could exhibit undefined behavior.

Consider another function:

@x = common local_unnamed_addr global i32 0, align 4
define i32 @get_x() {
entry:
%0 = load i32, i32* @x, align 4, !tbaa !2
ret i32 %0
}

We can mark this function speculatable, but not readnone: it doesn't exhibit undefined behavior or have side-effects, but it does read memory.

Given that, it seems a little dubious to translate `__attribute__((const))` into `speculatable`.


https://reviews.llvm.org/D33774

[Sanjoy Das via Phabricator]

sanjoy resigned from this revision.
Herald added subscribers: arjunp, jdoerfert, bollu.

CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33774/new/

https://reviews.llvm.org/D33774

[Michael Kruse via Phabricator]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.
Herald added a project: All.

`speculable` implies no undefined behavior (so it can even be speculatively executed with arguments when the source code would not, but its result eventually discarded), but `const` does not.