[PATCH] D157188: [clang-tidy] Add bugprone-new-bool-conversion

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D157188#4563143 , @Eugene.Zelenko 
wrote:

> In D157188#4563139 , @PiotrZSL 
> wrote:
>
>> In D157188#4563105 , 
>> @Eugene.Zelenko wrote:
>>
>>> Shouldn't C-style allocation be checked too? Same for custom allocators via 
>>> configuration option.
>>
>> In theory it could, but in such case name of check would need to change...
>
> Something like `bugprone-resource-bool-conversion` should be generic enough.

Maybe `bugprone-allocation-bool-conversion`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157188

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

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0198d76e1e76: [Bazel] Get `//clang` building on Windows with 
clang-cl. (authored by chandlerc).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D112399?vs=383634=383951#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112399

Files:
  utils/bazel/.bazelrc
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
  utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
  utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
  utils/bazel/llvm-project-overlay/llvm/config.bzl
  utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h

Index: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
===
--- utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
+++ utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
@@ -352,10 +352,10 @@
 #define HAVE_STD_IS_TRIVIALLY_COPYABLE 1
 
 /* Define to a function implementing stricmp */
-/* stricmp defined in Bazel */
+/* stricmp defined conditionally below. */
 
 /* Define to a function implementing strdup */
-/* strdup defined in Bazel */
+/* strdup defined conditionally below. */
 
 /* Whether GlobalISel rule coverage is being collected */
 #define LLVM_GISEL_COV_ENABLED 0
@@ -368,4 +368,17 @@
 
 /* HAVE_PROC_PID_RUSAGE defined in Bazel */
 
+/* Directly provide definitions here behind platform preprocessor definitions.
+ * The preprocessor conditions are sufficient to handle all of the configuration
+ * on platforms targeted by Bazel, and defining these here more faithfully
+ * matches how the users of this header expect things to work with CMake.
+ * FIXME: We should consider moving other platform defines to use this technique
+ * as well.
+ */
+
+#ifdef _WIN32
+#define stricmp _stricmp
+#define strdup _strdup
+#endif
+
 #endif
Index: utils/bazel/llvm-project-overlay/llvm/config.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/config.bzl
+++ utils/bazel/llvm-project-overlay/llvm/config.bzl
@@ -57,9 +57,15 @@
 ]
 
 win32_defines = [
-# MSVC specific
-"stricmp=_stricmp",
-"strdup=_strdup",
+# Windows system library specific defines.
+"_CRT_SECURE_NO_DEPRECATE",
+"_CRT_SECURE_NO_WARNINGS",
+"_CRT_NONSTDC_NO_DEPRECATE",
+"_CRT_NONSTDC_NO_WARNINGS",
+"_SCL_SECURE_NO_DEPRECATE",
+"_SCL_SECURE_NO_WARNINGS",
+"UNICODE",
+"_UNICODE",
 
 # LLVM features
 r'LTDL_SHLIB_EXT=\".dll\"',
Index: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
+++ utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
@@ -4,51 +4,72 @@
 
 """A macro to produce a loadable plugin library for the target OS.
 
-This macro produces a `cc_binary` rule with the name `name + "_impl"`. It
-forces the rule to statically link in its dependencies but to be linked as a
-shared "plugin" library. It then creates binary aliases to `.so`, `.dylib`
-,and `.dll` suffixed names for use on various platforms and selects between
-these into a filegroup with the exact name passed to the macro.
+This macro produces a set of platform-specific `cc_binary` rules, by appending
+the platform suffix (`.dll`, `.dylib`, or `.so`) to the provided `name`. It then
+connects these to a `cc_import` rule with `name` exactly and `hdrs` that can be
+used by other Bazel rules to depend on the plugin library.
+
+The `srcs` attribute for the `cc_binary` rules is `srcs + hdrs`. Other explicit
+arguments are passed to all of the rules where they apply, and can be used to
+configure generic aspects of all generated rules such as `testonly`. Lastly,
+`kwargs` is expanded into all the `cc_binary` rules.
 """
 
-load("@rules_cc//cc:defs.bzl", "cc_binary")
-load(":binary_alias.bzl", "binary_alias")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library")
 
-def cc_plugin_library(name, **kwargs):
+def cc_plugin_library(name, srcs, hdrs, include_prefix = None, strip_include_prefix = None, alwayslink = False, features = [], tags = [], testonly = False, **kwargs):
 # Neither the name of the plugin binary nor tags on whether it is built are
-# configurable. Instead, we build a `cc_binary` that implements the plugin
-# library using a `_impl` suffix. Bazel will use appropriate flags to cause
-# this file to be a plugin library regardless of its name. We then create
-# binary aliases in the different possible platform names, and select
-# between these different names into a filegroup. The macro's name becomes
-  

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1828
 ],
-copts = [
-"-Wno-uninitialized",
-],
+copts = select({
+"@bazel_tools//src/conditions:windows": [],

rnk wrote:
> Enabling warnings is good, but what made this change necessary? The `-Wno-` 
> flag should have worked with clang-cl. If this change isn't necessary, maybe 
> go ahead and delete this copt in a separate commit.
Restored.


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

https://reviews.llvm.org/D112399

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

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D112883#3101685 , @GMNGeoffrey 
wrote:

>> Is this breaking actual users? I wouldn't expect it to break unless they 
>> depend on this target.
>
> I don't know :-) I just know that people have sent me patches to remove the 
> repository name dependence. I think 
> https://github.com/llvm/llvm-project/blob/d1fdd745d510f40d8741d44ce39f5ae24ee7f91a/utils/bazel/llvm-project-overlay/clang/BUILD.bazel#L1460
>  is the only previous instance in clang. That gets used only in "frontend". 
> I'm not sure if there are people using "ast" but not "frontend"

I think users of `ast` that don't use `frontend` would be *extremely* rare. I'd 
like to see if this works for folks for now?

The other approach we could explore is removing the reliance on file-relative 
inclusion of generated code completely which I think is the real sustainable 
answer here. CMake only causes this to work for pretty questionable reasons 
as-is. But it's a lot of work, and so I'd like to understand if we have to 
before we do.

Does that seem reasonable to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112883

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

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Thanks, making suggested changes and then landing!




Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:364
+# Clang-specific define on non-Windows platforms.
+"CLANG_HAVE_RLIMITS=1",
+],

GMNGeoffrey wrote:
> Seems like this should be in clang/config.h?
Ah, yeah. Might as well.



Comment at: utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel:485
+"@bazel_tools//src/conditions:windows": [
+# Need to disable the VFS tests that don't use Windows friendly 
paths.
+"--gtest_filter=-*VirtualFileOverlay*",

GMNGeoffrey wrote:
> Probably good to note here that these are also disabled in the CMake build
Good idea, will do.



Comment at: 
utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h:375
+ * matches how the users of this header expect things to work with CMake.
+ * FIXME: We should consider moving other platform defines to use this 
technique
+ * as well.

GMNGeoffrey wrote:
> Oh nice, yeah this seems like a reasonable approach. I can't believe we 
> didn't think of this before... The less Bazel goop the better. We're still 
> going to need some way to deal with the things that are more complicated than 
> just platform and some way to handle keeping this up to date. That might push 
> us back to doing it with Bazel. IDK
Well, *all* of Abseil's configuration is now done this way I think... so I've 
some optimism. Anyways, I'll send subsequent patches when I get some time.


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

https://reviews.llvm.org/D112399

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

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D112883#3101645 , @GMNGeoffrey 
wrote:

> :-( Did you try the separate library with `strip_include_prefix`?

I can add a separate library to do that.

It would still expose all consumers to `Opcodes.inc` instead of only exposing 
*this* library to it. If we can afford to have the dependency on the repository 
name, I feel like the current solution is actually a cleaner workaround. But if 
we genuinely cannot, then we could revisit this.

However, I thought with Bazel having a consistent repo name was important to 
allow cross-repo dependencies to reliably resolve, and so maybe this isn't that 
bad of a thing to rely on?




Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:711
+# headers such as `CXXABI.h`.
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST",
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST/Interp",

GMNGeoffrey wrote:
> This breaks anyone who calls the repository something other than 
> "llvm-project". I think we have real users for whom that is the case
This isn't the only place where we assume that in Clang's build at least. =[

Is this breaking actual users? I wouldn't expect it to break unless they depend 
on this target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112883

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

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd1fdd745d510: Re-introduce `copts` hacks for lib/AST 
includes. (authored by chandlerc).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112883

Files:
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel


Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -691,14 +691,25 @@
 "lib/AST/*.h",
 "lib/AST/Interp/*.cpp",
 "lib/AST/Interp/*.h",
-]),
+]) + [
+"lib/AST/AttrDocTable.inc",
+"lib/AST/Interp/Opcodes.inc",
+],
 hdrs = glob([
 "include/clang/AST/*.h",
 ]),
-includes = [
-"include",
-"lib/AST",
-"lib/AST/Interp",
+copts = [
+# FIXME: This is necessary to allow "file relative" include paths from
+# non-generated `srcs` to find generated `srcs` above. Bazel should
+# either make this work automatically by creating a unified tree of
+# `srcs` or at least provide a `local_includes` that has the path
+# translation logic of `includes` but is only used locally (similar to
+# `local_defines` vs. `defines`). Until one of those lands, this is the
+# least bad approach. Using `includes` is *specifically* problematic 
for
+# this library because it contains files that collide easily with 
system
+# headers such as `CXXABI.h`.
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST",
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST/Interp",
 ],
 textual_hdrs = [
 "include/clang/AST/AttrImpl.inc",
@@ -715,8 +726,6 @@
 "include/clang/AST/DeclNodes.inc",
 "include/clang/AST/StmtDataCollectors.inc",
 "include/clang/AST/StmtNodes.inc",
-"lib/AST/AttrDocTable.inc",
-"lib/AST/Interp/Opcodes.inc",
 ] + glob([
 "include/clang/AST/*.def",
 ]),


Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -691,14 +691,25 @@
 "lib/AST/*.h",
 "lib/AST/Interp/*.cpp",
 "lib/AST/Interp/*.h",
-]),
+]) + [
+"lib/AST/AttrDocTable.inc",
+"lib/AST/Interp/Opcodes.inc",
+],
 hdrs = glob([
 "include/clang/AST/*.h",
 ]),
-includes = [
-"include",
-"lib/AST",
-"lib/AST/Interp",
+copts = [
+# FIXME: This is necessary to allow "file relative" include paths from
+# non-generated `srcs` to find generated `srcs` above. Bazel should
+# either make this work automatically by creating a unified tree of
+# `srcs` or at least provide a `local_includes` that has the path
+# translation logic of `includes` but is only used locally (similar to
+# `local_defines` vs. `defines`). Until one of those lands, this is the
+# least bad approach. Using `includes` is *specifically* problematic for
+# this library because it contains files that collide easily with system
+# headers such as `CXXABI.h`.
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST",
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST/Interp",
 ],
 textual_hdrs = [
 "include/clang/AST/AttrImpl.inc",
@@ -715,8 +726,6 @@
 "include/clang/AST/DeclNodes.inc",
 "include/clang/AST/StmtDataCollectors.inc",
 "include/clang/AST/StmtNodes.inc",
-"lib/AST/AttrDocTable.inc",
-"lib/AST/Interp/Opcodes.inc",
 ] + glob([
 "include/clang/AST/*.def",
 ]),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
chandlerc added reviewers: GMNGeoffrey, dblaikie.
Herald added a subscriber: mcrosier.
chandlerc requested review of this revision.

Sadly, these are necessary AFAICT. There is a file `lib/AST/CXXABI.h`.
On case insensitive file systems like macOS this will collide with
`cxxabi.h` on the system if we use the `includes` trick to allow
file-relative `#include` of generated files.

I've tested this on both Linux and Windows to make sure it remains
reasonably portable.


https://reviews.llvm.org/D112883

Files:
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel


Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -691,14 +691,25 @@
 "lib/AST/*.h",
 "lib/AST/Interp/*.cpp",
 "lib/AST/Interp/*.h",
-]),
+]) + [
+"lib/AST/AttrDocTable.inc",
+"lib/AST/Interp/Opcodes.inc",
+],
 hdrs = glob([
 "include/clang/AST/*.h",
 ]),
-includes = [
-"include",
-"lib/AST",
-"lib/AST/Interp",
+copts = [
+# FIXME: This is necessary to allow "file relative" include paths from
+# non-generated `srcs` to find generated `srcs` above. Bazel should
+# either make this work automatically by creating a unified tree of
+# `srcs` or at least provide a `local_includes` that has the path
+# translation logic of `includes` but is only used locally (similar to
+# `local_defines` vs. `defines`). Until one of those lands, this is the
+# least bad approach. Using `includes` is *specifically* problematic 
for
+# this library because it contains files that collide easily with 
system
+# headers such as `CXXABI.h`.
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST",
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST/Interp",
 ],
 textual_hdrs = [
 "include/clang/AST/AttrImpl.inc",
@@ -715,8 +726,6 @@
 "include/clang/AST/DeclNodes.inc",
 "include/clang/AST/StmtDataCollectors.inc",
 "include/clang/AST/StmtNodes.inc",
-"lib/AST/AttrDocTable.inc",
-"lib/AST/Interp/Opcodes.inc",
 ] + glob([
 "include/clang/AST/*.def",
 ]),


Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -691,14 +691,25 @@
 "lib/AST/*.h",
 "lib/AST/Interp/*.cpp",
 "lib/AST/Interp/*.h",
-]),
+]) + [
+"lib/AST/AttrDocTable.inc",
+"lib/AST/Interp/Opcodes.inc",
+],
 hdrs = glob([
 "include/clang/AST/*.h",
 ]),
