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

[Tobias Grosser via Phabricator via cfe-commits]

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()) {
   FuncAttrs.addAttribute(llvm::Attribute::ReadNone);
   FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
+  FuncAttrs.addAttribute(llvm::Attribute::Speculatable);
 } else if (TargetDecl->hasAttr()) {
   FuncAttrs.addAttribute(llvm::Attribute::ReadOnly);
   FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);


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_R

[PATCH] D41412: [libcxx] implement concat() and split()

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

Hi @timshen,

I am very interested in these patches. Any chance you can take up the 
upstreaming process again?

Best,
Tobias


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

https://reviews.llvm.org/D41412



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41412: [libcxx] implement concat() and split()

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

It seems this patch went through at least one review and the only open comment 
is the discussion if __builtin_shuffle should be placed in the configuration. 
From my perspective, both solutions are technically feasible. While it seems 
unlikely that gcc will gain this specific builtin, I can see @mclow.lists  
preferring to be consistent in keeping compiler-specific stuff in one file 
rather than having to evaluate for each extension the likeliness of this 
extension being adopted over time.

If you don't feel strongly about this change, I guess moving it to _config 
would address this last point and would require then only an ok, but no further 
discussions.

If you cannot reach @mclow.lists I am happy to either reach out or check for 
other reviewers.  Eric Fiselier seems very active at the moment.


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

https://reviews.llvm.org/D41412



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41412: [libcxx] implement concat() and split()

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

In D41412#1592152 , @timshen wrote:

> Tobias,
>
> I spoke to @EricWF who is willing to take a look at all these patches. 
> However, I don't know when exactly will the review starts.


Amazing. I am super interested in seeing this moving but am happy to give you 
some time to get started. ;-) Thanks a lot for picking this up!


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

https://reviews.llvm.org/D41412



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41412: [libcxx] implement concat() and split()

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

@EricWF thanks again for offering your help here. We are not in a rush here, 
but I wonder if you happen to have a rough estimate when you might have the 
time to look into these patches?


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

https://reviews.llvm.org/D41412



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41412: [libcxx] implement concat() and split()

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.
Herald added a subscriber: sanjoy.google.

@ericwf and @timshen, @Maxf and I are indeed very interested in seeing this 
library upstreamed soon. Over the last weeks, we developed a software prototype 
based on these bindings and already started to tune performance on clang/LLVM. 
As part of this, we looked into efficient vector code generation, started to 
implement where expressions, ... Currently, everything is very much prototype 
quality, but we would very much prefer to get things integrated into libcxx. 
For this, it's important to have a working and committed implementation, such 
that we can start submitting patches ourselves and can obtain feedback on these 
patches early on. Otherwise, we might take directions that won't really be 
compatible with libcxx. How can we best help with getting these patches 
upstream?


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

https://reviews.llvm.org/D41412



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138278: TableGen: honor LLVM_LINK_LLVM_DYLIB by default

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

This patch overcomes the issue of cl options being defined multiple times. That 
is great!

I just tried it on Windows on top of e7fb6c394f519d6e6812f1fbbff1571d5e51f5c4 
 with 
(msys2), as well as LLVM_LINK_LLVM_DYLIB, LLVM_BUILD_LLVM_DYLIB, and MLIR 
enabled. I get the following error:

  cmd.exe /C "cd . && C:\msys64\clang64\bin\clang++.exe 
-fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion 
-Wmisleading-indentation -Wctad-maybe-unsupported -ffunction-sections 
-fdata-sections -Werror=mismatched-tags -O3 -DNDEBUG -Wl,--stack,16777216
-Wl,--gc-sections 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/AttrOrTypeDefGen.cpp.obj
 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/AttrOrTypeFormatGen.cpp.obj
 tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DialectGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/DirectiveCommonGen.cpp.obj
 tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/EnumsGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/FormatGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/LLVMIRConversionGen.cpp.obj
 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/LLVMIRIntrinsicGen.cpp.obj
 tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/mlir-tblgen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpClass.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpDefinitionsGen.cpp.obj
 tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpDocGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpFormatGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpGenHelpers.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpInterfacesGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/OpPythonBindingGen.cpp.obj
 tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/PassCAPIGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/PassDocGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/PassGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/RewriterGen.cpp.obj 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/SPIRVUtilsGen.cpp.obj 
-o bin\mlir-tblgen.exe -Wl,--out-implib,lib\libmlir-tblgen.dll.a 
-Wl,--major-image-version,0,--minor-image-version,0  
lib/libMLIRSupportIndentedOstream.a  lib/libMLIRTblgenLib.a  
lib/libMLIRTableGen.a  lib/libLLVM-16git.dll.a  -lkernel32 -luser32 -lgdi32 
-lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 && cd ."
  ld.lld: error: undefined symbol: 
llvm::RecordKeeper::getAllDerivedDefinitionsIfDefined(llvm::StringRef) const
  >>> referenced by 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/AttrOrTypeDefGen.cpp.obj:(std::__1::__function::__func<$_2,
 std::__1::allocator<$_2>, bool (llvm::RecordKeeper const&, 
llvm::raw_ostream&)>::operator()(llvm::RecordKeeper const&, llvm::raw_ostream&))
  >>> referenced by 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/AttrOrTypeDefGen.cpp.obj:(std::__1::__function::__func<$_3,
 std::__1::allocator<$_3>, bool (llvm::RecordKeeper const&, 
llvm::raw_ostream&)>::operator()(llvm::RecordKeeper const&, llvm::raw_ostream&))
  >>> referenced by 
tools/mlir/tools/mlir-tblgen/CMakeFiles/mlir-tblgen.dir/AttrOrTypeDefGen.cpp.obj:(std::__1::__function::__func<$_4,
 std::__1::allocator<$_4>, bool (llvm::RecordKeeper const&, 
llvm::raw_ostream&)>::operator()(llvm::RecordKeeper const&, llvm::raw_ostream&))
  >>> referenced 8 more times


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138278

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138278: TableGen: honor LLVM_LINK_LLVM_DYLIB by default

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

I just checked and I get the same error on a linux system. @nhaehnle, did you 
try an mlir build with the previously mentioned cmake options?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138278

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138278: TableGen: honor LLVM_LINK_LLVM_DYLIB by default

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.

That is perfectly fine. I just wanted to let you know that we are still 
interested in this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138278

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138278: TableGen: honor LLVM_LINK_LLVM_DYLIB by default

[Tobias Grosser via Phabricator via cfe-commits]

grosser added a comment.
Herald added subscribers: bviyer, asb.

@nhaehnle, I wanted to check in briefly if you are getting the same error 
messages?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138278

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits