[PATCH] D33029: [clang-format] add option for dangling parenthesis

[Thomas via Phabricator via cfe-commits]

zroug added a comment.

I'd also like to note that this is the code style preferred by most modern code 
formatters that I know of and use:

- rustfmt for rust code
- prettier for javascript code
- black for python code


https://reviews.llvm.org/D33029



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

[PATCH] D41077: [analyser] different.CallArgsOrder checker implementation

[Anna Thomas via Phabricator via cfe-commits]

anna resigned from this revision.
anna added a comment.

Perhaps added me incorrectly as reviewer?


Repository:
  rC Clang

https://reviews.llvm.org/D41077



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk created this revision.
reuk added reviewers: Typz, krasimir, cfe-commits.

This patch aims to add support for the following rules from the JUCE coding 
standards:

- Always put a space before an open parenthesis that contains text - e.g. foo 
(123);
- Never put a space before an empty pair of open/close parenthesis - e.g. foo();

The entire set of JUCE coding guidelines can be found here 
. Unfortunately, 
clang-format can't enforce all of these style rules at the moment, so I'm 
trying to add support for them.

Patch by Reuben Thomas


Repository:
  rC Clang

https://reviews.llvm.org/D55170

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9271,6 +9271,59 @@
   verifyFormat("typedef void (*cb) (int);", Space);
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*(&a + 1);\n"
+   "&((&a)[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11055,6 +11108,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -62,7 +62,7 @@
 if (NonTemplateLess.count(CurrentToken->Previous))
   return false;
 
-const FormatToken &Previous = *CurrentToken->Previous;  // The '<'.
+const FormatToken &Previous = *CurrentToken->Previous; // The '<'.
 if (Previous.Previous) {
   if (Previous.Previous->Tok.isLiteral())
 return false;
@@ -366,7 +366,7 @@
   // specifier parameter, although this is technically valid:
   // [[foo(:)]]
   if (AttrTok->is(tok::colon) ||
-  AttrTok->startsSequence(tok::identifier, tok::identifier) || 
+  AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
 return false;
   if (AttrTok->is(tok::ellipsis))
@@ -523,11 +523,11 @@
 // Here, we set FirstObjCSelectorName when the end of the method call is
 // reached, in case it was not set already.
 if (!Contexts.back().FirstObjCSelectorName) {
-FormatToken* Previous = CurrentToken->getPr

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a subscriber: klimek.
reuk added a comment.

Would someone review this please? I'm not sure who to add for review (sorry), 
maybe one of the following?
@klimek @Typz @krasimir


Repository:
  rC Clang

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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 189942.
reuk added a comment.

I've rebased onto master, and removed unrelated formatting changes. I've also 
tried to remove some of the duplicate parens-related expressions.

I agree that the heavy nested boolean expressions are a bit painful to read, 
but I'm not sure whether a more general tidy-up falls within the scope of this 
PR.


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

https://reviews.llvm.org/D55170

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/TokenAnnotator.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9296,6 +9296,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*(&a + 1);\n"
+   "&((&a)[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11080,6 +11134,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine &Line, const FormatToken &Tok,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken &Right) const;
+
   bool spaceRequiredBetween(const AnnotatedLine &Line, const FormatToken &Left,
 const FormatToken &Right);
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2400,6 +2400,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {
@@ -2539,19 +2545,19 @@
   return true;
 return Line.Type == 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

Thanks for the review, and for approving this PR. It's very much appreciated!

I don't have merge rights - could someone commit this for me please?


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

https://reviews.llvm.org/D55170



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

Does it make sense to allow `AllowShortIfElseStatementsOnASingleLine` to be 
enabled when `AllowShortIfStatementsOnASingleLine` is not?

Perhaps it would be better to have `AllowShortIfStatementsOnASingleLine` be 
represented by an enum with values `Never`, `WithoutElse`, `Always`. The old 
`true/false` values from config files prior to this change would need to be 
made to map to the appropriate enum keys.


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

https://reviews.llvm.org/D59087



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

This looks much better now, thanks for taking another look! I've flagged some 
formatting/spelling nits, and I think the tests/docs could be a little more 
explicit about the behaviour of `WithoutElse`. Other than that, looks great!




Comment at: clang/docs/ClangFormatStyleOptions.rst:375
+  * ``SIS_Never`` (in configuration: ``Never``)
+Do not allow short if functions
+

For consistency, these descriptions should end with periods `.`



Comment at: clang/docs/ClangFormatStyleOptions.rst:386
+Allow short if functions on the same line, as long as else
+is not a compound statement
 

Missing `.`



Comment at: clang/docs/ClangFormatStyleOptions.rst:390
+
+   if (a) return;
+   else 

I think an example of the generated formatting *with* a compound else statement 
would be useful here too, in addition to the existing example.



Comment at: clang/docs/ClangFormatStyleOptions.rst:395
+  * ``SIS_Always`` (in configuration: ``Always``)
+Allow short if statements even if the else is a compounf statement
+

compounf -> compound



Comment at: clang/docs/ClangFormatStyleOptions.rst:395
+  * ``SIS_Always`` (in configuration: ``Always``)
+Allow short if statements even if the else is a compounf statement
+

reuk wrote:
> compounf -> compound
Missing `.`



Comment at: clang/include/clang/Format/Format.h:264
+/// Always put short ifs on the same line if
+/// the else is not a compound statement or not
+/// \code

Missing `.`



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:417
+// Only inline simple if's (no nested if or else), unless specified
+if (Style.AllowShortIfStatementsOnASingleLine!=FormatStyle::SIS_Always) {
+  if (I + 2 != E && Line.startsWith(tok::kw_if) &&

Has this line been clang-formatted? I'd expect spaces around the binop



Comment at: clang/unittests/Format/FormatTest.cpp:494
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = 
FormatStyle::SIS_WithoutElse;
+  verifyFormat("if (a)\n"
+   "  f();\n"

I think having a test here which shows the expected behaviour for non-compound 
else statements with `WithoutElse` would be helpful, as in the documentation.


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

https://reviews.llvm.org/D59087



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

The code looks good now. There's just a few places left where the formatting 
looks a bit suspect (sorry for missing those last time). I think once that's 
fixed this will be good to go.




Comment at: clang/unittests/Format/FormatTest.cpp:553
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = 
FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;

This line, along with some of the following changed lines, have more than 80 
characters. Make sure to clang-format changes! Consider using 
`git-clang-format` to format just this patch.


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

https://reviews.llvm.org/D59087



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[Reuben Thomas via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D59087



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

@MyDeveloperDay I'm not sure you built this branch. Perhaps you applied this 
patch to an older version of the repo. For me, 
SpaceBeforeCpp11BracedListOptions is defined at Format.h:1570.

This builds fine for me, on macOS 10.14 with Xcode 10.1's clang. I just rebased 
onto the most recent clang master and updated llvm. I've been testing/building 
with this command:

  cmake --build build --target FormatTests && 
./build/tools/clang/unittests/Format/FormatTests && cmake --build build 
--target clang-format

@klimek That's disappointing, I thought other JUCE users might benefit from 
this patch (it's quite an established library), and the amount of code that I'm 
a adding is not that high.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 191015.
reuk added a comment.

@MyDeveloperDay I'm sorry, you're absolutely correct. I'd got some other 
JUCE-related changes mixed up in this PR. Should be fixed now.


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

https://reviews.llvm.org/D55170

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/TokenAnnotator.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9337,6 +9337,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*(&a + 1);\n"
+   "&((&a)[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11121,6 +11175,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine &Line, const FormatToken &Tok,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken &Right) const;
+
   bool spaceRequiredBetween(const AnnotatedLine &Line, const FormatToken &Left,
 const FormatToken &Right);
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2400,6 +2400,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {
@@ -2539,19 +2545,19 @@
   return true;
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, t

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 191050.

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

https://reviews.llvm.org/D55170

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/TokenAnnotator.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9337,6 +9337,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*(&a + 1);\n"
+   "&((&a)[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11121,6 +11175,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine &Line, const FormatToken &Tok,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken &Right) const;
+
   bool spaceRequiredBetween(const AnnotatedLine &Line, const FormatToken &Left,
 const FormatToken &Right);
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2400,6 +2400,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {
@@ -2539,19 +2545,19 @@
   return true;
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequen

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

@klimek I agree that the rule is somewhat arbitrary. However, it's the style 
rule of an established codebase with many users (I don't have a concrete 
number, but the project has 1400 stars on github 
). I've found this patch useful when 
contributing to JUCE and I thought others might too.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk marked an inline comment as done.
reuk added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

klimek wrote:
> I'm really confused about the changes os parens. It seems like this should 
> just change the spaceRequiredBeforeParens(...), but instead seems to change 
> parens in a way that completely changes the logic? (or alternatively is 
> phabricator showing this wrong?)
I think this looks like a bigger change than it really is because I added an 
open-parens on line 2548, which then affected the formatting of the following 
lines. I also added the `isSimpleTypeSpecifier` check, so that functional-style 
casts (`int (foo)`) are given the correct spacing.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 191560.
reuk added a comment.

Removed unnecessary parens.


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

https://reviews.llvm.org/D55170

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/TokenAnnotator.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9337,6 +9337,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*(&a + 1);\n"
+   "&((&a)[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11121,6 +11175,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine &Line, const FormatToken &Tok,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken &Right) const;
+
   bool spaceRequiredBetween(const AnnotatedLine &Line, const FormatToken &Left,
 const FormatToken &Right);
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2403,6 +2403,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {
@@ -2549,9 +2555,9 @@
  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
tok::kw_new, tok::kw_delete) &&
   (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
+   (spaceRequiredBeforeParens(Right) 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357344: [clang-format]: Add NonEmptyParentheses spacing 
option (authored by reuk, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55170?vs=191560&id=192969#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55170

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/lib/Format/TokenAnnotator.h
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1690,6 +1690,17 @@
 ///}
 /// \endcode
 SBPO_ControlStatements,
+/// Put a space before opening parentheses only if the parentheses are not
+/// empty i.e. '()'
+/// \code
+///   void() {
+/// if (true) {
+///   f();
+///   g (x, y, z);
+/// }
+///   }
+/// \endcode
+SBPO_NonEmptyParentheses,
 /// Always put a space before opening parentheses, except when it's
 /// prohibited by the syntax rules (in function-like macro definitions) or
 /// when determined by other style rules (after unary operators, opening
Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -285,6 +285,8 @@
 IO.enumCase(Value, "Never", FormatStyle::SBPO_Never);
 IO.enumCase(Value, "ControlStatements",
 FormatStyle::SBPO_ControlStatements);
+IO.enumCase(Value, "NonEmptyParentheses",
+FormatStyle::SBPO_NonEmptyParentheses);
 IO.enumCase(Value, "Always", FormatStyle::SBPO_Always);
 
 // For backward compatibility.
Index: cfe/trunk/lib/Format/TokenAnnotator.h
===
--- cfe/trunk/lib/Format/TokenAnnotator.h
+++ cfe/trunk/lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine &Line, const FormatToken &Tok,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken &Right) const;
+
   bool spaceRequiredBetween(const AnnotatedLine &Line, const FormatToken &Left,
 const FormatToken &Right);
 
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2453,6 +2453,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   const FormatToken &Left,
   const FormatToken &Right) {
@@ -2599,9 +2605,9 @@
  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
tok::kw_new, tok::kw_delete) &&
   (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
+   (spaceRequiredBeforeParens(Right) &&
 (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
+ Left.is(tok::r_paren) || Left.isSimpleTypeSpecifier() ||
  (Left.is(tok::r_square) && Left.MatchingParen &&
   Left.MatchingParen->is(TT_LambdaLSquare))) &&
 Line.Type != LT_PreprocessorDirective);
@@ -2795,7 +2801,7 @@
   Left.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow))
 return true;
   if (Right.is(TT_OverloadedOperatorLParen))
-return Style.SpaceBeforeParens == FormatStyle::SBPO_Always;
+return spaceRequiredBeforeParens(Right);
   if (Left.is(tok::comma))
 return true;
   if (Right.is(tok::comma))
@@ -2879,7 +2885,7 @@
 return true;
   if (Left.is(TT_TemplateCloser) && Right.is(tok::l_paren) &&
   Right.isNot(TT_FunctionTypeLParen))
-return Style.SpaceBeforeParens == FormatStyle::SBPO_Always;
+return spaceRequiredBeforeParens(Right);
   if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) &&
   Left.MatchingParen && Left.MatchingParen->is(TT_OverloadedOperatorLParen))
 return false;
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

[Reuben Thomas via Phabricator via cfe-commits]

reuk created this revision.
reuk added reviewers: MyDeveloperDay, klimek.
reuk added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch aims to support the following rule from the Juce coding standards 
:

> The ! operator should always be followed by a space, e.g. if (! foo)

Leaving a space after `!` stops it from blending into the rest of the 
expression, which makes it easier to tell at a glance what the expression is 
doing.

Patch by Reuben Thomas


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60320

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9683,6 +9683,16 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11475,6 +11485,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
+  true);
+  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
+  false);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2831,8 +2831,10 @@
   return false;
 return true;
   }
-  if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+  if (Left.is(TT_UnaryOperator)) {
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
+   Right.is(TT_BinaryOperator);
+  }
 
   // If the next token is a binary operator or a selector name, we have
   // incorrectly classified the parenthesis as a cast. FIXME: Detect correctly.
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -489,6 +489,7 @@
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
+IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
 IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
Style.SpaceBeforeRangeBasedForLoopColon);
 IO.mapOptional("SpaceInEmptyParentheses", Style.SpaceInEmptyParentheses);
@@ -726,6 +727,7 @@
   LLVMStyle.ReflowComments = true;
   LLVMStyle.SpacesInParentheses = false;
   LLVMStyle.SpacesInSquareBrackets = false;
+  LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -1718,6 +1718,9 @@
   /// Defines in which cases to put a space before opening parentheses.
   SpaceBeforeParensOptions SpaceBeforeParens;
 
+  /// Defines whether a space should be placed after a logical `!`
+  bool SpaceAfterLogicalNot;
+
   /// If ``false``, spaces will be removed before range-based for loop
   /// colon.
   /// \code
@@ -1726,7 +1729,7 @@
   /// \endcode
   bool SpaceBeforeRangeBasedForLoopColon;
 
-  /// If ``true``, spaces may be inserted into ``()``.
+  /// \brief If ``true``, spaces may be inserted into ``()``.
   /// \code
   ///true:false:
   ///void f( ) {vs.   void f() {
@@ -1918,6 +1921,7 @@
R.SpaceBeforeCtorInitializerColon &&
SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
SpaceBeforeParens == R.SpaceBeforeParens &&
+   SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
SpaceBeforeRangeBasedForLoopColon ==
R.SpaceBeforeRangeBasedForLoopColon &&
SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 193889.

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

https://reviews.llvm.org/D60320

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9683,6 +9683,16 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11475,6 +11485,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
+  true);
+  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
+  false);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2831,8 +2831,10 @@
   return false;
 return true;
   }
-  if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+  if (Left.is(TT_UnaryOperator)) {
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
+   Right.is(TT_BinaryOperator);
+  }
 
   // If the next token is a binary operator or a selector name, we have
   // incorrectly classified the parenthesis as a cast. FIXME: Detect correctly.
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -489,6 +489,7 @@
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
+IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
 IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
Style.SpaceBeforeRangeBasedForLoopColon);
 IO.mapOptional("SpaceInEmptyParentheses", Style.SpaceInEmptyParentheses);
@@ -726,6 +727,7 @@
   LLVMStyle.ReflowComments = true;
   LLVMStyle.SpacesInParentheses = false;
   LLVMStyle.SpacesInSquareBrackets = false;
+  LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1718,6 +1718,9 @@
   /// Defines in which cases to put a space before opening parentheses.
   SpaceBeforeParensOptions SpaceBeforeParens;
 
+  /// Defines whether a space should be placed after a logical `!`
+  bool SpaceAfterLogicalNot;
+
   /// If ``false``, spaces will be removed before range-based for loop
   /// colon.
   /// \code
@@ -1918,6 +1921,7 @@
R.SpaceBeforeCtorInitializerColon &&
SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
SpaceBeforeParens == R.SpaceBeforeParens &&
+   SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
SpaceBeforeRangeBasedForLoopColon ==
R.SpaceBeforeRangeBasedForLoopColon &&
SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9683,6 +9683,16 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11475,6 +11485,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfter

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 194055.
reuk added a comment.

Updated with fixes. Thanks for pointing out that the options are ordered 
alphabetically, I hadn't noticed that!


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

https://reviews.llvm.org/D60320

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9719,6 +9719,17 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+  verifyFormat("! ! x", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11511,6 +11522,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
+  true);
+  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
+  false);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2831,8 +2831,10 @@
   return false;
 return true;
   }
-  if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+  if (Left.is(TT_UnaryOperator)) {
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
+   Right.is(TT_BinaryOperator);
+  }
 
   // If the next token is a binary operator or a selector name, we have
   // incorrectly classified the parenthesis as a cast. FIXME: Detect correctly.
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -478,6 +478,7 @@
 IO.mapOptional("SortIncludes", Style.SortIncludes);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
 IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
+IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
 IO.mapOptional("SpaceAfterTemplateKeyword",
Style.SpaceAfterTemplateKeyword);
 IO.mapOptional("SpaceBeforeAssignmentOperators",
@@ -730,6 +731,7 @@
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
+  LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1628,6 +1628,13 @@
   /// \endcode
   bool SpaceAfterCStyleCast;
 
+  /// If ``true``, a space is inserted after the logical not operator (``!``).
+  /// \code
+  ///true:  false:
+  ///! someExpression();vs. !someExpression();
+  /// \endcode
+  bool SpaceAfterLogicalNot;
+
   /// If \c true, a space will be inserted after the 'template' keyword.
   /// \code
   ///true:  false:
@@ -1911,6 +1918,7 @@
PointerAlignment == R.PointerAlignment &&
RawStringFormats == R.RawStringFormats &&
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
+   SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -1952,6 +1952,13 @@
  true:  false:
  (int) i;   vs. (int)i;
 
+**SpaceAfterLogicalNot** (``bool``)
+  If ``true``, a space is inserted after the logical not operator (``!``).
+  .. code-block:: c++
+
+ true:

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

I updated `ClangFormatStyleOptions.rst` by hand, is there a way to do that 
automatically instead (for future patches)?


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

https://reviews.llvm.org/D60320



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

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 194057.
reuk added a comment.

Fixed formatting nit.


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

https://reviews.llvm.org/D60320

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9719,6 +9719,17 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+  verifyFormat("! ! x", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11511,6 +11522,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
+  true);
+  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
+  false);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2832,7 +2832,8 @@
 return true;
   }
   if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
+   Right.is(TT_BinaryOperator);
 
   // If the next token is a binary operator or a selector name, we have
   // incorrectly classified the parenthesis as a cast. FIXME: Detect correctly.
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -478,6 +478,7 @@
 IO.mapOptional("SortIncludes", Style.SortIncludes);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
 IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
+IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
 IO.mapOptional("SpaceAfterTemplateKeyword",
Style.SpaceAfterTemplateKeyword);
 IO.mapOptional("SpaceBeforeAssignmentOperators",
@@ -730,6 +731,7 @@
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
+  LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1628,6 +1628,13 @@
   /// \endcode
   bool SpaceAfterCStyleCast;
 
+  /// If ``true``, a space is inserted after the logical not operator (``!``).
+  /// \code
+  ///true:  false:
+  ///! someExpression();vs. !someExpression();
+  /// \endcode
+  bool SpaceAfterLogicalNot;
+
   /// If \c true, a space will be inserted after the 'template' keyword.
   /// \code
   ///true:  false:
@@ -1911,6 +1918,7 @@
PointerAlignment == R.PointerAlignment &&
RawStringFormats == R.RawStringFormats &&
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
+   SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -1952,6 +1952,13 @@
  true:  false:
  (int) i;   vs. (int)i;
 
+**SpaceAfterLogicalNot** (``bool``)
+  If ``true``, a space is inserted after the logical not operator (``!``).
+  .. code-block:: c++
+
+ true:  false:
+ ! someExpression();vs. !someExpression();
+
 **SpaceAfterTemplateKeyword** (``bool``)
   If ``true``, a space will be inser

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

[Reuben Thomas via Phabricator via cfe-commits]

reuk closed this revision.
reuk added a comment.

Closed by https://reviews.llvm.org/rG91f60b44958f, 
https://reviews.llvm.org/rL357908, https://reviews.llvm.org/rC357908 (sorry, I 
forgot to update the body of the commit message to close this automatically)


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

https://reviews.llvm.org/D60320



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

[PATCH] D60225: [clang-format] [PR19056] Add support for indenting class members and methods one level under the modifiers

[Reuben Thomas via Phabricator via cfe-commits]

reuk added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2009
+  // classes case
+  if (Style.AccessModifierIndentation && Line->Level % 2 == 0)
+--Line->Level;

klimek wrote:
> What if the class starts at level 1? (for example, inside a function or due 
> to namespace indentation)
> 
> namespace A {
>   class B {
> public:
>   ..
>   };
> }
Probably worth adding a test or two that format nested classes, classes inside 
namespaces, classes defined inside functions etc.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2253
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true,
- /*MunchSemi=*/false);
+ /*MunchSemi=*/false,
+ /*IndentedAccessModifiers=*/ContainsAccessModifier);

Yeah, this would look way nicer using a settings struct.



Comment at: clang/lib/Format/UnwrappedLineParser.h:89
   void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
-  bool MunchSemi = true);
+  bool MunchSemi = true, bool IndentedAccessModifiers = false);
   void parseChildBlock();

Adjacent `bool` parameters is a bit nasty. Maybe pass a struct of settings 
instead? 
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#i4-make-interfaces-precisely-and-strongly-typed


Repository:
  rC Clang

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

https://reviews.llvm.org/D60225



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

[PATCH] D60726: Fixed -Wconversion-null warning in GCC.

[Reuben Thomas via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D60726



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

[PATCH] D60726: Fixed -Wconversion-null warning in GCC.

[Reuben Thomas via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358441: [clang-format] Fix -Wconversion-null warning in GCC 
(authored by reuk, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60726?vs=195224&id=195240#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60726

Files:
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -11393,6 +11393,7 @@
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
+  CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
@@ -11566,12 +11567,6 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
-  Style.SpaceAfterLogicalNot = false;
-  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
-  true);
-  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
-  false);
-
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);


Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -11393,6 +11393,7 @@
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
+  CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
@@ -11566,12 +11567,6 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
-  Style.SpaceAfterLogicalNot = false;
-  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
-  true);
-  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
-  false);
-
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69144: [Format] Add format check for throwing negative numbers

[Jonathan Thomas via Phabricator via cfe-commits]

jonathoma created this revision.
jonathoma added a reviewer: modocache.
jonathoma added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
jonathoma edited the summary of this revision.
jonathoma added reviewers: sammccall, Quuxplusone.

The code `throw -1;` is currently formatted by clang-format as 
`throw - 1;`. This diff adds a fix for this edge case and a test to check
for this in the future.

For context, I am looking into a related bug in the clang-formatting of
coroutine keywords: `co_yield -1;` is also reformatted in this manner 
as `co_yield - 1;`. A later diff will add these changes and tests for the
`co_yield` and `co_return` keywords.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69144

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1756,7 +1756,7 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1756,7 +1756,7 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69179: [Format] Add format check for coroutine keywords with negative numbers

[Jonathan Thomas via Phabricator via cfe-commits]

jonathoma created this revision.
Herald added subscribers: cfe-commits, modocache.
Herald added a project: clang.
jonathoma retitled this revision from "[Format] Add format check for throwing 
negative numbers" to "[Format] Add format check for coroutine keywords with 
negative numbers".
jonathoma edited the summary of this revision.
jonathoma abandoned this revision.

As a followup to D69144 , this diff fixes the 
coroutine keyword spacing
for `co_yield` / `co_return`ing negative numbers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69179

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
@@ -6925,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1693,7 +1693,8 @@
 if (PrevToken->isOneOf(tok::l_paren, tok::l_square, tok::l_brace,
tok::comma, tok::semi, tok::kw_return, tok::colon,
tok::equal, tok::kw_delete, tok::kw_sizeof,
-   tok::kw_throw) ||
+   tok::kw_throw, tok::kw_co_return,
+   tok::kw_co_yield) ||
 PrevToken->isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
TT_UnaryOperator, TT_CastRParen))
   return TT_UnaryOperator;
@@ -1756,7 +1757,7 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
@@ -6925,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1693,7 +1693,8 @@
 if (PrevToken->isOneOf(tok::l_paren, tok::l_square, tok::l_brace,
tok::comma, tok::semi, tok::kw_return, tok::colon,
tok::equal, tok::kw_delete, tok::kw_sizeof,
-   tok::kw_throw) ||
+   tok::kw_throw, tok::kw_co_return,
+   tok::kw_co_yield) ||
 PrevToken->isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
TT_UnaryOperator, TT_CastRParen))
   return TT_UnaryOperator;
@@ -1756,7 +1757,7 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Jonathan Thomas via Phabricator via cfe-commits]

jonathoma created this revision.
jonathoma added reviewers: modocache, sammccall, Quuxplusone.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
jonathoma planned changes to this revision.

As a followup to D69144 , this diff fixes the 
coroutine keyword spacing
for co_yield / co_returning negative numbers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69180

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
@@ -6925,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1756,7 +1756,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
@@ -6925,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1756,7 +1756,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Jonathan Thomas via Phabricator via cfe-commits]

jonathoma updated this revision to Diff 225672.
jonathoma added a comment.

Rebase properly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69180

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6926,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1757,7 +1757,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6926,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1757,7 +1757,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Jonathan Thomas via Phabricator via cfe-commits]

jonathoma updated this revision to Diff 225671.
jonathoma added a comment.

Rebase onto master


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69180

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
@@ -6925,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1756,7 +1756,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6912,6 +6912,7 @@
   verifyFormat("alignof(char);", getGoogleStyle());
 
   verifyFormat("return -1;");
+  verifyFormat("throw -1;");
   verifyFormat("switch (a) {\n"
"case -1:\n"
"  break;\n"
@@ -6925,6 +6926,9 @@
   verifyFormat("int a = /* confusing comment */ -1;");
   // FIXME: The space after 'i' is wrong, but hopefully, this is a rare case.
   verifyFormat("int a = i /* confusing comment */++;");
+
+  verifyFormat("co_yield -1;");
+  verifyFormat("co_return -1;");
 }
 
 TEST_F(FormatTest, DoesNotIndentRelativeToUnaryOperators) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1756,7 +1756,8 @@
 // Use heuristics to recognize unary operators.
 if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
tok::question, tok::colon, tok::kw_return,
-   tok::kw_case, tok::at, tok::l_brace))
+   tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
+   tok::kw_co_return, tok_kw_co_yield))
   return TT_UnaryOperator;
 
 // There can't be two consecutive binary operators.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155145: [X86] Add AVX-VNNI-INT16 instructions.

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

We see a crash bisected to this patch about using an illegal instruction. 
Here's the CPUInfo for the machine:

  CPU info:
  current cpu id: 22
  total 32(physical cores 16) (assigned logical cores 32) (assigned physical 
cores 16) (assigned_sockets:2 of 2) (8 cores per cpu, 2 threads per core) 
family 6 model 45 stepping 7 microcode 0x71a, cmov, cx8, fxsr, mmx, sse, sse2, 
sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper, avx, aes, clmul, ht, tsc, 
tscinvbit, tscinv, clflush
  AvgLoads: 0.30, 0.10, 0.18
  CPU Model and flags from /proc/cpuinfo:
  model name  : Intel(R) Xeon(R) CPU E5-2650 0 @ 2.00GHz
  flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx 
pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology 
nonstop_tsc cpuid aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est 
tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt 
tsc_deadline_timer aes xsave avx lahf_lm epb pti ssbd ibrs ibpb stibp 
tpr_shadow vnmi flexpriority ept vpid xsaveopt dtherm ida arat pln pts md_clear 
flush_l1d
  Online cpus: 0-31
  Offline cpus:
  BIOS frequency limitation: 
  Frequency switch latency (ns): 2
  Available cpu frequencies: 
  Current governor: schedutil
  Core performance/turbo boost: 

I don't see `avxvnniint16` in the flags list nor avx2. So, this (relatively 
new) instruction shouldn't be generated for this machine. Any ideas on why this 
might be happening?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155145

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

[PATCH] D155145: [X86] Add AVX-VNNI-INT16 instructions.

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

In D155145#4544068 , @pengfei wrote:

> In D155145#4543326 , @anna wrote:
>
>> We see a crash bisected to this patch about using an illegal instruction. 
>> Here's the CPUInfo for the machine:
>>
>>   CPU info:
>>   current cpu id: 22
>>   total 32(physical cores 16) (assigned logical cores 32) (assigned physical 
>> cores 16) (assigned_sockets:2 of 2) (8 cores per cpu, 2 threads per core) 
>> family 6 model 45 stepping 7 microcode 0x71a, cmov, cx8, fxsr, mmx, sse, 
>> sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper, avx, aes, clmul, ht, 
>> tsc, tscinvbit, tscinv, clflush
>>   AvgLoads: 0.30, 0.10, 0.18
>>   CPU Model and flags from /proc/cpuinfo:
>>   model name  : Intel(R) Xeon(R) CPU E5-2650 0 @ 2.00GHz
>>   flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
>> cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx 
>> pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology 
>> nonstop_tsc cpuid aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est 
>> tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt 
>> tsc_deadline_timer aes xsave avx lahf_lm epb pti ssbd ibrs ibpb stibp 
>> tpr_shadow vnmi flexpriority ept vpid xsaveopt dtherm ida arat pln pts 
>> md_clear flush_l1d
>>   Online cpus: 0-31
>>   Offline cpus:
>>   BIOS frequency limitation: 
>>   Frequency switch latency (ns): 2
>>   Available cpu frequencies: 
>>   Current governor: schedutil
>>   Core performance/turbo boost: 
>>
>> I don't see `avxvnniint16` in the flags list nor avx2. So, this (relatively 
>> new) instruction shouldn't be generated for this machine. Any ideas on why 
>> this might be happening?
>
> As far as I can see from the patch, the only way to generate avxvnniint16 
> instructions is to call its specific intrinsics explicitly. And we will check 
> compiling options in FE before allowing to call the intrinsics. We do have an 
> optimization to generate vnni instructions without intrinsics, but we haven't 
> extend it to avxvnniint16 so far.
> So I don't know what's wrong in your case, could you provide a reproducer for 
> your problem?

I've investigated what is going on. With this patch, we are now passing in 
`+avxvnniint16` into machine attributes. With that attribute, we now generate 
an instruction which is illegal on sandybridge machine:

   0x3013f2af:  jmpq   0x3013f09b
 0x3013f2b4:mov%rax,%rdi
 0x3013f2b7:and$0xfff0,%rdi
  => 0x3013f2bb:vpbroadcastd %xmm0,%ymm2
 0x3013f2c0:vpbroadcastd %xmm1,%ymm3

The instruction `vpbroadcastd %xmm0,%ymm2` requires `AVX2` CPU flag: 
https://www.felixcloutier.com/x86/vpbroadcast. However, the machine has only 
AVX flag.

This is the complete mattr generated:

  !3 = 
!{!"-mattr=-prfchw,-cldemote,+avx,+aes,+sahf,+pclmul,-xop,+crc32,-xsaves,-avx512fp16,-sm4,+sse4.1,-avx512ifma,+xsave,-avx512pf,+sse4.2,-tsxldtrk,-ptwrite,-widekl,-sm3,-invpcid,+64bit,-xsavec,-avx512vpopcntdq,+cmov,-avx512vp2intersect,-avx512cd,-movbe,-avxvnniint8,-avx512er,-amx-int8,-kl,-sha512,-avxvnni,-rtm,-adx,-avx2,-hreset,-movdiri,-serialize,-vpclmulqdq,-avx512vl,-uintr,-clflushopt,-raoint,-cmpccxadd,-bmi,-amx-tile,+sse,-gfni,+avxvnniint16,-amx-fp16,+xsaveopt,-rdrnd,-avx512f,-amx-bf16,-avx512bf16,-avx512vnni,+cx8,-avx512bw,+sse3,-pku,-fsgsbase,-clzero,-mwaitx,-lwp,-lzcnt,-sha,-movdir64b,-wbnoinvd,-enqcmd,-prefetchwt1,-avxneconvert,-tbm,-pconfig,-amx-complex,+ssse3,+cx16,-bmi2,-fma,+popcnt,-avxifma,-f16c,-avx512bitalg,-rdpru,-clwb,+mmx,+sse2,-rdseed,-avx512vbmi2,-prefetchi,-rdpid,-fma4,-avx512vbmi,-shstk,-vaes,-waitpkg,-sgx,+fxsr,-avx512dq,-sse4a"}

I've confirmed if we changed to `-avxvnniint16` we do not generate 
`vpbroadcastd`.

W.r.t. how we get the machine attributes generated through our front-end:

  if (!sys::getHostCPUFeatures(Features))
return std::move(mattr);

  // Fill mattr with default values.
  mattr.reserve(Features.getNumItems());
  for (auto &I : Features) {
std::string attr(I.first());
mattr.emplace_back(std::string(I.second ? "+" : "-") + attr);
  }

So, the problem is in getHostCPUFeatures, possibly this line from the patch : 
`Features["avxvnniint16"] = HasLeaf7Subleaf1 && ((EDX >> 10) & 1) && 
HasAVXSave;`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155145

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

[PATCH] D155145: [X86] Add AVX-VNNI-INT16 instructions.

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

In D155145#4551621 , @craig.topper 
wrote:

> In D155145#4551526 , @anna wrote:
>
>> In D155145#4544068 , @pengfei 
>> wrote:
>>
>>> In D155145#4543326 , @anna wrote:
>>>
 We see a crash bisected to this patch about using an illegal instruction. 
 Here's the CPUInfo for the machine:

   CPU info:
   current cpu id: 22
   total 32(physical cores 16) (assigned logical cores 32) (assigned 
 physical cores 16) (assigned_sockets:2 of 2) (8 cores per cpu, 2 threads 
 per core) family 6 model 45 stepping 7 microcode 0x71a, cmov, cx8, fxsr, 
 mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper, avx, aes, 
 clmul, ht, tsc, tscinvbit, tscinv, clflush
   AvgLoads: 0.30, 0.10, 0.18
   CPU Model and flags from /proc/cpuinfo:
   model name  : Intel(R) Xeon(R) CPU E5-2650 0 @ 2.00GHz
   flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge 
 mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall 
 nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl 
 xtopology nonstop_tsc cpuid aperfmperf pni pclmulqdq dtes64 monitor ds_cpl 
 vmx smx est tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic popcnt 
 tsc_deadline_timer aes xsave avx lahf_lm epb pti ssbd ibrs ibpb stibp 
 tpr_shadow vnmi flexpriority ept vpid xsaveopt dtherm ida arat pln pts 
 md_clear flush_l1d
   Online cpus: 0-31
   Offline cpus:
   BIOS frequency limitation: 
   Frequency switch latency (ns): 2
   Available cpu frequencies: 
   Current governor: schedutil
   Core performance/turbo boost: 

 I don't see `avxvnniint16` in the flags list nor avx2. So, this 
 (relatively new) instruction shouldn't be generated for this machine. Any 
 ideas on why this might be happening?
>>>
>>> As far as I can see from the patch, the only way to generate avxvnniint16 
>>> instructions is to call its specific intrinsics explicitly. And we will 
>>> check compiling options in FE before allowing to call the intrinsics. We do 
>>> have an optimization to generate vnni instructions without intrinsics, but 
>>> we haven't extend it to avxvnniint16 so far.
>>> So I don't know what's wrong in your case, could you provide a reproducer 
>>> for your problem?
>>
>> I've investigated what is going on. With this patch, we are now passing in 
>> `+avxvnniint16` into machine attributes. With that attribute, we now 
>> generate an instruction which is illegal on sandybridge machine:
>>
>>0x3013f2af:   jmpq   0x3013f09b
>>  0x3013f2b4: mov%rax,%rdi
>>  0x3013f2b7: and$0xfff0,%rdi
>>   => 0x3013f2bb: vpbroadcastd %xmm0,%ymm2
>>  0x3013f2c0: vpbroadcastd %xmm1,%ymm3
>>
>> The instruction `vpbroadcastd %xmm0,%ymm2` requires `AVX2` CPU flag: 
>> https://www.felixcloutier.com/x86/vpbroadcast. However, the machine has only 
>> AVX flag.
>>
>> This is the complete mattr generated:
>>
>>   !3 = 
>> !{!"-mattr=-prfchw,-cldemote,+avx,+aes,+sahf,+pclmul,-xop,+crc32,-xsaves,-avx512fp16,-sm4,+sse4.1,-avx512ifma,+xsave,-avx512pf,+sse4.2,-tsxldtrk,-ptwrite,-widekl,-sm3,-invpcid,+64bit,-xsavec,-avx512vpopcntdq,+cmov,-avx512vp2intersect,-avx512cd,-movbe,-avxvnniint8,-avx512er,-amx-int8,-kl,-sha512,-avxvnni,-rtm,-adx,-avx2,-hreset,-movdiri,-serialize,-vpclmulqdq,-avx512vl,-uintr,-clflushopt,-raoint,-cmpccxadd,-bmi,-amx-tile,+sse,-gfni,+avxvnniint16,-amx-fp16,+xsaveopt,-rdrnd,-avx512f,-amx-bf16,-avx512bf16,-avx512vnni,+cx8,-avx512bw,+sse3,-pku,-fsgsbase,-clzero,-mwaitx,-lwp,-lzcnt,-sha,-movdir64b,-wbnoinvd,-enqcmd,-prefetchwt1,-avxneconvert,-tbm,-pconfig,-amx-complex,+ssse3,+cx16,-bmi2,-fma,+popcnt,-avxifma,-f16c,-avx512bitalg,-rdpru,-clwb,+mmx,+sse2,-rdseed,-avx512vbmi2,-prefetchi,-rdpid,-fma4,-avx512vbmi,-shstk,-vaes,-waitpkg,-sgx,+fxsr,-avx512dq,-sse4a"}
>>
>> I've confirmed if we changed to `-avxvnniint16` we do not generate 
>> `vpbroadcastd`.
>>
>> W.r.t. how we get the machine attributes generated through our front-end:
>>
>>   if (!sys::getHostCPUFeatures(Features))
>> return std::move(mattr);
>> 
>>   // Fill mattr with default values.
>>   mattr.reserve(Features.getNumItems());
>>   for (auto &I : Features) {
>> std::string attr(I.first());
>> mattr.emplace_back(std::string(I.second ? "+" : "-") + attr);
>>   }
>>
>> So, the problem is in getHostCPUFeatures, possibly this line from the patch 
>> : 
>> `Features["avxvnniint16"] = HasLeaf7Subleaf1 && ((EDX >> 10) & 1) && 
>> HasAVXSave;`.
>
> Does this patch help
>
>   diff --git a/llvm/lib/TargetParser/Host.cpp b/llvm/lib/TargetParser/Host.cpp
>   index 1141df09307c..11a6879fb76a 100644
>   --- a/llvm/lib/TargetParser/Host.cpp
>   +++ b/llvm/lib/TargetParser/Host

[PATCH] D155145: [X86] Add AVX-VNNI-INT16 instructions.

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

> Can you capture the values of EAX, EBX, ECX, and EDX after the two calls to 
> getX86CpuIDAndInfoEx that have 0x7 as the first argument? Maybe there's a bug 
> in CPUID on Sandy Bridge.

Sure, on the original code before the patch you suggested right?
The two calls are:

   bool HasLeaf7 =
  MaxLevel >= 7 && !getX86CpuIDAndInfoEx(0x7, 0x0, &EAX, &EBX, &ECX, 
&EDX);
  +   llvm::errs() << "Before setting fsgsbase the value for EAX: " << EAX
  + << " EBX: " << EBX << " ECX: " << ECX << "  EDX: " << 
EDX
  + << "\n";

  Features["fsgsbase"]   = HasLeaf7 && ((EBX >>  0) & 1);
  
  bool HasLeaf7Subleaf1 =
  MaxLevel >= 7 && !getX86CpuIDAndInfoEx(0x7, 0x1, &EAX, &EBX, &ECX, 
&EDX);
  +   llvm::errs() << "Before setting sha512 the value for EAX: " << EAX
  + << " EBX: " << EBX << " ECX: " << ECX << "  EDX: " << 
EDX
  + << "\n";
  Features["sha512"] = HasLeaf7Subleaf1 && ((EAX >> 0) & 1);
  ...
  we set avxvnniint16 after this

Takes a while to get a build on this machine, should have the output soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155145

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

[PATCH] D155145: [X86] Add AVX-VNNI-INT16 instructions.

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

In D155145#4554786 , @anna wrote:

>> Can you capture the values of EAX, EBX, ECX, and EDX after the two calls to 
>> getX86CpuIDAndInfoEx that have 0x7 as the first argument? Maybe there's a 
>> bug in CPUID on Sandy Bridge.
>
> Sure, on the original code before the patch you suggested right?
> The two calls are:
>
>bool HasLeaf7 =
>   MaxLevel >= 7 && !getX86CpuIDAndInfoEx(0x7, 0x0, &EAX, &EBX, &ECX, 
> &EDX);
>   +   llvm::errs() << "Before setting fsgsbase the value for EAX: " << EAX
>   + << " EBX: " << EBX << " ECX: " << ECX << "  EDX: " << 
> EDX
>   + << "\n";
> 
>   Features["fsgsbase"]   = HasLeaf7 && ((EBX >>  0) & 1);
>   
>   bool HasLeaf7Subleaf1 =
>   MaxLevel >= 7 && !getX86CpuIDAndInfoEx(0x7, 0x1, &EAX, &EBX, &ECX, 
> &EDX);
>   +   llvm::errs() << "Before setting sha512 the value for EAX: " << EAX
>   + << " EBX: " << EBX << " ECX: " << ECX << "  EDX: " << 
> EDX
>   + << "\n";
>   Features["sha512"] = HasLeaf7Subleaf1 && ((EAX >> 0) & 1);
>   ...
>   we set avxvnniint16 after this
>
> Takes a while to get a build on this machine, should have the output soon.

@ctopper here is the output:

  Before setting fsgsbase the value for EAX: 0 EBX: 0 ECX: 0  EDX: 2617246720 
// this is after the HasLeaf7 calculation
  Before setting sha512 the value for EAX: 0 EBX: 0 ECX: 0  EDX: 2617246720 // 
this is after the HasLeaf7Subleaf1 calculation

So, with your patch `HasLeaf7Subleaf1` is 0 as EAX is 0. Pls let me know if you 
need  any additional diagnostics output (we actually lose access to the machine 
on friday, since it is being retired!).

> The documentation says that invalid subleaves of leaf 7 should return all 0s. 
> So we thought it was safe to check the bits of sub leaf 1 even if eax from 
> subleaf 0 doesn't say subleaf 1 is supported.

This means the CPUID doesn't satisfy the documentation since EDX != 0 for 
SubLeaf1?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155145

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

[PATCH] D155145: [X86] Add AVX-VNNI-INT16 instructions.

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

thank you @craig.topper  and @pengfei .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155145

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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked an inline comment as done.
anna added inline comments.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1172
+  if (NumInstChecked++ > MaxInstCheckedForThrow ||
+  isGuaranteedToTransferExecutionToSuccessor(&I))
+return true;

Noticed while adding couple more tests, there are 2 bugs here:
1. the `isGuaranteedToTransferExecutionToSuccessor` check should be inverted
2.  make_range should be until the return instruction - so we do not want 
`std::prev` on the returnInstruction. what's needed is: 
`make_range(RVal->getIterator(), RInst->getIterator())`
This means that from the callsite until (and excluding) the return instruction 
should be guaranteed to transfer execution to successor - only then we can 
backward propagate the attribute to that callsite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 251901.
anna added a comment.

Noticed while adding couple more tests, there were 2 bugs:

1 the isGuaranteedToTransferExecutionToSuccessor check should be inverted

2. make_range should be until the return instruction - so we do not want 
std::prev on the returnInstruction. what's needed is: 
make_range(RVal->getIterator(), RInst->getIterator())