-includes = [
-"include",
-"lib/AST",
-"lib/AST/Interp",
+copts = [
+# FIXME: This is necessary to allow "file relative" include paths from
+# non-generated `srcs` to find generated `srcs` above. Bazel should
+# either make this work automatically by creating a unified tree of
+# `srcs` or at least provide a `local_includes` that has the path
+# translation logic of `includes` but is only used locally (similar to
+# `local_defines` vs. `defines`). Until one of those lands, this is the
+# least bad approach. Using `includes` is *specifically* problematic for
+# this library because it contains files that collide easily with system
+# headers such as `CXXABI.h`.
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST",
+"-I$(GENDIR)/external/llvm-project/clang/lib/AST/Interp",
 ],
 textual_hdrs = [
 "include/clang/AST/AttrImpl.inc",
@@ -715,8 +726,6 @@
 "include/clang/AST/DeclNodes.inc",
 "include/clang/AST/StmtDataCollectors.inc",
 "include/clang/AST/StmtNodes.inc",
-"lib/AST/AttrDocTable.inc",
-"lib/AST/Interp/Opcodes.inc",
 ] + glob([
 "include/clang/AST/*.def",
 ]),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested review of this revision.
chandlerc added a comment.

PTAL, and thanks for feedback so far!




Comment at: clang/include/clang/Basic/Builtins.def:1059
+#undef strcasecmp
+#undef strncasecmp
+

rnk wrote:
> thakis wrote:
> > GMNGeoffrey wrote:
> > > chandlerc wrote:
> > > > thakis wrote:
> > > > > Why do we need this with bazel but not with other windows builds?
> > > > I don't know how this never was hit by other builders. I don't usually 
> > > > develop on Windows so I have very little experience with different 
> > > > builds there.
> > > > 
> > > > My guess is that it is the particular way that Bazel+clang-cl compile 
> > > > on Windows causes sligthtly more to be transitively included with 
> > > > `#include ` and that grows the number of functions hit with 
> > > > this. I thought about trying to "fix" that, but the existing `#undef` 
> > > > lines made me think it wouldn't be completely successful.
> > > > 
> > > > Are you worried about the change? Looking for a different fix?
> > > Yeah I want to note that my review is basically only for the Bazel stuff. 
> > > This looked fine to me based on the existing `undef`s, but mostly 
> > > trusting Chandler to determine that this is OK and isn't in the territory 
> > > of the Bazel build requiring unsavory things in the core project.
> > Worried is a strong word :) It just feels off. The redundant build system 
> > config changes shouldn't need changes to LLVM's code itself imho. If 
> > there's some difference in build behavior here, it feels like we should fix 
> > that on the build config side of things, else we'll have weird one-off 
> > fixes like this in other places potentially. I feel we should at least 
> > understand what's going on.
> > 
> > (This isn't a terribly strong opinion fwiw.)
> Does Bazel build clang with modules? That could lead to windows.h 
> proliferation.
> 
> If you are motivated to investigate, you can re-run the failing compile 
> command with /showIncludes to work out where the windows.h include comes in. 
> Take the corresponding object file, build it with cmake ninja, extract the 
> compile command there, run it with showincludes, and compare the output.
I figured this all out. It was right in front of me. New patch fixes.



Comment at: utils/bazel/.bazelrc:113-116
 # Use Clang's internal warning flags instead of the ones that sometimes map
 # through to MSVC's flags.
 build:clang-cl --copt=/clang:-Wall --host_copt=/clang:-Wall
 build:clang-cl --copt=/clang:-Werror --host_copt=/clang:-Werror

rnk wrote:
> It is true that MSVC's -Wall is Clang's -Weverything, so this needs a prefix. 
> -Werror could be -WX, but it's just cosmetic.
Yeah, I just figured it was more readable in this section to consistently use 
`/clang:-W`.



Comment at: utils/bazel/.bazelrc:125
+# Disable some warnings hit even with `clang-cl` in Clang's own code.
+build:clang-cl --copt=/clang:-Wno-inconsistent-dllimport 
--host_copt=/clang:-Wno-inconsistent-dllimport
+build:clang-cl --cxxopt=/clang:-Wno-c++11-narrowing 
--host_cxxopt=/clang:-Wno-c++11-narrowing

rnk wrote:
> You shouldn't need to prefix warning flags with `/clang:`, those are 
> supported out of the box.
Again, this just seemed more consistent. I can change if you think the other is 
better though.



Comment at: 
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h:81
 /* The LLVM product name and version */
 #define BACKEND_PACKAGE_STRING "LLVM 12.0.0git"
 

GMNGeoffrey wrote:
> rnk wrote:
> > Unrelated to your change, but is this stale?
> Yes and we don't currently have a good way to keep it up to date. The overall 
> problem of how to make sure we keep up to date with CMake configure knobs is 
> unsolved. I wonder if we could run CMake in some limited capacity, but that's 
> a whole can of worms...
I'll just commit an update to this separately. I assume that doesn't need 
separate review? ;]


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

https://reviews.llvm.org/D112399

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

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 383634.
chandlerc marked 5 inline comments as done.
chandlerc edited the summary of this revision.
chandlerc added a reviewer: rnk.
chandlerc added a comment.

Major update to better fix some of the issues here. No longer requires any 
changes outside of Bazel.


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

https://reviews.llvm.org/D112399

Files:
  utils/bazel/.bazelrc
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
  utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
  utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
  utils/bazel/llvm-project-overlay/llvm/config.bzl
  utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h

Index: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
===
--- utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
+++ utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
@@ -352,10 +352,10 @@
 #define HAVE_STD_IS_TRIVIALLY_COPYABLE 1
 
 /* Define to a function implementing stricmp */
-/* stricmp defined in Bazel */
+/* stricmp defined conditionally below. */
 
 /* Define to a function implementing strdup */
-/* strdup defined in Bazel */
+/* strdup defined conditionally below. */
 
 /* Whether GlobalISel rule coverage is being collected */
 #define LLVM_GISEL_COV_ENABLED 0
@@ -368,4 +368,17 @@
 
 /* HAVE_PROC_PID_RUSAGE defined in Bazel */
 
+/* Directly provide definitions here behind platform preprocessor definitions.
+ * The preprocessor conditions are sufficient to handle all of the configuration
+ * on platforms targeted by Bazel, and defining these here more faithfully
+ * matches how the users of this header expect things to work with CMake.
+ * FIXME: We should consider moving other platform defines to use this technique
+ * as well.
+ */
+
+#ifdef _WIN32
+#define stricmp _stricmp
+#define strdup _strdup
+#endif
+
 #endif
Index: utils/bazel/llvm-project-overlay/llvm/config.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/config.bzl
+++ utils/bazel/llvm-project-overlay/llvm/config.bzl
@@ -57,9 +57,15 @@
 ]
 
 win32_defines = [
-# MSVC specific
-"stricmp=_stricmp",
-"strdup=_strdup",
+# Windows system library specific defines.
+"_CRT_SECURE_NO_DEPRECATE",
+"_CRT_SECURE_NO_WARNINGS",
+"_CRT_NONSTDC_NO_DEPRECATE",
+"_CRT_NONSTDC_NO_WARNINGS",
+"_SCL_SECURE_NO_DEPRECATE",
+"_SCL_SECURE_NO_WARNINGS",
+"UNICODE",
+"_UNICODE",
 
 # LLVM features
 r'LTDL_SHLIB_EXT=\".dll\"',
Index: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
+++ utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
@@ -4,51 +4,72 @@
 
 """A macro to produce a loadable plugin library for the target OS.
 
-This macro produces a `cc_binary` rule with the name `name + "_impl"`. It
-forces the rule to statically link in its dependencies but to be linked as a
-shared "plugin" library. It then creates binary aliases to `.so`, `.dylib`
-,and `.dll` suffixed names for use on various platforms and selects between
-these into a filegroup with the exact name passed to the macro.
+This macro produces a set of platform-specific `cc_binary` rules, by appending
+the platform suffix (`.dll`, `.dylib`, or `.so`) to the provided `name`. It then
+connects these to a `cc_import` rule with `name` exactly and `hdrs` that can be
+used by other Bazel rules to depend on the plugin library.
+
+The `srcs` attribute for the `cc_binary` rules is `srcs + hdrs`. Other explicit
+arguments are passed to all of the rules where they apply, and can be used to
+configure generic aspects of all generated rules such as `testonly`. Lastly,
+`kwargs` is expanded into all the `cc_binary` rules.
 """
 
-load("@rules_cc//cc:defs.bzl", "cc_binary")
-load(":binary_alias.bzl", "binary_alias")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library")
 
-def cc_plugin_library(name, **kwargs):
+def cc_plugin_library(name, srcs, hdrs, include_prefix = None, strip_include_prefix = None, alwayslink = False, features = [], tags = [], testonly = False, **kwargs):
 # Neither the name of the plugin binary nor tags on whether it is built are
-# configurable. Instead, we build a `cc_binary` that implements the plugin
-# library using a `_impl` suffix. Bazel will use appropriate flags to cause
-# this file to be a plugin library regardless of its name. We then create
-# binary aliases in the different possible platform names, and select
-# between these different names into a filegroup. The macro's name becomes
-# the filegroup name and it contains exactly one target that is the target
- 

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:1059
+#undef strcasecmp
+#undef strncasecmp
+

thakis wrote:
> Why do we need this with bazel but not with other windows builds?
I don't know how this never was hit by other builders. I don't usually develop 
on Windows so I have very little experience with different builds there.

My guess is that it is the particular way that Bazel+clang-cl compile on 
Windows causes sligthtly more to be transitively included with `#include 
` and that grows the number of functions hit with this. I thought 
about trying to "fix" that, but the existing `#undef` lines made me think it 
wouldn't be completely successful.

Are you worried about the change? Looking for a different fix?



Comment at: 
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h:78
 /* Define if we have sys/resource.h (rlimits) */
-#define CLANG_HAVE_RLIMITS 1
+/* CLANG_HAVE_RLIMITS defined in Bazel */
 

GMNGeoffrey wrote:
> Hmm I think we probably should have a change-detector for this config as well 
> as the LLVM ones
Yeah, but didn't want to try to add that in this change.

IMO, the better thing would be for this to go away as it is almost entirely 
redundant already But agin, not this change



Comment at: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl:33
+for impl_name in [dll_name, dylib_name, so_name]:
+cc_binary(
+name = impl_name,

GMNGeoffrey wrote:
> What about this makes `binary_alias` no longer work?
The `output_group` in the subsequent `filegroup`. It requires the actions in 
the sources to have a correctly named `output_group`, which `cc_binary` with 
`linkshared` does.

It's possible that I could just rename things enough by adding multiple layers 
of file groups and maybe some added "copy this file" rules... But I'm worried 
that wouldn't work well long term. For example, it seems reasonable for the 
`.lib` file to *name* the `.dll` file that should be loaded. And so if we build 
it with `cc_binary` that has the wrong name, I'm worried things won't work as 
expected. I have higher confidence in this arrangement where the `cc_binary` 
directly produces the desired name.

Something like that actually happened when I tried a different way of wiring 
this up that still used the `binary_alias` on Linux -- it ended up actually 
loading `liblibclang_impl.so` and not `libclang.so`. That was a different 
setup, so maaaybe I wouldn't hit that exact situation, but its the kind of 
thing that inclined me towards this model.


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

https://reviews.llvm.org/D112399

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

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
chandlerc added a reviewer: GMNGeoffrey.
Herald added a subscriber: mcrosier.
chandlerc requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

This required substantially more invasive changes I'm afraid.

First, there is an issue with a Clang header that isn't really robust on
Windows unless you get the includes *just* right. I've reworked it to be
a bit more defensive.

Second, I needed a different approach to get `libclang` working well.
This, IMO, improves things on all platforms. Now we build the plugin and
actually wrap it back up with `cc_import`. We have to use a collection
of manually tagged `cc_binary` rules to get the naming to work out the
right way, but this isn't too different from the prior approach. By
directly having a `cc_binary` rule for each platform spelling of
`libclang`, we can actually extract the interface library from it and
correctly depend on it with `cc_import`. I think the result now is much
closer to the intent and to the CMake build for libclang.

Last but not least, some tests needed disabling. This is actually
narrower than what CMake does. The issue isn't indicative of anything
serious -- the test just assumes Unix-style paths.


https://reviews.llvm.org/D112399

Files:
  clang/include/clang/Basic/Builtins.def
  utils/bazel/.bazelrc
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
  utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
  utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl

Index: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
+++ utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
@@ -4,51 +4,72 @@
 
 """A macro to produce a loadable plugin library for the target OS.
 
-This macro produces a `cc_binary` rule with the name `name + "_impl"`. It
-forces the rule to statically link in its dependencies but to be linked as a
-shared "plugin" library. It then creates binary aliases to `.so`, `.dylib`
-,and `.dll` suffixed names for use on various platforms and selects between
-these into a filegroup with the exact name passed to the macro.
+This macro produces a set of platform-specific `cc_binary` rules, by appending
+the platform suffix (`.dll`, `.dylib`, or `.so`) to the provided `name`. It then
+connects these to a `cc_import` rule with `name` exactly and `hdrs` that can be
+used by other Bazel rules to depend on the plugin library.
+
+The `srcs` attribute for the `cc_binary` rules is `srcs + hdrs`. Other explicit
+arguments are passed to all of the rules where they apply, and can be used to
+configure generic aspects of all generated rules such as `testonly`. Lastly,
+`kwargs` is expanded into all the `cc_binary` rules.
 """
 
-load("@rules_cc//cc:defs.bzl", "cc_binary")
-load(":binary_alias.bzl", "binary_alias")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library")
 
-def cc_plugin_library(name, **kwargs):
+def cc_plugin_library(name, srcs, hdrs, include_prefix = None, strip_include_prefix = None, alwayslink = False, features = [], tags = [], testonly = False, **kwargs):
 # Neither the name of the plugin binary nor tags on whether it is built are
-# configurable. Instead, we build a `cc_binary` that implements the plugin
-# library using a `_impl` suffix. Bazel will use appropriate flags to cause
-# this file to be a plugin library regardless of its name. We then create
-# binary aliases in the different possible platform names, and select
-# between these different names into a filegroup. The macro's name becomes
-# the filegroup name and it contains exactly one target that is the target
-# platform suffixed plugin library.
+# configurable. Instead, we build a `cc_binary` with each name and
+# selectively depend on them based on platform.
 #
 # All-in-all, this is a pretty poor workaround. I think this is part of the
 # Bazel issue: https://github.com/bazelbuild/bazel/issues/7538
-cc_binary(
-name = name + "_impl",
-linkshared = True,
-linkstatic = True,
-**kwargs
-)
-binary_alias(
-name = name + ".so",
-binary = ":" + name + "_impl",
-)
-binary_alias(
-name = name + ".dll",
-binary = ":" + name + "_impl",
-)
-binary_alias(
-name = name + ".dylib",
-binary = ":" + name + "_impl",
-)
+so_name = name + ".so"
+dll_name = name + ".dll"
+dylib_name = name + ".dylib"
+interface_output_name = name + "_interface_output"
+import_name = name + "_import"
+for impl_name in [dll_name, dylib_name, so_name]:
+cc_binary(
+name = impl_name,
+srcs = srcs + hdrs,
+linkshared = True,
+

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaaf7ffd4e1aa: Teach `-fsanitize=fuzzer` to respect `-static` 
and `-static-libstdc++` when… (authored by chandlerc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80488

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/fuzzer.c


Index: clang/test/Driver/fuzzer.c
===
--- clang/test/Driver/fuzzer.c
+++ clang/test/Driver/fuzzer.c
@@ -29,6 +29,23 @@
 // RUN: %clang -fsanitize=fuzzer -fsanitize-coverage=trace-pc %s -### 2>&1 | 
FileCheck --check-prefixes=CHECK-MSG %s
 // CHECK-MSG-NOT: argument unused during compilation
 
+// Check that we respect whether thes tandard library should be linked
+// statically.
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ 
%s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBSTDCXX-DYNAMIC %s
+// CHECK-LIBSTDCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBSTDCXX-DYNAMIC: -lstdc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ 
-static-libstdc++ %s -### 2>&1 | FileCheck 
--check-prefixes=CHECK-LIBSTDCXX-STATIC %s
+// CHECK-LIBSTDCXX-STATIC: "-Bstatic" "-lstdc++"
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ %s 
-### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DYNAMIC %s
+// CHECK-LIBCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBCXX-DYNAMIC: -lc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ 
-static-libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-STATIC 
%s
+// CHECK-LIBCXX-STATIC: "-Bstatic" "-lc++"
+
 int LLVMFuzzerTestOneInput(const char *Data, long Size) {
   return 0;
 }
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -833,8 +833,15 @@
 if (SanArgs.needsFuzzerInterceptors())
   addSanitizerRuntime(TC, Args, CmdArgs, "fuzzer_interceptors", false,
   true);
-if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx))
+if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx)) {
+  bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
+ !Args.hasArg(options::OPT_static);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bdynamic");
+}
   }
 
   for (auto RT : SharedRuntimes)


Index: clang/test/Driver/fuzzer.c
===
--- clang/test/Driver/fuzzer.c
+++ clang/test/Driver/fuzzer.c
@@ -29,6 +29,23 @@
 // RUN: %clang -fsanitize=fuzzer -fsanitize-coverage=trace-pc %s -### 2>&1 | FileCheck --check-prefixes=CHECK-MSG %s
 // CHECK-MSG-NOT: argument unused during compilation
 
+// Check that we respect whether thes tandard library should be linked
+// statically.
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBSTDCXX-DYNAMIC %s
+// CHECK-LIBSTDCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBSTDCXX-DYNAMIC: -lstdc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ -static-libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBSTDCXX-STATIC %s
+// CHECK-LIBSTDCXX-STATIC: "-Bstatic" "-lstdc++"
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DYNAMIC %s
+// CHECK-LIBCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBCXX-DYNAMIC: -lc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ -static-libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-STATIC %s
+// CHECK-LIBCXX-STATIC: "-Bstatic" "-lc++"
+
 int LLVMFuzzerTestOneInput(const char *Data, long Size) {
   return 0;
 }
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -833,8 +833,15 @@
 if (SanArgs.needsFuzzerInterceptors())
   addSanitizerRuntime(TC, Args, CmdArgs, "fuzzer_interceptors", false,
   true);
-if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx))
+if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx)) {
+  bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
+ !Args.hasArg(options::OPT_static);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bdynamic");
+

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 300842.
chandlerc marked an inline comment as done.
chandlerc added a comment.

Update with a regression test.


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

https://reviews.llvm.org/D80488

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/fuzzer.c


Index: clang/test/Driver/fuzzer.c
===
--- clang/test/Driver/fuzzer.c
+++ clang/test/Driver/fuzzer.c
@@ -29,6 +29,23 @@
 // RUN: %clang -fsanitize=fuzzer -fsanitize-coverage=trace-pc %s -### 2>&1 | 
FileCheck --check-prefixes=CHECK-MSG %s
 // CHECK-MSG-NOT: argument unused during compilation
 
+// Check that we respect whether thes tandard library should be linked
+// statically.
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ 
%s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBSTDCXX-DYNAMIC %s
+// CHECK-LIBSTDCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBSTDCXX-DYNAMIC: -lstdc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ 
-static-libstdc++ %s -### 2>&1 | FileCheck 
--check-prefixes=CHECK-LIBSTDCXX-STATIC %s
+// CHECK-LIBSTDCXX-STATIC: "-Bstatic" "-lstdc++"
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ %s 
-### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DYNAMIC %s
+// CHECK-LIBCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBCXX-DYNAMIC: -lc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ 
-static-libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-STATIC 
%s
+// CHECK-LIBCXX-STATIC: "-Bstatic" "-lc++"
+
 int LLVMFuzzerTestOneInput(const char *Data, long Size) {
   return 0;
 }
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -833,8 +833,15 @@
 if (SanArgs.needsFuzzerInterceptors())
   addSanitizerRuntime(TC, Args, CmdArgs, "fuzzer_interceptors", false,
   true);
-if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx))
+if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx)) {
+  bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
+ !Args.hasArg(options::OPT_static);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bdynamic");
+}
   }
 
   for (auto RT : SharedRuntimes)


Index: clang/test/Driver/fuzzer.c
===
--- clang/test/Driver/fuzzer.c
+++ clang/test/Driver/fuzzer.c
@@ -29,6 +29,23 @@
 // RUN: %clang -fsanitize=fuzzer -fsanitize-coverage=trace-pc %s -### 2>&1 | FileCheck --check-prefixes=CHECK-MSG %s
 // CHECK-MSG-NOT: argument unused during compilation
 
+// Check that we respect whether thes tandard library should be linked
+// statically.
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBSTDCXX-DYNAMIC %s
+// CHECK-LIBSTDCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBSTDCXX-DYNAMIC: -lstdc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libstdc++ -static-libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBSTDCXX-STATIC %s
+// CHECK-LIBSTDCXX-STATIC: "-Bstatic" "-lstdc++"
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DYNAMIC %s
+// CHECK-LIBCXX-DYNAMIC-NOT: -Bstatic
+// CHECK-LIBCXX-DYNAMIC: -lc++
+//
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=libc++ -static-libstdc++ %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-STATIC %s
+// CHECK-LIBCXX-STATIC: "-Bstatic" "-lc++"
+
 int LLVMFuzzerTestOneInput(const char *Data, long Size) {
   return 0;
 }
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -833,8 +833,15 @@
 if (SanArgs.needsFuzzerInterceptors())
   addSanitizerRuntime(TC, Args, CmdArgs, "fuzzer_interceptors", false,
   true);
-if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx))
+if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx)) {
+  bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
+ !Args.hasArg(options::OPT_static);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bdynamic");
+}
   }
 
   for (auto RT : SharedRuntimes)
___

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D80488#2341009 , @vitalybuka wrote:

> LGTM, but it could be better with a test

Thanks, added test and landing.


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

https://reviews.llvm.org/D80488

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

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a reviewer: vitalybuka.
chandlerc marked an inline comment as done.
chandlerc added a comment.

Ping (and adding some sanitizer folks)? I'd really love to stop building with 
this local patch




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:754
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);

MaskRay wrote:
> This is correct. If GNU ld<2.25 compatibility is not needed, `--push-state`, 
> `-Bstatic`, link, `--pop-state`
I'd prefer to keep this as-is because it matches exactly what the toolchain 
does elsewhere. If we want to drop support for older `ld`s, we should update in 
both places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80488

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

[PATCH] D88789: [InstCombine] Revert rL226781 "Teach InstCombine to canonicalize loads which are only ever stored to always use a legal integer type if one is available." (PR47592)

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D88789#2310967 , @efriedma wrote:

> In D88789#2310606 , @chandlerc wrote:
>
>> FWIW, I still very much feel that this is the correct canonicalization, and 
>> that downstream problems *must* be fixed downstream. Avoiding this 
>> canonicalization doesn't actually fix them, it just makes us less *aware* of 
>> the problems that still fundamentally exist. =[
>
> I'd agree if we excluded all pointers from canonicalization.  But the 
> semantics of inttoptr and inttoptr-equivalent memory operations are weird; in 
> general, I'm not sure we can recover the original semantics of the code if we 
> throw away the pointer-ness of pointer load/store operations.
>
> To address the issue at hand, I think changing the isNonIntegralPointerType() 
> check to just isPtrOrPtrVectorTy() would be enough.  I think that might make 
> sense?

Keeping loads and stores of pointers as pointers to the extent possible doesn't 
seem like a bad idea, but I'm worried people will feel like this gives a 
*semantic* guarantee that isn't really there. Fundamentally, LLVM still doesn't 
currently have typed memory. All of the optimizer is built upon this assumption.

Anyways, while it doesn't seem intrinsically bad to preserve pointer types as 
much as possible, I feel like the underlying problem should be addressed in a 
more fundamental way -- that this change will just shift the problem to more 
complex cases where the frontend happens to use a memcpy or something similar. 
I wonder if revisiting D75505  makes somewhat 
more sense, although clearly it would need some different approach to address 
the compile time issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88789

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

[PATCH] D87837: [Driver] Add disabled-by-default -Wuse-ld-path for the deprecation warning for -fuse-ld=/abs/path

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Good to confirm with Dave of course, but this looks pretty great to me, and 
thanks so much!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87837

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

[PATCH] D87837: [Driver] Remove the deprecation warning for -fuse-ld=/abs/path

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

FWIW, I would like to see *something* like this (both in the release and on 
trunk) but not sure this is actually the patch we want... Clang typically uses 
`FIXME` comments, and doesn't leave commented out code...

I feel like this should be an off-by-default warning, with an actual warning 
flag maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87837

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

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.

Cool, thanks and LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71687



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

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
Herald added subscribers: cfe-commits, mcrosier.
Herald added a project: clang.

No idea if this is 'correct' or the right way to fix this, so just
sending this mostly as an FYI. Someone who works more closely on the
sanitizers might need to take it over and figure out how this should be
working and add relevant test cases, etc.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80488

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -747,8 +747,15 @@
   !Args.hasArg(options::OPT_shared)) {
 
 addSanitizerRuntime(TC, Args, CmdArgs, "fuzzer", false, true);
-if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx))
+if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx)) {
+  bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
+ !Args.hasArg(options::OPT_static);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bdynamic");
+}
   }
 
   for (auto RT : SharedRuntimes)


Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -747,8 +747,15 @@
   !Args.hasArg(options::OPT_shared)) {
 
 addSanitizerRuntime(TC, Args, CmdArgs, "fuzzer", false, true);
-if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx))
+if (!Args.hasArg(clang::driver::options::OPT_nostdlibxx)) {
+  bool OnlyLibstdcxxStatic = Args.hasArg(options::OPT_static_libstdcxx) &&
+ !Args.hasArg(options::OPT_static);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bstatic");
   TC.AddCXXStdlibLibArgs(Args, CmdArgs);
+  if (OnlyLibstdcxxStatic)
+CmdArgs.push_back("-Bdynamic");
+}
   }
 
   for (auto RT : SharedRuntimes)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: clang/test/Misc/loop-opt-setup.c:12
+// Check br i1 to make sure that the loop is fully unrolled
 // CHECK-NOT: br i1
 

This is dead now that you have different prefixes...



Comment at: clang/test/Misc/loop-opt-setup.c:32-33
+
+// Check br i1 to make sure the loop is gone, there will still be a label 
branch for the infinite loop.
+// CHECK-NEWPM-NOT: br i1
+

But for which function?

I'd rework these to be more modern `CHECK-LABEL` and affirmative checks for the 
unrolling.



Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:4-6
+; We don't end up deleting the loop, but we remove everything inside of it so 
checking for any
+; reasonable instruction from the original loop will work.
+; CHECK-NOT: br i1

echristo wrote:
> chandlerc wrote:
> > Make sure it is in the correct function at least, and maybe after the label 
> > for the loop header?
> Got it. There's not a lot of function left at the end:
> 
> define void @walrus() local_unnamed_addr #0 {
> entry:
>   br label %for.cond.preheader
> 
> for.cond.preheader:   ; preds = 
> %for.cond.preheader, %entry
>   br label %for.cond.preheader
> }
Then switch to generated `CHECK` lines?

The checks we have here will pass easily after incorrect edits that cause this 
test to not actually check what it intends to. I'd either check something that 
constructively shows unrolling or generate exact checks (if its small enough to 
be genuinely stable).



Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:19-20
+
+; Function Attrs: noinline nounwind optnone uwtable
+define linkonce_odr dso_local void @_Z6Helperv() #1 comdat {
+entry:

echristo wrote:
> chandlerc wrote:
> > Nit, but minimize and clean this function up a touch?
> > 
> > At the least, removing all the target features seems valuable, and I'd give 
> > things stable names instead of numbered values.
> Got it. Most things have stable names, only a few temporaries have numbered 
> names.
Sure, but even those will make future edits to this really frustrating with 
irrelevant diff, etc.

And this test case can still be minimized. As an example, the function is 
currently attributed as *optnone*. =/

I think you can just run this through the metarenamer pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71687



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

[PATCH] D72893: [NewPassManager] Add assertions when getting statefull cached analysis.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM other than two nits here, this is really awesome!




Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:856-858
+auto *ResultFAMCP =
+(*C, CG);
+ResultFAMCP->updateFAM(FAM);

asbirlea wrote:
> chandlerc wrote:
> > Check that it doesn't hit an assert failure, but I think you can remove 
> > this one.
> Removing this hits the assertion.
Dunno what I was thinking. Of course it does.

Anyways, skip the variable and just update the result directly?



Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:908
+// result.
+ResultFAMCP =
+(*C, CG);

Similar to above, skip the variable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72893



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

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Wooot about finally having a test case! (Sorry for nit picking it a bit )




Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:4-6
+; We don't end up deleting the loop, but we remove everything inside of it so 
checking for any
+; reasonable instruction from the original loop will work.
+; CHECK-NOT: br i1

Make sure it is in the correct function at least, and maybe after the label for 
the loop header?



Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:12-17
+; Function Attrs: noinline optnone uwtable
+define dso_local void @_Z3Runv() #0 {
+entry:
+  call void @_Z6Helperv()
+  ret void
+}

Are both functions needed?



Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:19-20
+
+; Function Attrs: noinline nounwind optnone uwtable
+define linkonce_odr dso_local void @_Z6Helperv() #1 comdat {
+entry:

Nit, but minimize and clean this function up a touch?

At the least, removing all the target features seems valuable, and I'd give 
things stable names instead of numbered values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71687



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

[PATCH] D72893: [NewPassManager] Add assertions when getting statefull cached analysis.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Really like the approach now. Pretty minor code nits below only. =D




Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:856-858
+auto *ResultFAMCP =
+(*C, CG);
+ResultFAMCP->updateFAM(FAM);

Check that it doesn't hit an assert failure, but I think you can remove this 
one.