This means that from the callsite until (and excluding) the return instruction 
should be guaranteed to transfer execution to successor - only then we can 
backward propagate the attribute to that callsite.
Updated patch and added test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  clang/test/CodeGen/builtins-systemz-zvector.c
  clang/test/CodeGen/builtins-systemz-zvector2.c
  clang/test/CodeGen/movbe-builtins.c
  clang/test/CodeGen/rot-intrinsics.c
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,112 @@
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK: @callee(
+; CHECK: call i8* @foo(i8* noalias %p)
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller
+; CHECK: call nonnull i8* @foo(i8* noalias
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_throwable
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_explicit_control_flow
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2
+; CHECK: call i8* @foo
+; CHECK: call i8* @bar
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+; CHECK-NOT: callee3
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: caller3
+; CHECK: call noalias dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_sideeffect_callsite
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test4
+; CHECK: call i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee_with_sideeffect_callsite(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @baz(i8*) nounwind readonly
+define internal i8* @callee5(i8* %p) alwaysinline {
+; CHECK-NOT: callee5
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @baz(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to foo.
+define i8* @test5(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test5
+; CHECK: call dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %s = call dereferenceable_or_null(12) i8* @callee5(i8* %gep)
+  ret i8* %s
+}
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -80,11 +80,21 @@
   cl::Hidden,
   cl::desc("Convert noalias attributes to metadata during inlining."));
 
+static cl::opt UpdateReturnAttributes(
+"update-return-attrs", cl::init(true), cl::Hidden,
+cl::desc("Update r

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked an inline comment as done.
anna added inline comments.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1172
+  if (NumInstChecked++ > MaxInstCheckedForThrow ||
+  isGuaranteedToTransferExecutionToSuccessor(&I))
+return true;

lebedev.ri wrote:
> anna wrote:
> > Noticed while adding couple more tests, there are 2 bugs here:
> > 1. the `isGuaranteedToTransferExecutionToSuccessor` check should be inverted
> > 2.  make_range should be until the return instruction - so we do not want 
> > `std::prev` on the returnInstruction. what's needed is: 
> > `make_range(RVal->getIterator(), RInst->getIterator())`
> > This means that from the callsite until (and excluding) the return 
> > instruction should be guaranteed to transfer execution to successor - only 
> > then we can backward propagate the attribute to that callsite.
> Are you aware of `llvm::isValidAssumeForContext()`?
> All this (including pitfalls) sound awfully close to that function.
as stated in a previous comment (https://reviews.llvm.org/D76140#1922292), 
adding `Assumes` here for simple cases seems like an overkill. It has 
significant IR churn and it also adds a use for something which can be easily 
inferred. 
Consider optimizations that depend on facts such as `hasOneUse` or a limited 
number of uses. We will now be inhibiting those optimizations. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked an inline comment as done.
anna added inline comments.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1172
+  if (NumInstChecked++ > MaxInstCheckedForThrow ||
+  isGuaranteedToTransferExecutionToSuccessor(&I))
+return true;

lebedev.ri wrote:
> anna wrote:
> > lebedev.ri wrote:
> > > anna wrote:
> > > > Noticed while adding couple more tests, there are 2 bugs here:
> > > > 1. the `isGuaranteedToTransferExecutionToSuccessor` check should be 
> > > > inverted
> > > > 2.  make_range should be until the return instruction - so we do not 
> > > > want `std::prev` on the returnInstruction. what's needed is: 
> > > > `make_range(RVal->getIterator(), RInst->getIterator())`
> > > > This means that from the callsite until (and excluding) the return 
> > > > instruction should be guaranteed to transfer execution to successor - 
> > > > only then we can backward propagate the attribute to that callsite.
> > > Are you aware of `llvm::isValidAssumeForContext()`?
> > > All this (including pitfalls) sound awfully close to that function.
> > as stated in a previous comment (https://reviews.llvm.org/D76140#1922292), 
> > adding `Assumes` here for simple cases seems like an overkill. It has 
> > significant IR churn and it also adds a use for something which can be 
> > easily inferred. 
> > Consider optimizations that depend on facts such as `hasOneUse` or a 
> > limited number of uses. We will now be inhibiting those optimizations. 
> While i venomously disagree with the avoidance of the usage of one versatile 
> interface
> and hope things will change once there's more progress on attributor & assume 
> bundles,
> in this case, as it can be seen even from the signature of the 
> `isValidAssumeForContext()` function,
> it implies/forces nothing about using assumes, but only performs a validity 
> checking,
> similar to the `MayContainThrowingOrExitingCall()`
> 
> https://github.com/llvm/llvm-project/blob/ca04d0c8fd269978be1c13fe1241172cdfe6a6ea/llvm/lib/Analysis/ValueTracking.cpp#L603
> 
> That being said, i haven't reviewed this code so maybe there's some 
> differences here
> that make that function unapplicable here.
> 
`isValidAssumeForContext(Inv, CxtI, DT)` does not force anything about assumes, 
but AFAICT all code which uses this function either has some sort of guard in 
the caller that the instruction is an assume. Also, the comments in the code 
state that it is for an assume. In fact, I believe if we intend to use that 
function more widely for other purposes, we should rename the function before 
using it (just a thought), and currently we should assert that `Inv` is an 
assume. It captures the intent of the function.

That being said, I checked the code in `isValidAssumeForContext` and it does 
not fit the bill here for multiple reasons. We either do:
1. `isValidAssumeForContext(RVal /* Inv */, RInst /* CxtI  */)` which fails 
when we do not have DT and just return true when RVal comes before RInst - this 
is always the case, since RVal will come before RInst.
2. `isValidAssumeForContext(RInst /* Inv*/, RVal /* CxtI*/)` and it fails at 
the `!isEphemeralValueOf(Inv /* RI */, CxtI /* RV*/)` check.  
(By fail here, I mean, it does not have the same behaviour as 
`MayContainThrowingOrExitingCall`).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 252868.
anna added a comment.

NFC w.r.t prev diff. Use VMap.lookup instead of a lambda which does the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  clang/test/CodeGen/builtins-systemz-zvector.c
  clang/test/CodeGen/builtins-systemz-zvector2.c
  clang/test/CodeGen/movbe-builtins.c
  clang/test/CodeGen/rot-intrinsics.c
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,112 @@
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK: @callee(
+; CHECK: call i8* @foo(i8* noalias %p)
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller
+; CHECK: call nonnull i8* @foo(i8* noalias
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_throwable
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_explicit_control_flow
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2
+; CHECK: call i8* @foo
+; CHECK: call i8* @bar
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+; CHECK-NOT: callee3
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: caller3
+; CHECK: call noalias dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_sideeffect_callsite
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test4
+; CHECK: call i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee_with_sideeffect_callsite(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @baz(i8*) nounwind readonly
+define internal i8* @callee5(i8* %p) alwaysinline {
+; CHECK-NOT: callee5
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @baz(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to foo.
+define i8* @test5(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test5
+; CHECK: call dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %s = call dereferenceable_or_null(12) i8* @callee5(i8* %gep)
+  ret i8* %s
+}
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -80,11 +80,21 @@
   cl::Hidden,
   cl::desc("Convert noalias attributes to metadata during inlining."));
 
+static cl::opt UpdateReturnAttributes(
+"update-return-attrs", cl::init(true), cl::Hidden,
+cl::desc("Update return attributes on calls within inlined body"));
+
 static cl::opt
 PreserveAlignmentAssumptions("preserve-alignment-assumptions-during-inlining",
   cl::init(true), cl::Hidden,
   cl::desc("Convert align attributes to assumptions during inlining."));
 
+static cl::opt MaxInstCheckedForThrow(
+"max-inst-checked-for-throw-during-inlining", cl::Hidden,
+cl::desc("the maximum number of instructions analyzed for may throw during "
+ "attribute inference in inline

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked 3 inline comments as done.
anna added inline comments.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1159
+
+  auto MayContainThrowingOrExitingCall = [&](Instruction *RVal,
+ Instruction *RInst) {

reames wrote:
> Pull this out as a static helper instead of a lambda, add an assert 
> internally that the two instructions are in the same block.  
> 
> Why?  Because I'm 80% sure the state capture on the lambda isn't needed, and 
> having it as a separate function forces that discipline.  
agreed. I'll do that in this change itself before landing. I am using this 
static helper in followon change D76792.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175
+  continue;
+// Sanity check that the cloned return instruction exists and is a return
+// instruction itself.

reames wrote:
> Ok, after staring at it a bit, I've convinced myself the code here is 
> correct, just needlessly conservative.
> 
> What you're doing is:
> If the callees return instruction and returned call both map to the same 
> instructions once inlined, determine whether there's a possible exit between 
> the inlined copy.
> 
> What you could be doing:
> If the callee returns a call, check if *in the callee* there's a possible 
> exit between call and return, then apply attribute to cloned call.
> 
> The key difference is when the caller directly returns the result vs uses it 
> locally.  The result here is that your transform is much more narrow in 
> applicability than it first appears.
yes, thanks for pointing it out. I realized it after our offline discussion :) 
For now, I will add a FIXME testcase which showcases the difference in code and 
handle that testcase in a followon change. 



Comment at: llvm/test/Transforms/Inline/ret_attr_update.ll:112
+  ret i8* %s
+}

reames wrote:
> There's a critical missing test case here:
> - Callee and caller have the same attributes w/different values (i.e. deref)
> 
> And thinking through the code, I think there might be a bug here.  It's not a 
> serious one, but the if the callee specifies a larger deref than the caller, 
> it looks like the the smaller value is being written over the larger.
> 
> Actually, digging through the attribute code, I think I'm wrong about the 
> bug.  However, you should definitely write the test to confirm and document 
> merging behaviour!
> 
> If it does turn out I'm correct, I'm fine with this being addressed in a 
> follow up patch provided that the test is added in this one and isn't a 
> functional issue.  
will check this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 253704.
anna added a comment.

addressed review comments. Added two test cases: deref value being different, 
inlined callee body better optimized compared to callee.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  clang/test/CodeGen/builtins-systemz-zvector.c
  clang/test/CodeGen/builtins-systemz-zvector2.c
  clang/test/CodeGen/movbe-builtins.c
  clang/test/CodeGen/rot-intrinsics.c
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,159 @@
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK: @callee(
+; CHECK: call i8* @foo(i8* noalias %p)
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller
+; CHECK: call nonnull i8* @foo(i8* noalias
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_throwable
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_explicit_control_flow
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2
+; CHECK: call i8* @foo
+; CHECK: call i8* @bar
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+; CHECK-NOT: callee3
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: caller3
+; CHECK: call noalias dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_sideeffect_callsite
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test4
+; CHECK: call i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee_with_sideeffect_callsite(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @baz(i8*) nounwind readonly
+define internal i8* @callee5(i8* %p) alwaysinline {
+; CHECK-NOT: callee5
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @baz(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to foo.
+define i8* @test5(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test5
+; CHECK: call dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %s = call dereferenceable_or_null(12) i8* @callee5(i8* %gep)
+  ret i8* %s
+}
+
+; deref attributes have different values on the callee and the call feeding into
+; the return.
+; AttrBuilder chooses the already existing value and does not overwrite it.
+define internal i8* @callee6(i8* %p) alwaysinline {
+; CHECK-NOT: callee6
+  %r = call dereferenceable_or_null(16) i8* @foo(i8* %p)
+  %v = call i8* @baz(i8* %p)
+  ret i8* %r
+}
+
+
+define i8* @test6(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test6
+; CHECK: call dereferenceable_or_null(16) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %s = call dereferenceable_or_null(12) i8* @callee6(i8* %gep)
+  ret i8* %s
+}
+
+; We add the attributes from the callee to both the calls below.
+define internal i8* @callee7(i8 *%ptr, i1 %cond) alwaysinline {
+; CHECK-NOT: @callee7(
+  br i1 %cond, label %pass, label %fail
+
+pass:
+  %r = call i8* @foo(i8* noalias %ptr)
+  ret i8* %r
+
+f

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked 3 inline comments as done.
anna added inline comments.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175
+  continue;
+// Sanity check that the cloned return instruction exists and is a return
+// instruction itself.

anna wrote:
> reames wrote:
> > Ok, after staring at it a bit, I've convinced myself the code here is 
> > correct, just needlessly conservative.
> > 
> > What you're doing is:
> > If the callees return instruction and returned call both map to the same 
> > instructions once inlined, determine whether there's a possible exit 
> > between the inlined copy.
> > 
> > What you could be doing:
> > If the callee returns a call, check if *in the callee* there's a possible 
> > exit between call and return, then apply attribute to cloned call.
> > 
> > The key difference is when the caller directly returns the result vs uses 
> > it locally.  The result here is that your transform is much more narrow in 
> > applicability than it first appears.
> yes, thanks for pointing it out. I realized it after our offline discussion 
> :) 
> For now, I will add a FIXME testcase which showcases the difference in code 
> and handle that testcase in a followon change. 
> The key difference is when the caller directly returns the result vs uses it 
> locally. The result here is that your transform is much more narrow in 
> applicability than it first appears.
I tried multiple test cases to showcase the difference between the two ideas 
above but failed. Specifically, `simplifyInstruction` used during inlining the 
callee is not too great at optimizing the body. For example, see added testcase 
`test7`. 

I also tried the less restrictive version (check the safety of the optimization 
in the callee itself, and do the attribute update on the cloned instruction), 
but didn't see any testcases in clang that needed update. Of course, that 
doesn't mean anything :)





Comment at: llvm/test/Transforms/Inline/ret_attr_update.ll:112
+  ret i8* %s
+}

anna wrote:
> reames wrote:
> > There's a critical missing test case here:
> > - Callee and caller have the same attributes w/different values (i.e. deref)
> > 
> > And thinking through the code, I think there might be a bug here.  It's not 
> > a serious one, but the if the callee specifies a larger deref than the 
> > caller, it looks like the the smaller value is being written over the 
> > larger.
> > 
> > Actually, digging through the attribute code, I think I'm wrong about the 
> > bug.  However, you should definitely write the test to confirm and document 
> > merging behaviour!
> > 
> > If it does turn out I'm correct, I'm fine with this being addressed in a 
> > follow up patch provided that the test is added in this one and isn't a 
> > functional issue.  
> will check this.
added test case and documented merge behaviour. No bug in code, since we use 
the already existing value on attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG28518d9ae39f: [InlineFunction] Handle return attributes on 
call within inlined body (authored by anna).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  clang/test/CodeGen/builtins-systemz-zvector.c
  clang/test/CodeGen/builtins-systemz-zvector2.c
  clang/test/CodeGen/movbe-builtins.c
  clang/test/CodeGen/rot-intrinsics.c
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,159 @@
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK: @callee(
+; CHECK: call i8* @foo(i8* noalias %p)
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller
+; CHECK: call nonnull i8* @foo(i8* noalias
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_throwable
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_explicit_control_flow
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2
+; CHECK: call i8* @foo
+; CHECK: call i8* @bar
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+; CHECK-NOT: callee3
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: caller3
+; CHECK: call noalias dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_sideeffect_callsite
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test4
+; CHECK: call i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee_with_sideeffect_callsite(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @baz(i8*) nounwind readonly
+define internal i8* @callee5(i8* %p) alwaysinline {
+; CHECK-NOT: callee5
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @baz(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to foo.
+define i8* @test5(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test5
+; CHECK: call dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %s = call dereferenceable_or_null(12) i8* @callee5(i8* %gep)
+  ret i8* %s
+}
+
+; deref attributes have different values on the callee and the call feeding into
+; the return.
+; AttrBuilder chooses the already existing value and does not overwrite it.
+define internal i8* @callee6(i8* %p) alwaysinline {
+; CHECK-NOT: callee6
+  %r = call dereferenceable_or_null(16) i8* @foo(i8* %p)
+  %v = call i8* @baz(i8* %p)
+  ret i8* %r
+}
+
+
+define i8* @test6(i8* %ptr, i64 %x) {
+; CHECK-LABEL: test6
+; CHECK: call dereferenceable_or_null(16) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %s = call dereferenceable_or_null(12) i8* @callee6(i8* %gep)
+  ret i8* %s
+}
+
+; We add the attributes from the callee to both the calls below.
+define internal i8* @callee7(i8 *%ptr, i1 %cond) alwaysinline {
+; CHECK-NOT: @callee7(
+  br i1 %cond, label %pass, label %fail
+
+pass:
+  %r = call i8* @foo(i8* noalias %ptr)
+  ret i8* %r
+
+fail:

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

I got a failure in one of the binaryFormats:
lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/MsgPackReader.cpp

  Attributes 'zeroext and signext' are incompatible!
%rev.i.i.i.i.i.i.i.i = tail call signext zeroext i16 @llvm.bswap.i16(i16 
%ret.0.copyload.i.i.i.i) #6
  in function _ZN4llvm7msgpack6Reader7readIntIsEENS_8ExpectedIbEERNS0_6ObjectE
  fatal error: error in backend: Broken function found, compilation aborted!

I believe this just showcases undefined behaviour since we were having a 
returnValue (i.e. call) with an incomptable attribute compared to the return 
attribute on the callsite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna planned changes to this revision.
anna added a comment.

see above comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

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

In D76140#1953201 , @anna wrote:

> I got a failure in one of the binaryFormats:
>  lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/MsgPackReader.cpp
>
>   Attributes 'zeroext and signext' are incompatible!
> %rev.i.i.i.i.i.i.i.i = tail call signext zeroext i16 @llvm.bswap.i16(i16 
> %ret.0.copyload.i.i.i.i) #6
>   in function _ZN4llvm7msgpack6Reader7readIntIsEENS_8ExpectedIbEERNS0_6ObjectE
>   fatal error: error in backend: Broken function found, compilation aborted!
>
>
> I believe this just showcases undefined behaviour since we were having a 
> returnValue (i.e. call) with an incomptable attribute compared to the return 
> attribute on the callsite.


The last statement is not true. Had a discussion offline with Philip and he 
pointed out that we missed the fact that attributes such as `signext` and 
`zeroext` are part of the *call* itself. We cannot propagate these attributes 
into the callee since such attributes are part of the ABI for the call it is 
attached to. 
I'm reopening this review to fix this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked an inline comment as done.
anna added inline comments.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175
+  continue;
+// Sanity check that the cloned return instruction exists and is a return
+// instruction itself.

anna wrote:
> anna wrote:
> > reames wrote:
> > > Ok, after staring at it a bit, I've convinced myself the code here is 
> > > correct, just needlessly conservative.
> > > 
> > > What you're doing is:
> > > If the callees return instruction and returned call both map to the same 
> > > instructions once inlined, determine whether there's a possible exit 
> > > between the inlined copy.
> > > 
> > > What you could be doing:
> > > If the callee returns a call, check if *in the callee* there's a possible 
> > > exit between call and return, then apply attribute to cloned call.
> > > 
> > > The key difference is when the caller directly returns the result vs uses 
> > > it locally.  The result here is that your transform is much more narrow 
> > > in applicability than it first appears.
> > yes, thanks for pointing it out. I realized it after our offline discussion 
> > :) 
> > For now, I will add a FIXME testcase which showcases the difference in code 
> > and handle that testcase in a followon change. 
> > The key difference is when the caller directly returns the result vs uses 
> > it locally. The result here is that your transform is much more narrow in 
> > applicability than it first appears.
> I tried multiple test cases to showcase the difference between the two ideas 
> above but failed. Specifically, `simplifyInstruction` used during inlining 
> the callee is not too great at optimizing the body. For example, see added 
> testcase `test7`. 
> 
> I also tried the less restrictive version (check the safety of the 
> optimization in the callee itself, and do the attribute update on the cloned 
> instruction), but didn't see any testcases in clang that needed update. Of 
> course, that doesn't mean anything :)
> 
> 
Clarified this with Philip offline. The current patch is not restrictive. In 
fact, now that I think of it, sometimes, it may be better - 
`simplifyInstruction` can fold away instructions and reduce the "window size" 
between the RV and the ReturnInst.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

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

fixed buildbot failure. see above comments and added testcase `test8`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 254213.
anna added a comment.
This revision is now accepted and ready to land.

whitelist valid return attributes and only add those. Added testcase for 
signext.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,223 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK-LABEL: @callee(
+; CHECK-NEXT:[[R:%.*]] = call i8* @foo(i8* noalias [[P:%.*]])
+; CHECK-NEXT:ret i8* [[R]]
+;
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call nonnull i8* @foo(i8* noalias [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:[[COND_I:%.*]] = icmp ne i8* [[R_I]], null
+; CHECK-NEXT:call void (i1, ...) @llvm.experimental.guard(i1 [[COND_I]]) [ "deopt"() ]
+; CHECK-NEXT:[[R_I1:%.*]] = call i8* @bar(i8* [[GEP]])
+; CHECK-NEXT:[[COND_I2:%.*]] = icmp ne i8* [[R_I1]], null
+; CHECK-NEXT:br i1 [[COND_I2]], label [[RET_I:%.*]], label [[ORIG_I:%.*]]
+; CHECK:   ret.i:
+; CHECK-NEXT:br label [[CALLEE_WITH_EXPLICIT_CONTROL_FLOW_EXIT:%.*]]
+; CHECK:   orig.i:
+; CHECK-NEXT:br label [[CALLEE_WITH_EXPLICIT_CONTROL_FLOW_EXIT]]
+; CHECK:   callee_with_explicit_control_flow.exit:
+; CHECK-NEXT:[[Q3:%.*]] = phi i8* [ [[R_I1]], [[RET_I]] ], [ [[GEP]], [[ORIG_I]] ]
+; CHECK-NEXT:br i1 [[COND:%.*]], label [[PRET:%.*]], label [[QRET:%.*]]
+; CHECK:   pret:
+; CHECK-NEXT:ret i8* [[R_I]]
+; CHECK:   qret:
+; CHECK-NEXT:ret i8* [[Q3]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller3(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call noalias dereferenceable_or_null(12) i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @test4(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:[[V_I:%.*]] = call i8* @inf_loop_call(i8* [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee_with_sideeffect_callsite(i8* %gep)
+  ret i8* %p
+}

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 254222.
anna added a comment.

fixed missing code left out during rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,223 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK-LABEL: @callee(
+; CHECK-NEXT:[[R:%.*]] = call i8* @foo(i8* noalias [[P:%.*]])
+; CHECK-NEXT:ret i8* [[R]]
+;
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call nonnull i8* @foo(i8* noalias [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:[[COND_I:%.*]] = icmp ne i8* [[R_I]], null
+; CHECK-NEXT:call void (i1, ...) @llvm.experimental.guard(i1 [[COND_I]]) [ "deopt"() ]
+; CHECK-NEXT:[[R_I1:%.*]] = call i8* @bar(i8* [[GEP]])
+; CHECK-NEXT:[[COND_I2:%.*]] = icmp ne i8* [[R_I1]], null
+; CHECK-NEXT:br i1 [[COND_I2]], label [[RET_I:%.*]], label [[ORIG_I:%.*]]
+; CHECK:   ret.i:
+; CHECK-NEXT:br label [[CALLEE_WITH_EXPLICIT_CONTROL_FLOW_EXIT:%.*]]
+; CHECK:   orig.i:
+; CHECK-NEXT:br label [[CALLEE_WITH_EXPLICIT_CONTROL_FLOW_EXIT]]
+; CHECK:   callee_with_explicit_control_flow.exit:
+; CHECK-NEXT:[[Q3:%.*]] = phi i8* [ [[R_I1]], [[RET_I]] ], [ [[GEP]], [[ORIG_I]] ]
+; CHECK-NEXT:br i1 [[COND:%.*]], label [[PRET:%.*]], label [[QRET:%.*]]
+; CHECK:   pret:
+; CHECK-NEXT:ret i8* [[R_I]]
+; CHECK:   qret:
+; CHECK-NEXT:ret i8* [[Q3]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller3(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call noalias dereferenceable_or_null(12) i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @test4(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:[[V_I:%.*]] = call i8* @inf_loop_call(i8* [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee_with_sideeffect_callsite(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @baz(i8*) nounwind readonly
+define internal i8* @callee5(i8* %p) alwaysin

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna added a comment.

In D76140#1957416 , @reames wrote:

> LGTM again, with minor change.


will update it.

> p.s. Sorry for missing the functional issue the first time.  All of the test 
> changes should have made the issue obvious, but despite reading the LangRef 
> description of signext, I somehow managed to miss the separation between ABI 
> and optimization attributes.

thanks for the review Philip and pointing out the problem. All of us had missed 
the functional issue the first time around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbf7a16a76871: [InlineFunction] Update valid return 
attributes at callsite within callee body (authored by anna).

Changed prior to commit:
  https://reviews.llvm.org/D76140?vs=254222&id=254573#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,223 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*) argmemonly nounwind
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK-LABEL: @callee(
+; CHECK-NEXT:[[R:%.*]] = call i8* @foo(i8* noalias [[P:%.*]])
+; CHECK-NEXT:ret i8* [[R]]
+;
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call nonnull i8* @foo(i8* noalias [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo
+; because the guard is a throwing call
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+declare i8* @bar(i8*) readonly nounwind
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:[[COND_I:%.*]] = icmp ne i8* [[R_I]], null
+; CHECK-NEXT:call void (i1, ...) @llvm.experimental.guard(i1 [[COND_I]]) [ "deopt"() ]
+; CHECK-NEXT:[[R_I1:%.*]] = call i8* @bar(i8* [[GEP]])
+; CHECK-NEXT:[[COND_I2:%.*]] = icmp ne i8* [[R_I1]], null
+; CHECK-NEXT:br i1 [[COND_I2]], label [[RET_I:%.*]], label [[ORIG_I:%.*]]
+; CHECK:   ret.i:
+; CHECK-NEXT:br label [[CALLEE_WITH_EXPLICIT_CONTROL_FLOW_EXIT:%.*]]
+; CHECK:   orig.i:
+; CHECK-NEXT:br label [[CALLEE_WITH_EXPLICIT_CONTROL_FLOW_EXIT]]
+; CHECK:   callee_with_explicit_control_flow.exit:
+; CHECK-NEXT:[[Q3:%.*]] = phi i8* [ [[R_I1]], [[RET_I]] ], [ [[GEP]], [[ORIG_I]] ]
+; CHECK-NEXT:br i1 [[COND:%.*]], label [[PRET:%.*]], label [[QRET:%.*]]
+; CHECK:   pret:
+; CHECK-NEXT:ret i8* [[R_I]]
+; CHECK:   qret:
+; CHECK-NEXT:ret i8* [[Q3]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller3(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call noalias dereferenceable_or_null(12) i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
+
+declare i8* @inf_loop_call(i8*) nounwind
+; We cannot propagate attributes to foo because we do not know whether inf_loop_call
+; will return execution.
+define internal i8* @callee_with_sideeffect_callsite(i8* %p) alwaysinline {
+  %r = call i8* @foo(i8* %p)
+  %v = call i8* @inf_loop_call(i8* %p)
+  ret i8* %r
+}
+
+; do not add deref attribute to foo
+define i8* @test4(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @test4(
+; CHECK-NEXT:[[GEP:%.*]] = getelementptr inbounds i8, i8* [[PTR:%.*]], i64 [[X:%.*]]
+; CHECK-NEXT:[[R_I:%.*]] = call i8* @foo(i8* [[GEP]])
+; CHECK-NEXT:[[V_I:%.*]] = call i8* @inf_loop_call(i8* [[GEP]])
+; CHECK-NEXT:ret i8* [[R_I]]
+;
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call 

[PATCH] D69180: [Format] Add format check for coroutine keywords with negative numbers

[Jonathan Thomas via Phabricator via cfe-commits]

jonathoma added a comment.

@modocache Feel free to commit from my end! Thanks again :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69180



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 251447.
anna added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

fixed clang tests. rot-intrinsics.c testcase has 5 different RUNs with 3 
prefixes. Depending on target-triple, the attribute is added to the caller, so 
I've disabled the optimization for that specific test with 
-update-return-attrs=false


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  clang/test/CodeGen/builtins-systemz-zvector.c
  clang/test/CodeGen/builtins-systemz-zvector2.c
  clang/test/CodeGen/movbe-builtins.c
  clang/test/CodeGen/rot-intrinsics.c
  clang/test/CodeGen/waitpkg.c
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,75 @@
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*)
+declare i8* @bar(i8*)
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK: @callee(
+; CHECK: call i8* @foo(i8* noalias %p)
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller
+; CHECK: call nonnull i8* @foo(i8* noalias
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo.
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_throwable
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_explicit_control_flow
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2
+; CHECK: call i8* @foo
+; CHECK: call i8* @bar
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+; CHECK-NOT: callee3
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: caller3
+; CHECK: call noalias dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -80,11 +80,21 @@
   cl::Hidden,
   cl::desc("Convert noalias attributes to metadata during inlining."));
 
+static cl::opt UpdateReturnAttributes(
+"update-return-attrs", cl::init(true), cl::Hidden,
+cl::desc("Update return attributes on calls within inlined body"));
+
 static cl::opt
 PreserveAlignmentAssumptions("preserve-alignment-assumptions-during-inlining",
   cl::init(true), cl::Hidden,
   cl::desc("Convert align attributes to assumptions during inlining."));
 
+static cl::opt MaxInstCheckedForThrow(
+"max-inst-checked-for-throw-during-inlining", cl::Hidden,
+cl::desc("the maximum number of instructions analyzed for may throw during "
+ "attribute inference in inlined body"),
+cl::init(4));
+
 llvm::InlineResult llvm::InlineFunction(CallBase *CB, InlineFunctionInfo &IFI,
 AAResults *CalleeAAR,
 bool InsertLifetime) {
@@ -1136,6 +1146,77 @@
   }
 }
 
+static void AddReturnAttributes(CallSite CS, ValueToValueMapTy &VMap) {
+  if (!UpdateReturnAttributes)
+return;
+  AttrBuilder AB(CS.getAttributes(), AttributeList::ReturnIndex);
+  if (AB.empty())
+return;
+
+  auto *CalledFunction = CS.getCalledFunction();
+  auto &Context = CalledFunction->getContext();
+
+  auto GetClonedValue = [&](Instruction *I) -> Value * {
+ValueToValueMapTy::iterator VMI = VMap.find(I);
+if (VMI == VMap.end())
+  return nullptr;
+return VMI->second;
+  };
+
+  auto MayContainThrowingCall = [&](Instruction *RVal, Instruction *RInst) {
+unsigned NumInstChecked = 0;
+for (auto &I :
+ make_range(RVa

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna marked an inline comment as done.
anna added inline comments.
Herald added a reviewer: aartbik.



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1172
+return true;
+return false;
+  };

jdoerfert wrote:
> `mayThrow` is not sufficient. As with my earlier example, a potential `exit` 
> is sufficient to break this, thus you need `willreturn` as well.
What we need is just `isGuaranteedToTransferExecutionToSuccessor`. That handles 
`mayThrow`, exits/pthread_exit and willreturn.

Just to note, an unconditional exit in the callee itself is not an issue here. 
The problem is something like this:
```
;nothrow
foo(i8* %arg) {
if (%arg == null)
  exit;
ret %arg
}
callee() {
  %r = call i8* @bar
  %v = call i8* @foo(i8* %r)
   ret i8* %r
}
caller() {
  call nonnull i8* @callee
}
```
 Here propagating nonnull to callsite `bar` is incorrect since if %r is null, 
the program exits. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

anna updated this revision to Diff 251615.
anna added a comment.

use isGuaranteedToTransferExecutionToSuccessor instead of `MayThrow`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

Files:
  clang/test/CodeGen/builtins-systemz-zvector.c
  clang/test/CodeGen/builtins-systemz-zvector2.c
  clang/test/CodeGen/movbe-builtins.c
  clang/test/CodeGen/rot-intrinsics.c
  clang/test/CodeGen/waitpkg.c
  llvm/lib/Transforms/Utils/InlineFunction.cpp
  llvm/test/Transforms/Inline/ret_attr_update.ll

Index: llvm/test/Transforms/Inline/ret_attr_update.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/ret_attr_update.ll
@@ -0,0 +1,75 @@
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+; RUN: opt < %s -passes=always-inline -S | FileCheck %s
+
+declare i8* @foo(i8*)
+declare i8* @bar(i8*)
+
+define i8* @callee(i8 *%p) alwaysinline {
+; CHECK: @callee(
+; CHECK: call i8* @foo(i8* noalias %p)
+  %r = call i8* @foo(i8* noalias %p)
+  ret i8* %r
+}
+
+define i8* @caller(i8* %ptr, i64 %x) {
+; CHECK-LABEL: @caller
+; CHECK: call nonnull i8* @foo(i8* noalias
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee(i8* %gep)
+  ret i8* %p
+}
+
+declare void @llvm.experimental.guard(i1,...)
+; Cannot add nonnull attribute to foo.
+define internal i8* @callee_with_throwable(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_throwable
+  %r = call i8* @foo(i8* %p)
+  %cond = icmp ne i8* %r, null
+  call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
+  ret i8* %r
+}
+
+; Here also we cannot add nonnull attribute to the call bar.
+define internal i8* @callee_with_explicit_control_flow(i8* %p) alwaysinline {
+; CHECK-NOT: callee_with_explicit_control_flow
+  %r = call i8* @bar(i8* %p)
+  %cond = icmp ne i8* %r, null
+  br i1 %cond, label %ret, label %orig
+
+ret:
+  ret i8* %r
+
+orig:
+  ret i8* %p
+}
+
+define i8* @caller2(i8* %ptr, i64 %x, i1 %cond) {
+; CHECK-LABEL: @caller2
+; CHECK: call i8* @foo
+; CHECK: call i8* @bar
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call nonnull i8* @callee_with_throwable(i8* %gep)
+  %q = call nonnull i8* @callee_with_explicit_control_flow(i8* %gep)
+  br i1 %cond, label %pret, label %qret
+
+pret:
+  ret i8* %p
+
+qret:
+  ret i8* %q
+}
+
+define internal i8* @callee3(i8 *%p) alwaysinline {
+; CHECK-NOT: callee3
+  %r = call noalias i8* @foo(i8* %p)
+  ret i8* %r
+}
+
+; add the deref attribute to the existing attributes on foo.
+define i8* @caller3(i8* %ptr, i64 %x) {
+; CHECK-LABEL: caller3
+; CHECK: call noalias dereferenceable_or_null(12) i8* @foo
+  %gep = getelementptr inbounds i8, i8* %ptr, i64 %x
+  %p = call dereferenceable_or_null(12) i8* @callee3(i8* %gep)
+  ret i8* %p
+}
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -80,11 +80,21 @@
   cl::Hidden,
   cl::desc("Convert noalias attributes to metadata during inlining."));
 
+static cl::opt UpdateReturnAttributes(
+"update-return-attrs", cl::init(true), cl::Hidden,
+cl::desc("Update return attributes on calls within inlined body"));
+
 static cl::opt
 PreserveAlignmentAssumptions("preserve-alignment-assumptions-during-inlining",
   cl::init(true), cl::Hidden,
   cl::desc("Convert align attributes to assumptions during inlining."));
 
+static cl::opt MaxInstCheckedForThrow(
+"max-inst-checked-for-throw-during-inlining", cl::Hidden,
+cl::desc("the maximum number of instructions analyzed for may throw during "
+ "attribute inference in inlined body"),
+cl::init(4));
+
 llvm::InlineResult llvm::InlineFunction(CallBase *CB, InlineFunctionInfo &IFI,
 AAResults *CalleeAAR,
 bool InsertLifetime) {
@@ -1136,6 +1146,80 @@
   }
 }
 
+static void AddReturnAttributes(CallSite CS, ValueToValueMapTy &VMap) {
+  if (!UpdateReturnAttributes)
+return;
+  AttrBuilder AB(CS.getAttributes(), AttributeList::ReturnIndex);
+  if (AB.empty())
+return;
+
+  auto *CalledFunction = CS.getCalledFunction();
+  auto &Context = CalledFunction->getContext();
+
+  auto GetClonedValue = [&](Instruction *I) -> Value * {
+ValueToValueMapTy::iterator VMI = VMap.find(I);
+if (VMI == VMap.end())
+  return nullptr;
+return VMI->second;
+  };
+
+  auto MayContainThrowingOrExitingCall = [&](Instruction *RVal,
+ Instruction *RInst) {
+unsigned NumInstChecked = 0;
+for (auto &I :
+ make_range(RVal->getIterator(), std::prev(RInst->getIterator(
+  if (NumInstChecked++ > MaxInstCheckedForThrow ||
+  isGuaranteedToTransferExecutionToSuccessor(&I))
+return true;