Comment at: llvm/include/llvm/IR/PassManager.h:812
+auto *RetRes = _cast(ResultConcept)->Result;
+if (calledFromProxy) {
+  PreservedAnalyses PA = PreservedAnalyses::none();

asbirlea wrote:
> chandlerc wrote:
> > Could this logic be moved into the callers that pass true here to avoid the 
> > need for a book parameter?
> I haven't addressed this. Yes, I can move the logic in to the caller but I 
> need to make the AnalysisManagerT::Invalidator constructor public.
> Is there another way to do this?
Ah, good point.

How about making this a `verifyNotInvalidated` method separate from 
`getCachedResult`? That still seems a bit cleaner to me than a bool parameter. 
What do you think?



Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:249-251
+  // Create a new empty Result. This needs to have the FunctionAnalysisManager
+  // inside through the updateFAM() API whenever the FAM's module proxy goes
   // away.

Don't fully understand this comments wording... Maybe something more along the 
lines of:

> We just return an empty result. The caller will use the `updateFAM` interface 
> to correctly register the relevant `FunctionAnalysisManager` based on the 
> context in which this proxy is run.



Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:341
+ FunctionAnalysisManager ) {
+  auto *ResultFAMCP = (C, G);
+  ResultFAMCP->updateFAM(FAM);

Omit the variable?



Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:406-411
   bool NeedFAMProxy =
   AM.getCachedResult(*OldC) != nullptr;
+  FunctionAnalysisManager *FAM = nullptr;
+  if (NeedFAMProxy)
+FAM =
+(*OldC, 
G).getManager();

Can this be simplified to:

```
FunctionAnalysisManager *FAM = nullptr;
if (auto *FAMProxy = 
AM.getCachedResultgetManager();
```

And then use `FAM` being non-null below instead of the `NeedFAMProxy` bool?




Comment at: llvm/lib/Analysis/CGSCCPassManager.cpp:705
+  if (HasFunctionAnalysisProxy) {
+auto *ResultFAMCP =
+(*C, G);

Could omit the variable?



Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:1172
 // re-use the exact same logic for updating the call graph to reflect the
 // change.
+// Inside the update, we also update the FunctionAnalysisManager in the

Add a blank line between the paragraphs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72893



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

[PATCH] D75936: Add a Pass to X86 that builds a Condensed CFG for Load Value Injection (LVI) Gadgets [4/6]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: llvm/lib/Target/X86/ImmutableGraph.h:46-56
+  using NodeValueT = _NodeValueT;
+  using EdgeValueT = _EdgeValueT;
+  using size_type = _SizeT;
+  class Node;
+  class Edge {
+friend class ImmutableGraph;
+template  friend class ImmutableGraphBuilder;

Folks, this isn't even close to following LLVM's coding conventions or naming 
conventions.

These violate the C++ standard.

This shouldn't have been landed as-is. Can you all back this out and actually 
dig into the review and get this to match LLVM's actual coding style and 
standards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75936



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

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Can we add an LLVM test w/ the metadata so that we have an entirely LLVM test 
flow that ensures the pass builder DTRT?

(I still would include the Clang side test which is also very useful to test 
integrating Clang w/ different flows through the pass manager.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71687



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Just wanted to say thanks for the performance data! I know it was hard to get, 
but it is really, really useful to help folks evaluate these kinds of changes 
with actual data around the options available.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

I'm seeing lots of updates to fix bugs, but no movement for many days on both 
my meta comments and (in some ways more importantly) James's meta comments. 
(And thanks Philip for chiming in too!)

Meanwhile, we really, really need to get this functionality in place. The 
entire story for minimizing the new microcode performance hit hinges on these 
patches, and I'm really worried by how little progress we're seeing here.


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

https://reviews.llvm.org/D70157



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

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Minor nits around redundant predicates for SROA. With thouse fixed, LGTM.

I'd really love to find a way to make TCO debuggable so that we don't lose 
that. I'm particularly worried about code that relies on it to not run out of 
stack. Not sure what the best thing to do here is though. Anyways, not relevant 
for this iteration. I mostly feel bad for a potential future re-churn of all 
the tests. ;]




Comment at: llvm/lib/Passes/PassBuilder.cpp:395
   // scalars.
-  FPM.addPass(SROA());
+  if (Level >= O1)
+FPM.addPass(SROA());

We know `O0` isn't used here, so this should be a no-op.



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:263
   FPM.add(createCFGSimplificationPass());
-  FPM.add(createSROAPass());
+  if (OptLevel >= 1)
+FPM.add(createSROAPass());

We early exit at `O0` above, so this is a no-op.



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:294
 MPM.add(createFunctionInliningPass(IP));
-MPM.add(createSROAPass());
+if (OptLevel >= 1)
+  MPM.add(createSROAPass());

We only reach here if `OptLevel > 0` so this should be redundant?



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:325
   // Break up aggregate allocas, using SSAUpdater.
-  MPM.add(createSROAPass());
+  if (OptLevel >= 1)
+MPM.add(createSROAPass());

This doesn't have the assert, but I believe this is only used above `O0` as 
well. Maybe just add the assert?



Comment at: llvm/test/Other/new-pm-defaults.ll:253
+; CHECK-Os-NEXT: Finished llvm::Function pass manager run.
+; CHECK-Oz-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-EP-SCALAR-LATE-NEXT: Running pass: NoOpFunctionPass

mehdi_amini wrote:
> Just a drive-by idea: you could have simplified the changes here with a new 
> prefix: "CHECK-O23sz" that you could add to the non-O1 invocations.
> 
Good idea!


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

https://reviews.llvm.org/D65410



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

(Just a reminder that we need to have both performance and code size numbers 
for this patch. And given that there are a few options, may need a few 
examples.)


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a reviewer: chandlerc.
chandlerc added a comment.

Thanks for sending this out!

I've made a minor comment on the flag structure, but my bigger question would 
be: can you summarize the performance hit and code size hit you've seen across 
some benchmarks? SPEC and the LLVM test suite would be nice, and maybe code 
size alone for some large binary like Chrome, Firefox, or Safari?

I'd be particularly interested in comparing the performance & code size hit 
incurred by the suggested "fused,jcc,jmp" set compared to all (adding call, 
ret, and indirect). If there is a big drop in either, I'd love to know which of 
these incurs the large drop.

Thanks,
-Chandler




Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:88
+SmallVector BranchTypes;
+StringRef(Val).split(BranchTypes, '-', -1, false);
+for (auto BranchType : BranchTypes) {

I feel like a comma-separated list would be a bit more clear...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70157



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

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc1499b90dc4: Improve Clangs getting involved document 
and make it more inclusive in wording. (authored by chandlerc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69351

Files:
  clang/www/get_involved.html


Index: clang/www/get_involved.html
===
--- clang/www/get_involved.html
+++ clang/www/get_involved.html
@@ -50,8 +50,8 @@
 as well.
 
 
-The best way to talk with other developers on the project is through the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
+The most common way to talk with other developers on the project is through
+the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
 list.  The clang mailing list is a very friendly place and we welcome
 newcomers.  In addition to the cfe-dev list, a significant amount of design
 discussion takes place on the Contributing Extensions to Clang
 
-Clang has always been designed as a platform for experimentation,
+Clang is designed to support experimentation,
 allowing programmers to easily extend the compiler to support great
 new language features and tools. At some point, the authors of these
 extensions may propose that the extensions become a part of Clang
-itself, to benefit the whole Clang community. But not every idea--not
-even every great idea--should become part of Clang. Extensions
-(particularly language extensions) pose a long-term maintenance burden
-on Clang, and therefore the benefits of the extension must outweigh
-those costs. Hence, these are the seven criteria used to evaluate the
-merits of a proposed extension:
+itself, to benefit the whole Clang community. However, extensions
+(particularly language extensions) have long-term maintenance costs
+for Clang. The benefits of the extension need to be evaluated against
+these costs. The Clang project uses the following criteria for this
+evaluation:
 
 
-  Evidence of a significant user community: This is based on a number of 
factors, including an actual, existing user community, the perceived likelihood 
that users would adopt such a feature if it were available, and any 
"trickle-down" effects that come from, e.g., a library adopting the feature and 
providing benefits to its users.
-
-  A specific need to reside within the Clang tree: There are some 
extensions that would be better expressed as a separate tool, and should remain 
as separate tools even if they end up being hosted as part of the LLVM umbrella 
project.
-
-  A complete specification: The specification must be sufficient to 
understand the design of the feature as well as interpret the meaning of 
specific examples. The specification should be detailed enough that another 
compiler vendor could conceivably implement the feature.
-
-  Representation within the appropriate governing organization: For 
extensions to a language governed by a standards committee (C, C++, OpenCL), 
the extension itself must have an active proposal and proponent within that 
committee and have a reasonable chance of acceptance. Clang should drive the 
standard, not diverge from it. This criterion does not apply to all extensions, 
since some extensions fall outside of the realm of the standards bodies.
-
-  A long-term support plan: Contributing a non-trivial extension to Clang 
implies a commitment to supporting that extension, improving the implementation 
and specification as Clang evolves. The capacity of the contributor to make 
that commitment is as important as the commitment itself.
-
-  A high-quality implementation: The implementation must fit well into 
Clang's architecture, follow LLVM's coding conventions, and meet Clang's 
quality standards, including high-quality diagnostics and rich AST 
representations. This is particularly important for language extensions, 
because users will learn how those extensions work through the behavior of the 
compiler.
-
-  A proper test suite: Extensive testing is crucial to ensure that the 
language extension is not broken by ongoing maintenance in Clang. The test 
suite should be complete enough that another compiler vendor could conceivably 
validate their implementation of the feature against it.
+  Evidence of a significant user community: This is based on a number of
+  factors, including an existing user community, the perceived likelihood that
+  users would adopt such a feature if it were available, and any secondary
+  effects that come from, e.g., a library adopting the feature and providing
+  benefits to its users.
+
+  A specific need to reside within the Clang tree: There are some 
extensions
+  that would be better expressed as a separate tool, and should remain as
+  separate tools even if they end up being hosted as part of the LLVM umbrella
+  project.
+
+  A specification: The specification must be 

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 226171.
chandlerc marked an inline comment as done.
chandlerc added a comment.

Address feedback from Manuel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69351

Files:
  clang/www/get_involved.html


Index: clang/www/get_involved.html
===
--- clang/www/get_involved.html
+++ clang/www/get_involved.html
@@ -50,8 +50,8 @@
 as well.
 
 
-The best way to talk with other developers on the project is through the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
+The most common way to talk with other developers on the project is through
+the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
 list.  The clang mailing list is a very friendly place and we welcome
 newcomers.  In addition to the cfe-dev list, a significant amount of design
 discussion takes place on the Contributing Extensions to Clang
 
-Clang has always been designed as a platform for experimentation,
+Clang is designed to support experimentation,
 allowing programmers to easily extend the compiler to support great
 new language features and tools. At some point, the authors of these
 extensions may propose that the extensions become a part of Clang
-itself, to benefit the whole Clang community. But not every idea--not
-even every great idea--should become part of Clang. Extensions
-(particularly language extensions) pose a long-term maintenance burden
-on Clang, and therefore the benefits of the extension must outweigh
-those costs. Hence, these are the seven criteria used to evaluate the
-merits of a proposed extension:
+itself, to benefit the whole Clang community. However, extensions
+(particularly language extensions) have long-term maintenance costs
+for Clang. The benefits of the extension need to be evaluated against
+these costs. The Clang project uses the following criteria for this
+evaluation:
 
 
-  Evidence of a significant user community: This is based on a number of 
factors, including an actual, existing user community, the perceived likelihood 
that users would adopt such a feature if it were available, and any 
"trickle-down" effects that come from, e.g., a library adopting the feature and 
providing benefits to its users.
-
-  A specific need to reside within the Clang tree: There are some 
extensions that would be better expressed as a separate tool, and should remain 
as separate tools even if they end up being hosted as part of the LLVM umbrella 
project.
-
-  A complete specification: The specification must be sufficient to 
understand the design of the feature as well as interpret the meaning of 
specific examples. The specification should be detailed enough that another 
compiler vendor could conceivably implement the feature.
-
-  Representation within the appropriate governing organization: For 
extensions to a language governed by a standards committee (C, C++, OpenCL), 
the extension itself must have an active proposal and proponent within that 
committee and have a reasonable chance of acceptance. Clang should drive the 
standard, not diverge from it. This criterion does not apply to all extensions, 
since some extensions fall outside of the realm of the standards bodies.
-
-  A long-term support plan: Contributing a non-trivial extension to Clang 
implies a commitment to supporting that extension, improving the implementation 
and specification as Clang evolves. The capacity of the contributor to make 
that commitment is as important as the commitment itself.
-
-  A high-quality implementation: The implementation must fit well into 
Clang's architecture, follow LLVM's coding conventions, and meet Clang's 
quality standards, including high-quality diagnostics and rich AST 
representations. This is particularly important for language extensions, 
because users will learn how those extensions work through the behavior of the 
compiler.
-
-  A proper test suite: Extensive testing is crucial to ensure that the 
language extension is not broken by ongoing maintenance in Clang. The test 
suite should be complete enough that another compiler vendor could conceivably 
validate their implementation of the feature against it.
+  Evidence of a significant user community: This is based on a number of
+  factors, including an existing user community, the perceived likelihood that
+  users would adopt such a feature if it were available, and any secondary
+  effects that come from, e.g., a library adopting the feature and providing
+  benefits to its users.
+
+  A specific need to reside within the Clang tree: There are some 
extensions
+  that would be better expressed as a separate tool, and should remain as
+  separate tools even if they end up being hosted as part of the LLVM umbrella
+  project.
+
+  A specification: The specification must be sufficient to understand the
+  design of the feature as well as 

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc marked 2 inline comments as done.
chandlerc added a comment.

Thanks, updated.




Comment at: clang/www/get_involved.html:113
+
+  A high-quality implementation: The implementation must fit well into
+  Clang's architecture, follow LLVM's coding conventions, and meet Clang's

klimek wrote:
> Perhaps: a standard-conforming implementation?
Sadly, that goes further than this is trying to go.

While this may be a bit vague, we do at least clarify the particular things we 
are looking for afterward. Better words are welcome if anyone comes up with 
them of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69351



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

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 226174.
chandlerc marked 3 inline comments as done.
chandlerc added a comment.

More fixes from Manuel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69351

Files:
  clang/www/get_involved.html


Index: clang/www/get_involved.html
===
--- clang/www/get_involved.html
+++ clang/www/get_involved.html
@@ -50,8 +50,8 @@
 as well.
 
 
-The best way to talk with other developers on the project is through the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
+The most common way to talk with other developers on the project is through
+the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
 list.  The clang mailing list is a very friendly place and we welcome
 newcomers.  In addition to the cfe-dev list, a significant amount of design
 discussion takes place on the Contributing Extensions to Clang
 
-Clang has always been designed as a platform for experimentation,
+Clang is designed to support experimentation,
 allowing programmers to easily extend the compiler to support great
 new language features and tools. At some point, the authors of these
 extensions may propose that the extensions become a part of Clang
-itself, to benefit the whole Clang community. But not every idea--not
-even every great idea--should become part of Clang. Extensions
-(particularly language extensions) pose a long-term maintenance burden
-on Clang, and therefore the benefits of the extension must outweigh
-those costs. Hence, these are the seven criteria used to evaluate the
-merits of a proposed extension:
+itself, to benefit the whole Clang community. However, extensions
+(particularly language extensions) have long-term maintenance costs
+for Clang. The benefits of the extension need to be evaluated against
+these costs. The Clang project uses the following criteria for this
+evaluation:
 
 
-  Evidence of a significant user community: This is based on a number of 
factors, including an actual, existing user community, the perceived likelihood 
that users would adopt such a feature if it were available, and any 
"trickle-down" effects that come from, e.g., a library adopting the feature and 
providing benefits to its users.
-
-  A specific need to reside within the Clang tree: There are some 
extensions that would be better expressed as a separate tool, and should remain 
as separate tools even if they end up being hosted as part of the LLVM umbrella 
project.
-
-  A complete specification: The specification must be sufficient to 
understand the design of the feature as well as interpret the meaning of 
specific examples. The specification should be detailed enough that another 
compiler vendor could conceivably implement the feature.
-
-  Representation within the appropriate governing organization: For 
extensions to a language governed by a standards committee (C, C++, OpenCL), 
the extension itself must have an active proposal and proponent within that 
committee and have a reasonable chance of acceptance. Clang should drive the 
standard, not diverge from it. This criterion does not apply to all extensions, 
since some extensions fall outside of the realm of the standards bodies.
-
-  A long-term support plan: Contributing a non-trivial extension to Clang 
implies a commitment to supporting that extension, improving the implementation 
and specification as Clang evolves. The capacity of the contributor to make 
that commitment is as important as the commitment itself.
-
-  A high-quality implementation: The implementation must fit well into 
Clang's architecture, follow LLVM's coding conventions, and meet Clang's 
quality standards, including high-quality diagnostics and rich AST 
representations. This is particularly important for language extensions, 
because users will learn how those extensions work through the behavior of the 
compiler.
-
-  A proper test suite: Extensive testing is crucial to ensure that the 
language extension is not broken by ongoing maintenance in Clang. The test 
suite should be complete enough that another compiler vendor could conceivably 
validate their implementation of the feature against it.
+  Evidence of a significant user community: This is based on a number of
+  factors, including an existing user community, the perceived likelihood that
+  users would adopt such a feature if it were available, and any secondary
+  effects that come from, e.g., a library adopting the feature and providing
+  benefits to its users.
+
+  A specific need to reside within the Clang tree: There are some 
extensions
+  that would be better expressed as a separate tool, and should remain as
+  separate tools even if they end up being hosted as part of the LLVM umbrella
+  project.
+
+  A specification: The specification must be sufficient to understand the
+  design of the feature as well as 

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
chandlerc added a reviewer: klimek.
Herald added a subscriber: mcrosier.
Herald added a project: clang.

Working with Meike and others to improve the wording in this document.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69351

Files:
  clang/www/get_involved.html


Index: clang/www/get_involved.html
===
--- clang/www/get_involved.html
+++ clang/www/get_involved.html
@@ -50,8 +50,8 @@
 as well.
 
 
-The best way to talk with other developers on the project is through the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
+The most common way to talk with other developers on the project is through
+the http://lists.llvm.org/mailman/listinfo/cfe-dev;>cfe-dev mailing
 list.  The clang mailing list is a very friendly place and we welcome
 newcomers.  In addition to the cfe-dev list, a significant amount of design
 discussion takes place on the Contributing Extensions to Clang
 
-Clang has always been designed as a platform for experimentation,
+Clang is designed to support experimentation,
 allowing programmers to easily extend the compiler to support great
 new language features and tools. At some point, the authors of these
 extensions may propose that the extensions become a part of Clang
-itself, to benefit the whole Clang community. But not every idea--not
-even every great idea--should become part of Clang. Extensions
-(particularly language extensions) pose a long-term maintenance burden
-on Clang, and therefore the benefits of the extension must outweigh
-those costs. Hence, these are the seven criteria used to evaluate the
-merits of a proposed extension:
+itself, to benefit the whole Clang community. However, extensions
+(particularly language extensions) have long-term maintenance costs
+for Clang. The benefits of the extension need to be evaluated against
+these costs. The Clang project uses the following criteria for this
+evaluation:
 
 
-  Evidence of a significant user community: This is based on a number of 
factors, including an actual, existing user community, the perceived likelihood 
that users would adopt such a feature if it were available, and any 
"trickle-down" effects that come from, e.g., a library adopting the feature and 
providing benefits to its users.
-
-  A specific need to reside within the Clang tree: There are some 
extensions that would be better expressed as a separate tool, and should remain 
as separate tools even if they end up being hosted as part of the LLVM umbrella 
project.
-
-  A complete specification: The specification must be sufficient to 
understand the design of the feature as well as interpret the meaning of 
specific examples. The specification should be detailed enough that another 
compiler vendor could conceivably implement the feature.
-
-  Representation within the appropriate governing organization: For 
extensions to a language governed by a standards committee (C, C++, OpenCL), 
the extension itself must have an active proposal and proponent within that 
committee and have a reasonable chance of acceptance. Clang should drive the 
standard, not diverge from it. This criterion does not apply to all extensions, 
since some extensions fall outside of the realm of the standards bodies.
-
-  A long-term support plan: Contributing a non-trivial extension to Clang 
implies a commitment to supporting that extension, improving the implementation 
and specification as Clang evolves. The capacity of the contributor to make 
that commitment is as important as the commitment itself.
-
-  A high-quality implementation: The implementation must fit well into 
Clang's architecture, follow LLVM's coding conventions, and meet Clang's 
quality standards, including high-quality diagnostics and rich AST 
representations. This is particularly important for language extensions, 
because users will learn how those extensions work through the behavior of the 
compiler.
-
-  A proper test suite: Extensive testing is crucial to ensure that the 
language extension is not broken by ongoing maintenance in Clang. The test 
suite should be complete enough that another compiler vendor could conceivably 
validate their implementation of the feature against it.
+  Evidence of a significant user community: This is based on a number of
+  factors, including an existing user community, the perceived likelihood that
+  users would adopt such a feature if it were available, and any secondary
+  effects that come from, e.g., a library adopting the feature and providing
+  benefits to its users.
+
+  A specific need to reside within the Clang tree: There are some 
extensions
+  that would be better expressed as a separate tool, and should remain as
+  separate tools even if they end up being hosted as part of the LLVM umbrella
+  project.
+
+  A complete specification: The specification must be sufficient to
+  understand the design of the feature as well as 

[PATCH] D68535: Fix loop unrolling initialization in the new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a reviewer: asbirlea.
chandlerc added a comment.
This revision is now accepted and ready to land.

Adding Alina so she is aware of the change and can comment if she spots 
anything I'm missing...

I think this is fine to go in as-is to fix the immediate issues. It'd be good 
to find a way to write an LLVM test for the unrolling functionality change and 
the unroll-and-jam change I comment on below. But I'm OK w/ those landing in 
follow-up changes, no need to hold up the simple fix here.




Comment at: clang/test/Misc/loop-opt-setup.c:2
+// RUN: %clang -O1 -fexperimental-new-pass-manager -fno-unroll-loops -S -o - 
%s -emit-llvm | FileCheck %s
+// RUN: %clang -O1 -fno-unroll-loops -S -o - %s -emit-llvm | FileCheck %s
+extern int a[16];

I'd use `-fno-experimental-new-pass-manager` here so we don't lose coverage 
when flipping defaults.



Comment at: llvm/lib/Passes/PassBuilder.cpp:953
   // We do UnrollAndJam in a separate LPM to ensure it happens before unroll
-  if (EnableUnrollAndJam) {
+  if (EnableUnrollAndJam && PTO.LoopUnrolling) {
 OptimizePM.addPass(

We don't have a test for this sadly Not sure the best way to add one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68535



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

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: compiler-rt/trunk/lib/profile/xxhash.h:41-42
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+

Sorry folks, but you can't do this.

You can't depend on ADT from compiler-rt currently.

There are at least two problems here:

First problem is that this pollutes the profile library with symbols from ADT. 
That really doesn't seem reasonable without *significant* and invasive changes 
to ADT. Otherwise building LLVM and linking it with the profile library will 
create an ODR violation (imagine different assert levels or different versions 
of LLVM buing built and the host toolchain).


Second, and much more critically, we haven't gotten to 100% relicensed on ADT, 
so it is critical that we not depend on it from runtime libraries.

Third, a lot of this code seems to use old license headers. Please do not add 
any code like this to LLVM, and instead use the new LLVM license for all new 
code.

For now, this patch (and any related patches) need to be reverted until these 
are addressed. Especially the license issues.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



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

[PATCH] D67323: [NewPM][Sancov] Create the Sancov Pass after building the pipelines

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM to fix folks broken by this.

That said, we should look for a way to test this in a follow up patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67323



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

[PATCH] D66988: [NewPM][Sancov] Make Sancov a Module Pass instead of 2 Passes

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM

This also makes sense to me why it would OOM. I suspect we're building a 
massive list, and at best we get lucky and they get de-duped later. At worst, 
this is actually fixing a serious functionality problem. We had the same thing 
w/ normal ASan before too. Since this pass doesn't need to be a function pass 
anyways, this seems totally fine. Thanks for tracking it down.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66988



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

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

One high level point that is at least worth clarifying, and maybe others will 
want to suggest a different approach:

The overall approach here is to have as small of a difference between the O1 
 and O2 
 pipelines as possible.

An alternative approach that we could take would be to design a focused O1 
 pipeline without regard to how 
much it diverges from the O2  
pipeline.

Which approach is used somewhat depends on the goals. I feel like the goal here 
is to get as close to the level of optimization at O2 
 as possible without losing compile 
time or coherent backtraces for test / assertion failures. For that goal, the 
approach taken makes sense. But it seems important to clarify that goal as 
otherwise I think we'd want to go in very different directions.

In D65410#1613555 , @hfinkel wrote:

> Thanks for starting on this. Can you go ahead and replace the sroa calls with 
> mem2reg calls for `O1` and then see what that does to the performance? That 
> strikes me as a major change, but certainly one that potentially makes sense, 
> so I'd rather we go ahead and test it now before we make decisions about 
> other adjustments.


I really think we need mem2reg at least at -O1... In fact, I really think we 
need SROA at O1 . If it is actually 
a compile time problem, I'd like to fix that in SROA. I don't really expect it 
to be though.

> FWIW, I thought that we might run InstCombine less often (or maybe replace it 
> with InstSimplify, in some places). Did you try that?

I think the biggest thing to do would be to avoid repeated runs of instcombine 
over the same code. I suspect we want at least one run after inliner and inside 
the CGSCC walk for canonicalization. But it'd be great to limit it to exactly 
one or mybe one before and one after the loop pipeline.




Comment at: llvm/lib/Passes/PassBuilder.cpp:407-414
   }
 
   // Speculative execution if the target has divergent branches; otherwise nop.
-  FPM.addPass(SpeculativeExecutionPass());
+  if (Level > O1)
+FPM.addPass(SpeculativeExecutionPass());
 
   // Optimize based on known information about branches, and cleanup afterward.

I think you can merge all of these?



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:362
 
+  // TODO: Investigate the cost/benefit of tail call elimination on debugging.
   MPM.add(createTailCallEliminationPass()); // Eliminate tail calls

hfinkel wrote:
> By definition, this loses information from the call stack, no?
Yeah, I'd have really expected this to be skipped.



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:432
 
+  // TODO: Investigate if this is too expensive at O1.
   MPM.add(createAggressiveDCEPass()); // Delete dead instructions

hfinkel wrote:
> Yes, I'd fall back to using regular DCE.
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65410



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

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM with two nits addressed, thanks!




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1125
+PB.addPGOInstrPassesForO0(MPM, CodeGenOpts.DebugPassManager,
+  /* RunProfileGen */ PGOOpt->Action ==
+  PGOOptions::IRInstr,

Minor nit, here and else where with the comment-named bools I'd use the style: 
`/*RunProfileGen=*/PGOOpt->Action == PGOOPtions::IRInstr`

This was suggested as more consistent w/ Clang style, and it seems a bit 
cleaner. Also, clang-tidy will recognize and check that the comment uses the 
same name as the parameter.



Comment at: llvm/lib/Passes/PassBuilder.cpp:1839
+addPGOInstrPassesForO0(MPM, DebugLogging,
+   /* RunProfileGen */
+   PGOOpt->Action == PGOOptions::IRInstr,

Same nit about the parameter comments here.


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

https://reviews.llvm.org/D64029



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

[PATCH] D63638: [clang][NewPM] Add new pass manager RUN lines to avx512f-builtins.c

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Just to make sure we're on the same page (and sorry I didn't jump in sooner)...

With the old PM, *anything* that is `always_inline` *gets* `instsimplify` run 
on it, even at -O0, even if you didn't want that. So using `-instsimplify` 
explicitly is, IMO, not any more scary of a reliance on LLVM's behavior than 
the old PM already subjected us to...

That said, if the x86 maintainers are comfortable with *only* using the new PM 
(because it has an always inliner that literally does nothing else and thus has 
an absolute minimum amount of LLVM transformations applied), I certainly don't 
have any objections. =D


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

https://reviews.llvm.org/D63638



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

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Sorry for the delay here.

It'd be nice to land the LLVM patch first and with its own testing -- we should 
have testing for the pass builder independent of Clang (IE, in the LLVM tests).

One option would be to test it with a unittest, not sure we have pass builder 
unittests at the moment.

Probably a better option would be to define a pseudo pass name like our 
`default`, maybe `pgo`. Then you can parse the `O0` the same way we do 
for the `default` thing. when it is O0 you can call the new routine I've 
suggested below. When it is higher, you can call the existing routine much like 
the default pipeline code does. This would all be implemented inside the pass 
builder so no issues w/ using the utility code there.

This would let us much more nicely write tests for the pipeline fragment 
generated for PGO both at O0 and above -- we have tests that specifically print 
out the pipeline sequence. This makes it very easy to visualize changes to the 
pipeline. And it would let you test the O0 code path here.

The clang part of the patch (once adapted to the narrower API) seems likely to 
be good then.

I'm happy for this to be two patches (first llvm, then Clang), or one patch. 
Whatever is easier for you.




Comment at: llvm/lib/Passes/PassBuilder.cpp:576
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }

xur wrote:
> I moved this pass under the condition when Early inline is enabled. I'm under 
> the impression that the intention is to clean up the code for the inline.
> Chandler: you added this pass. If you don't like this move, I can pull it out 
> and put it under another (Level > 0) condition.
This makes sense to me. It was probably just transcription error on my part.



Comment at: llvm/lib/Passes/PassBuilder.cpp:579-602
   if (RunProfileGen) {
 MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+if (Level > 0) {
+  FunctionPassManager FPM;
+  FPM.addPass(
+  createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));

Rather than moving the entire routine that is really only intended for the 
innards of the pass manager to be public, I'd do something a bit more narrow...

Maybe just add pipeline method to the pass builder to generate a minimal 
pipeline suitable for O0 usage?

I think this code will be simpler to read w/o having to have the O0 conditions 
plumbed all the way through it, and the duplication is tiny.

If you want, could pull out the use bits which are common and use early exit to 
make the code more clear:

```
if (!RunProfileGen) {
  addPGOUsePasses(...);
  return;
}

...
```

(Not sure what name to use for this, but you see the idea.) Then there would be 
almost no duplication between the O0 and this code, w/o having to have the 
conditions threaded throughout.


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

https://reviews.llvm.org/D64029



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM, thanks so much for sticking through this, I know it was ... nontrivial!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

The used thing still seems like there is an underlynig bug here. See below.

(Also a tiny nit, but that isn't the interesting part.)




Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:206
+
+std::string getSectionStart(const Triple ,
+const std::string ) {

Maybe call these `getFooBarImpl` so that you don't have to rely on name lookup 
trickery below to refer to them unambiguously?



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:750
   Constant::getNullValue(ArrayTy), "__sancov_gen_");
+  appendToCompilerUsed(*CurModule, {Array});
 

There is a `GlobalsToAppendToCompilerUsed` vector that we append `Array` to 
below (line 759). Why isn't that sufficient instead of this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63155#1563275 , @xur wrote:

> In D63155#1563266 , @chandlerc wrote:
>
> > I just think we also need to understand why *no other test failed*, and why 
> > the only test we have for doing PGO at O0 is this one and it could be 
> > "fixed" but such a deeply unrelated change
>
>
> We have special code to do PGO at O0 in old pass manager. This is not done in 
> the new pass manager.


I'm not sure how this addresses my question about test coverage.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63155



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63155#1563240 , @leonardchan wrote:

> In D63155#1563229 , @xur wrote:
>
> > This patch does not make sense to me.
> >
> > The reason of failing with -fexperimental-new-pass-manager is because we 
> > don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> > instrumentation/use passes are under 
> > PassBuilder::buildPerModuleDefaultPipeline.
> >
> > This patch add a pass
> >
> >   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
> >
> > which only gives you the wrong signal  of instrumentation is done.
> >
> > I wrote pass PGOInstrumentationGenCreateVar only for some trick 
> > interactions for thinlto under ldd for CSPGO.
> >  Regular FDO should not use it.
> >
> > The right fix is to enable PGO instrumentation and use in pass builder for 
> > O0.
> >
> > I would like to request to revert this patch.
>
>
> As an alternative, could I instead request that we remove the BackendUtil 
> changes and just mark the runs in gcc-flag-compatibility.c with 
> `-fno-experimental-new-pass-manager`. This way we could clarify that under 
> the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.


No, I think we should be doing PGO at O0 in the new PM if we do so in the old 
PM.

I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct 
to fix this test case. That seems plausible to me at least, and I think 
reverting and figuring out what the right way to do it is a fine approach.

I just think we also need to understand why *no other test failed*, and why the 
only test we have for doing PGO at O0 is this one and it could be "fixed" but 
such a deeply unrelated change


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63155



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63155#1563229 , @xur wrote:

> This patch does not make sense to me.
>
> The reason of failing with -fexperimental-new-pass-manager is because we 
> don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> instrumentation/use passes are under 
> PassBuilder::buildPerModuleDefaultPipeline.
>
> This patch add a pass
>
>   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
>
> which only gives you the wrong signal  of instrumentation is done.
>
> I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions 
> for thinlto under ldd for CSPGO.
>  Regular FDO should not use it.
>
> The right fix is to enable PGO instrumentation and use in pass builder for O0.
>
> I would like to request to revert this patch.


I mean, I'm happy for the patch to be reverted, but I still really don't 
understand why this fixes the test to work *exactly* the same as w/ the old 
pass manager and doesn't break any other tests if it is completely wrong? It 
seems like there must be something strange with the test coverage if this is so 
far off of correct without any failures...

I also don't understand what fix you are suggesting instead, but maybe you can 
show a patch?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63155



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

FWIW, I think we can wait for a bug to be filed or report come in with some 
functional test case before we change any behavior here.

I'm just suggesting how to make the test better below.




Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668
+// We must also remove exception handling before attaching sample profile
+// data.
+MPM.addPass(
+createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass()));

leonardchan wrote:
> tejohnson wrote:
> > chandlerc wrote:
> > > Does the this need to go before the EarlyFPM as well as before the sample 
> > > profile loader?
> > > 
> > > Also, this is ... a pretty expensive thing to do... Do we really need 
> > > this? We've been using ThinLTO and sample PGO with the new PM for a long 
> > > time and haven't seen any real issue. I think we can probably just leave 
> > > this difference between the two pass managers and remove the test for 
> > > this pass running rather than adding it.
> > > 
> > > CC-ing some others just to double check, but I'm not aware of any issues 
> > > we've seen with PGO that were because of this discrepancy.
> > We don't tend to use exception handling so might not be hitting the issue 
> > described in the headline.
> > 
> > If this is fixing an issue with SamplePGO and EH cleanup interactions, can 
> > you add a test that tests for that (i.e. is there a code optimization issue 
> > currently?).
> I'm not entirely familiar with the expected codegen for SamplePGO or PruneEH 
> and don't really know if not adding these 2 passes breaks anything.
> 
> Under the legacy PM, exception handling was removed via `PruneEH` before 
> creating the sample profile loader, so this change was meant to match the 
> behavior since `function-attrs` and `function(simplify-cfg)` (the new PM 
> equivalent for `-prune-eh`) did not initially run before 
> `SampleProfileLoaderPass` was added to the pipeline.
> 
> The original patch that added `CodeGen/pgo-sample.c` (D21197) doesn't seem to 
> have any codegen tests either and only checks that PruneEH runs before sample 
> profile loader creation, so I'm also unsure of what code optimizations are 
> meant to happen.
Going back to this original issue, I think I understand more what is going on...

Specifically, I can imagine that having `invoke` still in place caused some 
issues.

So I would just keep the testing that (in the new PM) SimplifyCFG is run before 
the profile loading pass, and ignore function attrs.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Eh, this seems close enough now. I'd like a better approach for the x86 
builtins, but no idea what it will end up being.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added reviewers: davidxl, tejohnson.
chandlerc added a comment.

See inline comment, but I think we should just drop the testing of the function 
attribute bit here rather than adjusting the pipeline.




Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668
+// We must also remove exception handling before attaching sample profile
+// data.
+MPM.addPass(
+createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass()));

Does the this need to go before the EarlyFPM as well as before the sample 
profile loader?

Also, this is ... a pretty expensive thing to do... Do we really need this? 
We've been using ThinLTO and sample PGO with the new PM for a long time and 
haven't seen any real issue. I think we can probably just leave this difference 
between the two pass managers and remove the test for this pass running rather 
than adding it.

CC-ing some others just to double check, but I'm not aware of any issues we've 
seen with PGO that were because of this discrepancy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

just a minor comment on one of these...




Comment at: clang/test/CodeGen/pgo-sample.c:10
+
+// The new pass manager analog to PrunEH is a combination of 'function-attrs'
+// and 'function(simplify-cgf)'.

s/PrunEH/PruneEH/



Comment at: clang/test/CodeGen/pgo-sample.c:12-14
+// NEWPM-DAG: PostOrderFunctionAttrsPass
+// NEWPM-DAG: SimplifyCFGPass
+// NEWPM-DAG: SampleProfileLoaderPass

The DAG worries me a bit ... The point here is to check that we remove EH 
before attaching sample profile data, and with the DAG it isn't clear that this 
happens *before*.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156



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

[PATCH] D63580: [clang][NewPM] Do not eliminate available_externally durng `-O2 -flto` runs

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM, again, really nice. Tiny tweak below.




Comment at: llvm/test/Other/available-externally-lto.ll:1
+; Ensure that we don't emit available_externally functions at -O2, unless 
-flto is present in which case we should preserve them for link-time inlining 
decisions.
+; RUN: opt < %s -S -passes='default' | FileCheck %s

Wrap to 80-columns?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63580



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

[PATCH] D63577: [clang][NewPM] Move EntryExitInstrumenterPass to the start of the pipeline

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM, nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63577



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232
+  MPM.addPass(ModuleSanitizerCoveragePass(SancovOpts));
+  MPM.addPass(
+  
createModuleToFunctionPassAdaptor(SanitizerCoveragePass(SancovOpts)));
+}

leonardchan wrote:
> chandlerc wrote:
> > Is there an existing function pass pipeline we can put this in, maybe by 
> > using an extension point? That would make it much faster to run I suspect.
> I tried this earlier, but when I use `registerOptimizerLastEPCallback()` 
> before the default module pipeline is made, I end up getting this linker 
> error for `-O2` runs:
> 
> ```
> fuzzer.c:(.text.sancov.module_ctor_8bit_counters[sancov.module_ctor_8bit_counters]+0x11):
>  undefined reference to `__start___sancov_pcs'
> fuzzer.c:(.text.sancov.module_ctor_8bit_counters[sancov.module_ctor_8bit_counters]+0x16):
>  undefined reference to `__stop___sancov_pcs'
> ```
> 
> I don't understand how I get this because the symbols are declared in the IR 
> dump as:
> 
> ```
> @__start___sancov_pcs = external hidden global i64*
> @__stop___sancov_pcs = external hidden global i64*
> ```
> 
> for both the optimized cases if I use the adaptor or EP. I'm still linking 
> with the same compiler-rt in each case.
> 
> The only difference I think of using the adaptor after 
> `buildPerModuleDefaultPipeline()` vs using the extension point callback 
> before is that the order in which the pass will be called changes, although I 
> still don't see how it could lead to this undefined reference if I still link 
> against compiler-rt.
> 
> Additionally, the only difference between the IR between using the adaptor vs 
> the EP is a few extra globals/declarations seem to be missing when using the 
> EP, which makes me think another pass that ran after this one removed them, 
> although these don't seem to be related to the error.
> 
Hmm...

Maybe those missing globals / declarations are relevant. Maybe they are 
necessary to get the linking against the sancov runtime to work correctly?

You could try adding these globals / declarations to the 'used' set so they 
don't get removed by later passes:
https://llvm.org/docs/LangRef.html#the-llvm-compiler-used-global-variable

Otherwise, I have no idea, maybe makes sense to loop in the sanitizer folks 
explicitly for help debugging this...



Comment at: llvm/lib/Passes/PassRegistry.def:248
 FUNCTION_PASS("tsan", ThreadSanitizerPass())
+FUNCTION_PASS("sancov-func", SanitizerCoveragePass())
 #undef FUNCTION_PASS

leonardchan wrote:
> chandlerc wrote:
> > Consistency would suggest just `sancov` here? I don't feel strongly though.
> Ah, so the reason why I don't use `sancov` here is because in the tests when 
> using `-passes='function(sancov)', I get the following error:
> 
> ```
> /usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-b133876586/bin/opt:
>  unknown function pass 
> '/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-b133876586/bin/sancov'
> ```
> 
> I think this is because lit tries to substitute `sancov` with the one under 
> `bin/sancov` in my build dir. I'd be up for changing it if there's a way 
> around this.
That's... really entertaining.

I suspect the tool should really be `llvm-sancov` to avoid this kind of thing.

But anyways, i'm OK with this or calling it something else. There is probably a 
way to hide a thing from `lit`'s substitution as well, I just don't know what 
it is


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D62225: [clang][NewPM] Fixing remaining -O0 tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

FWIW, this LGTM. We may want to tweak the behavior around flattening, but that 
can happen as a follow-up, and this seems to get us to a better state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62225



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

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

OMG, I'm so sorry, I had no idea that the tests would explode like that... 
Yeah, I don't think that's useful

Maybe a better approach is to just explicitly run the code through `opt 
-passes=instsimplify` before handing it to FileCheck? That should produce 
almost identical results to the old PM's inliner?

Feel free to just try this out and report back -- if it isn't looking good, 
don't invest a bunch of time in it, we should talk to Craig and figure out what 
the right long-term strategy here is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63156#1543937 , @leonardchan wrote:

> I did some more digging and found the following differences between PMs for 
> each test, and they seem to all differ and can be fixed for different reasons.


Awesome, and sorry this is such a big effort, but I think these are all going 
to end up being pretty interesting root causes. I guess yay, Clang's testing is 
doing its job?

> **CodeGen/aggregate-assign-call.c**: The new PM on -O1 codegen produces the 
> do/while loop differently but still emits the lifetime start/end intrinsics 
> around temporary variables, which is what the test checks for. Here we can 
> just check for the instructions while ignoring the branches generated.

Weird, but makes sense.

> **CodeGen/arm_acle.c**: A bunch of instructions seem to have been combined 
> into a call to `llvm.fshl.i32`, so we just check for those under the new PM.

Same.

> **CodeGen/available-externally-suppress.c**: `available_externally` was not 
> emitted during `-O2 -flto` runs when it should still be retained for link 
> time inlining purposes. This can be fixed by checking that we aren't 
> LTOPrelinking when adding the `EliminateAvailableExternallyPass`.

Yikes, this is a good bug to fix!

> **CodeGen/pgo-sample.c [No new fix]**: The test checks that `PruneEH` runs, 
> but there doesn’t seem to be a corresponding new PM pass for it. Should there 
> be? If there is one then we can just check for that in the debug PM dump.

The analog would be `function-attrs` and `function(simplify-cfg)` in some 
combination. Maybe this should just check that some specific thing is optimized 
away at `-O2` rather than checking the specific details of pass pipeline 
construction?

> **CodeGen/x86_64-instrument-functions.c [No new fix]**: This one's a bit 
> complicated. The initial problem seems to be that `EntryExitInstrumenterPass` 
> was not added into the pipeline to emit `__cyg_profile_func_enter/exit()` 
> calls. Adding that to the pipeline seems to instrument correctly now and add 
> these calls, but we run into a problem with inlining. `root()` expects 2 
> calls to `__cyg_profile_func_enter` and 2 calls to `__cyg_profile_func_exit` 
> in its body,
> 
>   ; Function Attrs: nounwind
>   define i32 @root(i32 returned %x) #0 {
>   entry:
> %0 = tail call i8* @llvm.returnaddress(i32 0)
> tail call void @__cyg_profile_func_enter(i8* bitcast (i32 (i32)* @root to 
> i8*), i8* %0) #2
> tail call void @__cyg_profile_func_enter(i8* bitcast (i32 (i32)* @leaf to 
> i8*), i8* %0) #2
> tail call void @__cyg_profile_func_exit(i8* bitcast (i32 (i32)* @leaf to 
> i8*), i8* %0) #2
> tail call void @__cyg_profile_func_exit(i8* bitcast (i32 (i32)* @root to 
> i8*), i8* %0) #2
> ret i32 %x
>   }
> 
> 
> but we get only 1 call
> 
>   ; Function Attrs: norecurse nounwind readnone
>   define i32 @root(i32 returned %x) local_unnamed_addr #0 {
>   entry:
> %0 = call i8* @llvm.returnaddress(i32 0)
> call void @__cyg_profile_func_enter(i8* bitcast (i32 (i32)* @root to 
> i8*), i8* %0)
> %1 = call i8* @llvm.returnaddress(i32 0)
> call void @__cyg_profile_func_exit(i8* bitcast (i32 (i32)* @root to i8*), 
> i8* %1)
> ret i32 %x
>   }
> 
> 
> I suspect this is due to the `leaf()` being inlined into `root()` even though 
> inlining should happen after it. The legacy `EntryExitInstrumenter` pass 
> seems to be added to the legacy FunctionPassManager first before all others 
> passes. Under the legacy PM, when this pass runs, `root()` should look like:
> 
>   ; Function Attrs: nounwind
>   define i32 @root(i32 %x) #1 {
>   entry:
> %x.addr = alloca i32, align 4
> store i32 %x, i32* %x.addr, align 4, !tbaa !2
> %0 = load i32, i32* %x.addr, align 4, !tbaa !2
> %call = call i32 @leaf(i32 %0)  ; this call is not yet inlined
> ret i32 %call
>   }
> 
> 
> but `root()` looks like this in the new PM pass:
> 
>   ; Function Attrs: norecurse nounwind readnone
>   define i32 @root(i32 returned %x) local_unnamed_addr #1 {
>   entry:
> ret i32 %x
>   }

Yeah, I think the entry/exit pass really wants to come *before* any other pass. 
That should be possible even in the new PM by using an extension point?

> **CodeGenCXX/auto-var-init.cpp**: Auto var initialization is performed 
> differently under new PM than legacy. With initialization for structs with 
> default values, the legacy PM will store the stream of 0xAAs, then initialize 
> the members with default values in a constructor, whereas the new PM will 
> just store the correct value for the whole struct right away without 
> initialization. In one case, it seems that ctor function was also inlined. 
> Fixed by check accounting for these checks separately.

Cool, makes sense.

> **CodeGenCXX/conditional-temporaries.cpp**: The new pm seems to be unable to 
> statically determine the return value of some functions. For now I just add 
> separate checks for the new PM IR.

[PATCH] D63170: [clang][NewPM] Fix broken -O0 test from missing assumptions

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM. Bit annoying that we need to do this at O0, but fine...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63170



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

[PATCH] D63168: [clang][NewPM] Fix split debug test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Code change LGTM, but again, let's update the test to check both ways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63168



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

I understand the change to explicitly say `-O2`. I also understand the change 
to add an explicit `-fno-experimental-new-pass-manager` to a `RUN` line when we 
have another `RUN` line that explicitly uses `-fexperiemntal-new-pass-manager`.

But for many of these cases, there is no new-PM `RUN` line in the test. If 
we're going to fix one `RUN` line to a particular pass manager, we should fix 
both IMO unless there is something quite odd about the IR being tested that 
makes the result of optimizinng be weirdly different between the two pass 
managers. That would be somewhat surprising, so I kinda want to know if we 
actually see that so often to understand what is causing this divergence.

In general, Clang tests shouldn't be checking such complex optimizations that 
the differences between the pass managers really manifests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Let's update the test to explicitly run w/ both PMs to make sure this keeps 
working. LGTM with that change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63155



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

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

This really confused me. We shouldn't be seeing this kind of difference in the 
new PM.

But I think I figured it out.

Both PMs have to run *some* inliner at -O0. This is because we need to inline 
`always_inline` functions. The new PM has a *super* simple (and fast 
compile-time wise) inliner designed for this. It's really good at its job. The 
old PM runs the normal inliner. This thing isn't simple at all. It does complex 
stuff to incrementally simplify code while inlining because that can have a 
huge effect on the overall efficiency of the compiler *when doing full 
optimization passes*. But we're not doing that.

The result is that because all of these tests check code that is emitted by 
`always_inline` functions in our builtin headers, and then *inlined* into the 
test functions, the inliner in the old pass manager did a bunch of "cleanup" of 
the code that the new PM doesn't do.

Honestly, I think the new PM's behavior is correct for -O0, but I think it 
makes these tests less easy to understand. I think we should instead change the 
command running here, and when we do I think we will find a set of checks that 
work for both PMs.

When we are willing to do something like run `opt` in addition to clang (the 
`convergent.cl` case), instead of just doing `-instnamer` we should also do 
`-instcombine`. I suspect this will remove the differences.

When we're just directly using `%clang_cc1` I suspect that rather than checking 
the completely unoptimized code we should check with `-O1` to make the IR much 
more stable and easy to inspect. It should also result in identical results 
from the two PMs.




Comment at: clang/test/CodeGenOpenCL/convergent.cl:2
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm %s -o - 
-fno-experimental-new-pass-manager | opt -instnamer -S | FileCheck 
-enable-var-scope %s --check-prefixes=CHECK,CHECK-LEGACY
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm %s -o - 
-fexperimental-new-pass-manager | opt -instnamer -S | FileCheck 
-enable-var-scope %s --check-prefixes=CHECK,CHECK-NEWPM
 

Probably want to use `opt -passes=instnamer` or some such to use the new PM in 
the `opt` invocation as well...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

[PATCH] D63153: [clang][NewPM] Fix broken -O0 test from the AlwaysInliner

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Code change LGTM. Can you update at least one of the tests to explicitly run 
both PMs so that we'll notice if this breaks in some weird way? Feel free to 
submit with that change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63153



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

This was a lot easier for me to understand too, thanks. Somewhat minor code 
changes below.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232
+  MPM.addPass(ModuleSanitizerCoveragePass(SancovOpts));
+  MPM.addPass(
+  
createModuleToFunctionPassAdaptor(SanitizerCoveragePass(SancovOpts)));
+}

Is there an existing function pass pipeline we can put this in, maybe by using 
an extension point? That would make it much faster to run I suspect.



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

Need to use the new file header.



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:14
+//===--===//
+#ifndef LLVM_TRANSFORMS_INSTRUMENTATION_SANITIZERCOVERAGE_H
+#define LLVM_TRANSFORMS_INSTRUMENTATION_SANITIZERCOVERAGE_H

I feel like we usually have whitespace here too?



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:24
+
+/// This is a wrapper for SanitizerCoverage usage in the new pass manager. The
+/// pass instruments functions for coverage.

Not really a wrapper, just the pass itself...

`SanitizerCoverage` is an implementation detail that the reader here doesn't 
really know about.



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:37
+
+/// This is a wrapper for the ModuleSanitizerCoverage. This adds initialization
+/// calls to the module for trace PC guards and 8bit counters if they are

Same comment as above.



Comment at: llvm/lib/Passes/PassRegistry.def:248
 FUNCTION_PASS("tsan", ThreadSanitizerPass())
+FUNCTION_PASS("sancov-func", SanitizerCoveragePass())
 #undef FUNCTION_PASS

Consistency would suggest just `sancov` here? I don't feel strongly though.



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:228-229
+  : Options(OverrideFromCL(Options)) {
+initializeModuleSanitizerCoverageLegacyPassPass(
+*PassRegistry::getPassRegistry());
   }

This should be in the legacy pass below? (actually, I tihnk it already is, so 
this can likely be removed)



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:262-269
+bool ShouldEmitInitCalls = false;
+for (const Function  : M) {
+  if (canInstrumentWithSancov(F)) {
+ShouldEmitInitCalls = true;
+break;
+  }
+}

```
if (!llvm::any_of(M, [](const Function ) { return can 
InstrumentWithSancov(F); }))
  return false;
```



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:442
+  const PostDominatorTree *PDT = (F);
+  SanitizerCoverage Sancov(*F.getParent(), Options);
+  if (Sancov.instrumentFunction(F, DT, PDT))

I completely misread this the first time through thinking you used a common 
object for state

Maybe change the constructor to accept a function to make it more obvious that 
this is intended to be built per-function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D62225: [clang][NewPM] Fixing -O0 tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

I think this ultimately needs to be split up into smaller patches. A bunch of 
these things can be landed independently. Here is my first cut at things to 
split out, each one into its own patch.

1. the LLVM change to the always inliner
2. the Clang change to how we build the always inliner
3. the PGO pipeline changes (which I have to admit I still don't fully 
understand)
4. The additions of `-fno-experimental-new-pass-manager` for test cases that 
are explicitly testing legacy PM behavior
5. switching tests to be resilient to changes in attribute group numbering (and 
adding a RUN line w/ the new PM to ensure we don't regress)

for #5 (or others) where *some* testing needs to be working before they can 
land, just sequence them after whatever they depend on

Other things I think can also be split out, but I suspect into *different* 
changes from what you have here:

6. Instead of passing `-fno-experimental-new-pass-manager` for tests that use 
`-O` but don't specify a number, Clang should pick a consistent value for the 
level I think

I'd be interested to then see what is left here.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1104-1105
   // which is just that always inlining occurs.
-  MPM.addPass(AlwaysInlinerPass());
+  // We always pass false here since according to the legacy PM logic for
+  // enabling lifetime intrinsics, we should not be compiling with O0.
+  MPM.addPass(AlwaysInlinerPass(/*InsertLifetimeIntrinsics=*/false));

leonardchan wrote:
> serge-sans-paille wrote:
> > echristo wrote:
> > > Can you elaborate more here? We do turn on the always inliner at O0 which 
> > > makes this comment a bit confusing.
> > I guess he means 
> > 
> > We always pass false here since according to the legacy PM logic for 
> > enabling lifetime intrinsics, they are not required with O0
> > 
> Yup, my bad. This is what I meant with this comment. Always inlining is used. 
> It's the lifetime intrinsics that aren't always used.
I don't think explaining it in terms of one pass manager or another is the righ 
thing to do.

Instead, I'd say what the desired result is:

```
Build a minimal pipeline based on the semantics required by Clang,
which is just that always inlining occurs. Further, disable generating
lifetime intrinsics to avoid enabling further optimizations during
code generation.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62225



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

I would just change this to have the module pass loop over the functions -- 
that seems like it'll be much cleaner.

As it is, I'm not seeing where the loop actually happens. But rather than 
trying to figure that out, just seems better to turn it into an explicit loop. 
That way you can get rid of all the check analysis overhead I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Sorry I've been a bit slow to respond here...

In D61634#1503089 , @hfinkel wrote:

> In D61634#1502201 , @tejohnson wrote:
>
> > In D61634#1502138 , @hfinkel wrote:
> >
> > > In D61634#1502043 , @efriedma 
> > > wrote:
> > >
> > > > > I have a related patch that turns -fno-builtin* options into module 
> > > > > flags
> > > >
> > > > Do you have any opinion on representing -fno-builtin using a module 
> > > > flag vs. a function attribute in IR?  It seems generally more flexible 
> > > > and easier to reason about a function attribute from my perspective.  
> > > > But I might be missing something about the semantics of -fno-builtin 
> > > > that would make that representation awkward.  Or I guess it might just 
> > > > be more work to implement, given we have some IPO passes that use 
> > > > TargetLibraryInfo?
> > >
> > >
> > > I think that a function attribute would be better. We generally use these 
> > > flags only in the context of certain translation units, and when we use 
> > > LTO, it would be sad if we had to take the most-conservative settings 
> > > across the entire application. When we insert new function call to a 
> > > standard library, we always do it in the context of some function. We'd 
> > > probably need to block inlining in some cases, but that's better than a 
> > > global conservative setting.
> >
>


FWIW, I definitely agree here. This really is the end state we're going to find 
ourselves in and we should probably go directly there.

>> Using function level attributes instead of module flags does provide finer 
>> grained control and avoids the conservativeness when merging IR for LTO. The 
>> downsides I see, mostly just in terms of the engineering effort to get this 
>> to work, are:
>> 
>> - need to prevent inlining with different attributes
> 
> I think that this should be relatively straightforward. You just need to 
> update `AttributeFuncs::areInlineCompatible` and/or 
> `AttributeFuncs::mergeAttributesForInlining` by adding a new MergeRule in 
> include/llvm/IR/Attributes.td and writing a function like 
> adjustCallerStackProbeSize.

+1

> 
> 
>> - currently the TargetLibraryInfo is constructed on a per-module basis. 
>> Presumably it would instead need to be created per Function - this one in 
>> particular seems like it would require fairly extensive changes.
> 
> Interesting point. The largest issue I see is that we need TLI available from 
> loop passes, etc., and we can't automatically recompute a function-level 
> analysis there. We need to make sure that it's always available and not 
> invalidated. TLI is one of those analysis passes, being derived only from 
> things which don't change (i.e., the target triple), or things that change 
> very rarely (e.g., function attributes), that we probably don't want to 
> require all passes to explicitly say that they preserve it (not that the 
> mechanical change to all existing passes is hard, but it's easy to forget), 
> so I think we'd like something like the CFG-only concept in the current 
> passes, but stronger and perhaps turned on by default, for this kind of 
> "attributes-only" pass. (@chandlerc , thoughts on this?).

Yep, this makes sense.

The new PM makes this quite easy. The analysis itself gets to implement the 
invalidation hook, and say "nope, I'm not invalidated". In fact, in the new PM, 
`TargetLibraryInfo` already works this way. We build an instance per function 
and say it is never invalidated.

However, they are just trivial wrappers around shared implementations, so it 
will still require some non-trivial changes. Will need to remove the 
module-based access and move clients over to provide a function when they query 
it, etc.

IIRC, `TargetTransformInfo` already basically works exactly this way in both 
old and new PMs and we should be able to look at exactly the techniques it uses 
in both pass managers to build an effective way to manage these per-function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

(Sorry for still more comments)




Comment at: test/CodeGen/loop-vectorize.c:10
+// CHECK-DISABLE-VECT-LABEL: @for_test()
+// CHECK-DISABLE-VECT: fmul double
+

You'll want to check that the vector form *doesn't* show up. I would expect 
this to pass even w/ vectorization enabled due to potential fallback loops.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

The file name of the test seems odd? How about `vectorize-loops.c`? I'd also 
make it a C test and put it in `test/CodeGen` instead of a C++ test.




Comment at: test/CodeGenCXX/no-pragma-loop.cpp:7
+// CHECK-ENABLE-VECT: for.body:
+// CHECK-ENABLE-VECT: vector.body:
+

I'd check for an instruction on a vector type as the block labels aren't really 
that stable.

Should be able to just check for something like:

```
// CHECK-LABEL: define @vectorize_loop_test {
// CHECK: mul <{{[0-9]+}} x double>
```

Or something...

Also, you'll want to explicitly pass a target triple here and require that 
target to be registered so we're not flaky when the host target changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Can you update some Clang IR generation test that uses these flags to run w/ 
the new PM? It should fail without this and pass with this.

LGTM w/ that test updated.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61620: [NewPassManager] Add tuning option: LoopUnrolling [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Same comment as on other Clang patch -- let's update an IR generation test to 
reflect this?

Should just be adding a RUN line.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61620



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

[PATCH] D58374: [Clang][NewPM] Don't bail out if the target machine is empty

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58374



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

[PATCH] D61142: Set LoopInterleaved in the PassManagerBuilder.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM, maybe add a comment here to archive some of the reasoning? (IE, that when 
unrolling is disabled we disable both literal unrolling but also interleaving 
for historical reasons)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61142



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

[PATCH] D61138: [PGO] Enable InstrProf lowering for Clang PGO instrumentation in the new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Awesome, LGTM

You might also update one of the instr prof tests to have two `RUN` lines, one 
for each pass manager. Feel free to do that (or not) and submit.


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

https://reviews.llvm.org/D61138



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

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: llvm/test/tools/gold/X86/opt-level.ll:53
+  ; CHECK-O1-OLDPM: select
+  ; The new PM does not do as many optimizations at O1
+  ; CHECK-O1-NEWPM: phi

tejohnson wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > This is intended? I'm surprised the two PMs don't have the same list of 
> > > passes for a given opt level?
> > I'm really not sure. I did compare the post-link LTO pipelines of both PMs 
> > at O0/O1/O2 and confirmed that the old PM is doing many more passes than 
> > the new PM at O1. Probably a question for @chandlerc ? In any case, I 
> > didn't want to address it here since it is orthogonal.
> Some more info:
> 
> This is the regular LTO post-link pipeline, ThinLTO post-link pipeline uses 
> essentially the same as a normal opt pipeline so would be consistent at -O1.
> 
> The optimization missing from the new PM regular LTO post link pipeline that 
> affects this test is SimplifyCFG. This and a few other optimizations are 
> added in the old PM at O1 via 
> PassManagerBuilder::addLateLTOOptimizationPasses. Note that 
> PassManagerBuilder::addLTOOptimizationPasses does exit early at -O1 (similar 
> to where we exit early in the new PM for the regular LTO post link 
> compilation). But the "late" LTO optimization passes are added 
> unconditionally above -O0. Is this correct/desired? There isn't an equivalent 
> in the new PM.
I don't think it was intentional, but I'm not sure I would directly replicate 
it if it requires adding an entirely new customization point. Is their some 
simpler way of getting equivalent results at O1?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61022



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

[PATCH] D58374: [Clang][NewPM] Don't bail out if the target machine is empty

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Maybe update at least some of the tests using these targets to additionally run 
with the new pass manager explicitly enabled via flag?




Comment at: clang/lib/CodeGen/BackendUtil.cpp:950
 
-  // The new pass manager always makes a target machine available to passes
-  // during construction.
-  CreateTargetMachine(/*MustCreateTM*/ true);
-  if (!TM)
-// This will already be diagnosed, just bail.
+  bool UsesCodeGen = (Action != Backend_EmitNothing &&
+  Action != Backend_EmitBC &&

I would say `RequiresCodeGen` instead of `UsesCodeGen` as we will still *use* 
the target manager even if it isn't required when emitting a `.ll` file.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58374



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

[PATCH] D58424: [NewPM] Add other sanitizers at O0

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58424



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

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D58375#1403144 , @efriedma wrote:

> I can understand why tests that use -O1 or -O2 would produce different 
> results with the new pass manager, but it looks like not all the tests are 
> like that.  Do you know why those tests are failing?
>
> For the tests that do use -O, instead of marking them unsupported, could you 
> use -fno-experimental-new-pass-manager or something like that?


For at least some of them, maybe we should run it in both modes (using the 
explicit flag as suggested by Eli) and check both forms.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58375



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

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Based on the -dev discussion, update once the target machine differences are 
addressed by mimicing the way the legacy PM works, which will hopefully 
restrict this similarly to what Eli is suggesting as well...


Repository:
  rC Clang

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

https://reviews.llvm.org/D58375



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

[PATCH] D28248: Work around GCC PR37804

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Hey Marshall and Michael,

As mentioned in my email to all the lists[1], patches posted to Phabricator 
before the new license was installed should be confirmed as under the new 
license before being rebased and applied. Not sure that happened here as the 
file headers are still the old file headers.

I'll update the file headers anyways, and I think this is fine as Michael is 
contributing with an @apple.com email address and so all of this is covered by 
their agreement anyways. But I wanted to mention it in case there are other 
in-flight patches on Phabricator where this is relevant.

-Chandler

[1]: http://lists.llvm.org/pipermail/llvm-dev/2018-December/128695.html 
(llvm-dev), http://lists.llvm.org/pipermail/libcxx-dev/2019-January/000140.html 
(libcxx-dev)


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

https://reviews.llvm.org/D28248



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

[PATCH] D57640: [NewPM][MSan] Add Options Handling

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Minor past-commit nit.




Comment at: lib/CodeGen/BackendUtil.cpp:1039
 [](FunctionPassManager , PassBuilder::OptimizationLevel Level) 
{
-  FPM.addPass(MemorySanitizerPass());
+  FPM.addPass(MemorySanitizerPass({}));
 });

This change isn't needed any more (since you added the default back).


Repository:
  rC Clang

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

https://reviews.llvm.org/D57640



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

[PATCH] D56932: [Driver] [NetBSD] Pass default library search paths to linker

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

There was a long discussion between two NetBSD maintainers about this (both 
already in the reviewers list of this patch). I'm not sure if there is an 
existing thread that would be better to follow up on as opposed to starting a 
fresh thread.

I think the question was: should the path search logic be handled in the 
`clang` driver or in LLD itself. FWIW, I prefer that NetBSD follow the same 
design as every other platform here with the (somewhat C and C++ specific) 
search logic provided by the `clang` driver.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56932



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

[PATCH] D56353: Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer' with'-mframe-pointer='

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Wow, thanks for the cleanups. This is much easier to read as a consequence. And 
sorry it took a while to get you another round of review. Comments below.




Comment at: include/clang/Driver/CC1Options.td:270
+def mframe_pointer_EQ : Joined<["-"], "mframe-pointer=">,
+  HelpText<"Specify effect of frame pointer elimination optimization 
(all,non-leaf,none)">, Values<"all,non-leaf,none">;
 def mdisable_tail_calls : Flag<["-"], "mdisable-tail-calls">,

Should say more `Specify which frame pointers to retain (all, non-leaf, none)`.



Comment at: lib/Driver/ToolChains/Clang.cpp:574
 
-static bool shouldUseFramePointer(const ArgList ,
-  const llvm::Triple ) {
-  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
-   options::OPT_fomit_frame_pointer))
-return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
-   mustUseNonLeafFramePointerForTarget(Triple);
-
-  if (Args.hasArg(options::OPT_pg))
-return true;
-
-  return useFramePointerForTargetByDefault(Args, Triple);
-}
+static StringRef DetermineFramePointer(const ArgList ,
+   const llvm::Triple ) {

Keep LLVM coding convention naming pattern please.



Comment at: lib/Driver/ToolChains/Clang.cpp:576-579
+  Arg *FP = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+options::OPT_fomit_frame_pointer);
+  Arg *LeafFP = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
+options::OPT_momit_leaf_frame_pointer);

This still doesn't make sense to me...

If the user specifies `-fomit-frame-point` or `-fno-omit-frame-pointer` *after* 
`-momit-leaf-frame-pointer` or `-mno-omit-leaf-frame-pointer`, then that last 
flag should win...

I think you need to first get the "base" FP state by checking the main two 
flags. Then you need to get the "last" FP state by checking *all four flags*. 
When the last flag is a leaf flag, then the state is determined by the base + 
the last. When the last flag is not one of the leaf flags, then the last flag 
fully specifies the result.

I think you can also process these variables into something simpler to test 
below, essentially handling all the matching logic in one place.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56353



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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a subscriber: hans.
chandlerc added a comment.

Adding Hans so he can be aware of the progress.


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

https://reviews.llvm.org/D56571



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

[PATCH] D56690: [Nios2] Remove Nios2 backend

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM as a patch, but same as the other wait to land until the -dev thread 
settles.


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

https://reviews.llvm.org/D56690



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

[PATCH] D56112: [clang-offload-bundler] Added install component

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc edited reviewers, added: ABataev, hfinkel; removed: chandlerc.
chandlerc added a comment.

Adding likely more useful reviewers... Not really interesting from a CMake 
perspective.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56112



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

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+   options::OPT_fomit_frame_pointer))
+return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+   mustUseFramePointerForTarget(Triple);

tabloid.adroit wrote:
> chandlerc wrote:
> > tabloid.adroit wrote:
> > > chandlerc wrote:
> > > > This doesn't correctly handle "last-flag-wins". Consider the case of 
> > > > `-mno-omit-leaf-frame-pointer -fomit-frame-pointer`. That should omit 
> > > > the leaf frame pointer, but if I read this correctly the logic here 
> > > > will use a leaf frame pointer.
> > > Updated test with this case along with some other cases.
> > > 
> > > // RUN: %clang -### -S -Os -mno-omit-leaf-frame-pointer 
> > > -fomit-frame-pointer %s 2>&1 | \
> > > // RUN:   FileCheck --check-prefix=OMIT-ALL5 %s
> > > // OMIT-ALL5-NOT: "-mdisable-fp-elim"
> > > // OMIT-ALL5-NOT: "-momit-leaf-frame-pointer"
> > > 
> > > This falls into lib/CodeGen/CGCall.cpp:1733, which causes 
> > > TargetOptions::DisableFramePointerElim returns false for all frames.
> > > 
> > > 
> > Then I don't understand what this change is doing.
> > 
> > This function, when called with arguments `-mno-omit-leaf-frame-pointer 
> > -fomit-frame-pointer` will not hit the code you've added here, and will 
> > instead return `true`. That doesn't seem like a sensible result given the 
> > desired change to these flags. If something *else* is causing us to still 
> > not use leaf frame pointers, that doesn't make the code here correct, it 
> > makes me question how this works at all (and how we are testing it).
> I see your point here. The logic is very confusing indeed.
> 
> It looks better if
> s/shouldUseFramePointer/addFlagDisableFPElim
> s/shouldUseLeafFramePointer/addFlagOmitLeafFramePointer
> 
> to show that here only decides compiler flag instead of the final code.
> 
> I think the correct way to handle these is to replace `-mdisable-fp-elim` and 
> `-momit-leaf-frame-pointer` compiler flags with one, say 
> `frame-pointer-model` = {KeepAll, OmitAll, KeepNonLeaf}, and let the driver 
> decide `frame-pointer-model` here. The downside is that it affects compiler 
> user unless we bridge deprecating flags on to new flag with some rules.
> 
Those flags are in the "CC1" interface -- the internal interface between the 
driver and the compiler internals. That interface has no stability requirements 
and isn't something we support users leveraging directly.

So I think we could actually change this to be sensible in much the way you are 
suggesting. Specifically, I would have `-frame-pointers={all,non-leaf,none}`. 
And then all of the compatibility and mapping logic in the driver will resolve 
to a very simple end result.

In code, I think we should follow a similar simplification where we have a 
single function to determine the total strategy here, rather than the logic 
being separated into multiple different functions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55915



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

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added reviewers: hans, rnk.
chandlerc added a comment.
This revision now requires changes to proceed.

The number of negations and such in the CC1 interface here and the attributes 
makes all of these tests super confusing. Wonder if we should fix that before 
making changes here.

Also adding folks to comment on the `cl.exe` driver mode and how it should 
behave.




Comment at: lib/CodeGen/CGCall.cpp:1739
   FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-  FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
 }

tabloid.adroit wrote:
> chandlerc wrote:
> > This seems like an unrelated change?
> The only user of "no-frame-pointer-elim-non-leaf" is 
> TargetOptions::DisableFramePointerElim where "no-frame-pointer-elim-non-leaf" 
> matters only if "no-frame-pointer-elim" is "false".  This is to make it less 
> confusing.
Yes, but that's kind of my point. This change is unrelated to the rest of the 
patch.

I would go ahead and land *just* this change and explain that it doesn't change 
behavior. Then the actual behavior change  can be landed independently.



Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+   options::OPT_fomit_frame_pointer))
+return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+   mustUseFramePointerForTarget(Triple);

tabloid.adroit wrote:
> chandlerc wrote:
> > This doesn't correctly handle "last-flag-wins". Consider the case of 
> > `-mno-omit-leaf-frame-pointer -fomit-frame-pointer`. That should omit the 
> > leaf frame pointer, but if I read this correctly the logic here will use a 
> > leaf frame pointer.
> Updated test with this case along with some other cases.
> 
> // RUN: %clang -### -S -Os -mno-omit-leaf-frame-pointer -fomit-frame-pointer 
> %s 2>&1 | \
> // RUN:   FileCheck --check-prefix=OMIT-ALL5 %s
> // OMIT-ALL5-NOT: "-mdisable-fp-elim"
> // OMIT-ALL5-NOT: "-momit-leaf-frame-pointer"
> 
> This falls into lib/CodeGen/CGCall.cpp:1733, which causes 
> TargetOptions::DisableFramePointerElim returns false for all frames.
> 
> 
Then I don't understand what this change is doing.

This function, when called with arguments `-mno-omit-leaf-frame-pointer 
-fomit-frame-pointer` will not hit the code you've added here, and will instead 
return `true`. That doesn't seem like a sensible result given the desired 
change to these flags. If something *else* is causing us to still not use leaf 
frame pointers, that doesn't make the code here correct, it makes me question 
how this works at all (and how we are testing it).



Comment at: test/Driver/cl-options.c:177
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /Oy- /O2 -### -- %s 2>&1 | 
FileCheck -check-prefix=Oy_2 %s
-// Oy_2: -momit-leaf-frame-pointer
+// Oy_2: -mdisable-fp-elim
 // Oy_2: -O2

Do we want to also change behavior for the CL options? We should discuss this 
w/ the Windows folks at least


Repository:
  rC Clang

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

https://reviews.llvm.org/D55915



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

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added inline comments.
This revision now requires changes to proceed.



Comment at: lib/CodeGen/CGCall.cpp:1739
   FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-  FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
 }

This seems like an unrelated change?



Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+   options::OPT_fomit_frame_pointer))
+return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+   mustUseFramePointerForTarget(Triple);

This doesn't correctly handle "last-flag-wins". Consider the case of 
`-mno-omit-leaf-frame-pointer -fomit-frame-pointer`. That should omit the leaf 
frame pointer, but if I read this correctly the logic here will use a leaf 
frame pointer.



Comment at: test/Driver/frame-pointer-elim.c:92
+
+// RUN: %clang -### -S -Os -fno-omit-frame-pointer 
-mno-omit-leaf-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-ALL2 %s

As indicated above, you need a test of the reverse order as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55915



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

I think this should be `internal-driver-option` and the text updated? I don't 
think these are necessarily experimental, they're internals of the compiler 
implementation, and not a supported interface for users. Unsure how to best 
explain this.


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

https://reviews.llvm.org/D55150



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

[PATCH] D54547: PTH-- Remove feature entirely-

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Should likely add release notes about this.

Also might be worth sending a note to cfe-dev as a heads up and give folks some 
time to say "wait wait".


Repository:
  rC Clang

https://reviews.llvm.org/D54547



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
chandlerc marked an inline comment as done.
Closed by commit rL341363: [x86/SLH] Add a real Clang flag and LLVM IR 
attribute for Speculative (authored by chandlerc, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51157?vs=162325=163789#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51157

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/attr-speculative-load-hardening.c
  cfe/trunk/test/Driver/x86-target-features.c
  llvm/trunk/docs/LangRef.rst
  llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/trunk/include/llvm/IR/Attributes.td
  llvm/trunk/lib/AsmParser/LLLexer.cpp
  llvm/trunk/lib/AsmParser/LLParser.cpp
  llvm/trunk/lib/AsmParser/LLToken.h
  llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/trunk/lib/IR/Attributes.cpp
  llvm/trunk/lib/IR/Verifier.cpp
  llvm/trunk/lib/Target/X86/X86SpeculativeLoadHardening.cpp
  llvm/trunk/lib/Target/X86/X86TargetMachine.cpp
  llvm/trunk/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/trunk/test/CodeGen/X86/O0-pipeline.ll
  llvm/trunk/test/CodeGen/X86/O3-pipeline.ll
  llvm/trunk/test/CodeGen/X86/speculative-load-hardening-gather.ll
  llvm/trunk/test/CodeGen/X86/speculative-load-hardening.ll
  llvm/trunk/test/Transforms/Inline/attributes.ll

Index: llvm/trunk/docs/LangRef.rst
===
--- llvm/trunk/docs/LangRef.rst
+++ llvm/trunk/docs/LangRef.rst
@@ -1636,6 +1636,28 @@
 This attribute indicates that HWAddressSanitizer checks
 (dynamic address safety analysis based on tagged pointers) are enabled for
 this function.
+``speculative_load_hardening``
+This attribute indicates that
+`Speculative Load Hardening `_
+should be enabled for the function body. This is a best-effort attempt to
+mitigate all known speculative execution information leak vulnerabilities
+that are based on the fundamental principles of modern processors'
+speculative execution. These vulnerabilities are classified as "Spectre
+variant #1" vulnerabilities typically. Notably, this does not attempt to
+mitigate any vulnerabilities where the speculative execution and/or
+prediction devices of specific processors can be *completely* undermined
+(such as "Branch Target Injection", a.k.a, "Spectre variant #2"). Instead,
+this is a target-independent request to harden against the completely
+generic risk posed by speculative execution to incorrectly load secret data,
+making it available to some micro-architectural side-channel for information
+leak. For a processor without any speculative execution or predictors, this
+is expected to be a no-op.
+
+When inlining, the attribute is sticky. Inlining a function that carries
+this attribute will cause the caller to gain the attribute. This is intended
+to provide a maximally conservative model where the code in a function
+annotated with this attribute will always (even after inlining) end up
+hardened.
 ``speculatable``
 This function attribute indicates that the function does not have any
 effects besides calculating its result and does not have undefined behavior.
Index: llvm/trunk/include/llvm/IR/Attributes.td
===
--- llvm/trunk/include/llvm/IR/Attributes.td
+++ llvm/trunk/include/llvm/IR/Attributes.td
@@ -176,6 +176,14 @@
 /// HWAddressSanitizer is on.
 def SanitizeHWAddress : EnumAttr<"sanitize_hwaddress">;
 
+/// Speculative Load Hardening is enabled.
+///
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and a conservative merge strategy where inlining an attributed
+/// body will add the attribute to the caller. This ensures that code carrying
+/// this attribute will always be lowered with hardening enabled.
+def SpeculativeLoadHardening : EnumAttr<"speculative_load_hardening">;
+
 /// Argument is swift error.
 def SwiftError : EnumAttr<"swifterror">;
 
@@ -232,6 +240,7 @@
 def : MergeRule<"setOR">;
 def : MergeRule<"setOR">;
 def : MergeRule<"setOR">;
+def : MergeRule<"setOR">;
 def : MergeRule<"adjustCallerSSPLevel">;
 def : MergeRule<"adjustCallerStackProbes">;
 def : MergeRule<"adjustCallerStackProbeSize">;
Index: llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
===
--- llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
+++ llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
@@ -591,6 +591,7 @@
   ATTR_KIND_NOCF_CHECK = 

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc marked 5 inline comments as done.
chandlerc added a comment.

All outstanding comments addressed, and landing this. Thanks all for the 
reviews and let me know if I missed anything.




Comment at: llvm/include/llvm/IR/Attributes.td:181-185
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and the default merge strategy of retaining the caller's
+/// attribute. This specifically matches the intent for this attribute which is
+/// that the context dominates, and inlined code will become hardened or lose
+/// its hardening based on the caller's attribute.

kristof.beyls wrote:
> After updating the LangRef.rst text, I think this comment also needs to be 
> updated. As is, it still documents the old inlining behaviour?
> I'm also not sure in how far the comment makes most sense here. This is 
> already documented in LangRef.rst, and AFAIK, the inlining compatibility mode 
> is not something that is defined here?
Updated the comment.

You can override the compatibility here -- see the `CompatRule` productions 
just before the `MergeRule` ones.

I'd like this to be documented near the code in addition to in the langref 
personally. It reminds the reader to look below at the rule sections.



Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:78-82
+static cl::opt EnableSpeculativeLoadHardening(
+"x86-speculative-load-hardening",
+cl::desc("Force enable speculative load hardening"), cl::init(false),
+cl::Hidden);
+

kristof.beyls wrote:
> I'm not sure, but do you really still need an option to force enable SLH when 
> you have function attributes now to enable it?
> When you generate LLVM-IR using clang, you now have a clang option to enable 
> that function attribute on all functions, so do you still have scenarios 
> where you need an LLVM backend option to override the function attribute?
Long term, no, we don't.

I just have some users that are already passing this flag. Out of convenience, 
I wanted to leave this working until they have picked up the new version of 
Clang and LLVM and migrated. I don't expect it to last long (a week or two?).


Repository:
  rL LLVM

https://reviews.llvm.org/D51157



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

[PATCH] D51567: CMake: Consolidate gtest detection code

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

I mean, sure.

I really don't know that supporting this ever expanding diversity of build 
strategies is worth its cost, but I don't see a specific reason to not take 
this patch


Repository:
  rC Clang

https://reviews.llvm.org/D51567



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