[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58418#1430160 , @thakis wrote:

> Why is this needed for index-while-building? My mental model for 
> index-while-building is that that streams out build index metadata as part of 
> the regular compile. Why does that require watching directories?


You're right that this isn't necessary for the indexing phase. But we also 
provide API so clients can consume the index. This functionality is used for 
getting notifications about index data changes.

You can see it for example here:
https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111


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

https://reviews.llvm.org/D58418



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

[PATCH] D56343: [clang-tidy] Refactor: Compose Method on check_clang_tidy.py

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 190789.
LegalizeAdulthood added a comment.
Herald added a subscriber: jdoerfert.

To avoid passing lots of state to/from extracted functions, extract a class 
instead and use member variables for most of the state


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

https://reviews.llvm.org/D56343

Files:
  test/clang-tidy/check_clang_tidy.py

Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -38,170 +38,175 @@
 f.write(text)
 f.truncate()
 
+
 def csv(string):
   return string.split(',')
 
-def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument('-expect-clang-tidy-error', action='store_true')
-  parser.add_argument('-resource-dir')
-  parser.add_argument('-assume-filename')
-  parser.add_argument('input_file_name')
-  parser.add_argument('check_name')
-  parser.add_argument('temp_file_name')
-  parser.add_argument('-check-suffix', '-check-suffixes',
-  default=[''], type=csv,
-  help="comma-separated list of FileCheck suffixes")
-
-  args, extra_args = parser.parse_known_args()
-
-  resource_dir = args.resource_dir
-  assume_file_name = args.assume_filename
-  input_file_name = args.input_file_name
-  check_name = args.check_name
-  temp_file_name = args.temp_file_name
-  expect_clang_tidy_error = args.expect_clang_tidy_error
-
-  file_name_with_extension = assume_file_name or input_file_name
-  _, extension = os.path.splitext(file_name_with_extension)
-  if extension not in ['.c', '.hpp', '.m', '.mm']:
-extension = '.cpp'
-  temp_file_name = temp_file_name + extension
-
-  clang_tidy_extra_args = extra_args
-  if len(clang_tidy_extra_args) == 0:
-clang_tidy_extra_args = ['--']
-if extension in ['.cpp', '.hpp', '.mm']:
-  clang_tidy_extra_args.append('--std=c++11')
-if extension in ['.m', '.mm']:
-  clang_tidy_extra_args.extend(
-  ['-fobjc-abi-version=2', '-fobjc-arc'])
-
-  # Tests should not rely on STL being available, and instead provide mock
-  # implementations of relevant APIs.
-  clang_tidy_extra_args.append('-nostdinc++')
-
-  if resource_dir is not None:
-clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)
-
-  with open(input_file_name, 'r') as input_file:
-input_text = input_file.read()
-
-  check_fixes_prefixes = []
-  check_messages_prefixes = []
-  check_notes_prefixes = []
-
-  has_check_fixes = False
-  has_check_messages = False
-  has_check_notes = False
-
-  for check in args.check_suffix:
-if check and not re.match('^[A-Z0-9\-]+$', check):
-  sys.exit('Only A..Z, 0..9 and "-" are ' +
-'allowed in check suffixes list, but "%s" was given' % (check))
-
-file_check_suffix = ('-' + check) if check else ''
-check_fixes_prefix = 'CHECK-FIXES' + file_check_suffix
-check_messages_prefix = 'CHECK-MESSAGES' + file_check_suffix
-check_notes_prefix = 'CHECK-NOTES' + file_check_suffix
-
-has_check_fix = check_fixes_prefix in input_text
-has_check_message = check_messages_prefix in input_text
-has_check_note = check_notes_prefix in input_text
-
-if has_check_note and has_check_message:
-  sys.exit('Please use either %s or %s but not both' %
-(check_notes_prefix, check_messages_prefix))
-
-if not has_check_fix and not has_check_message and not has_check_note:
-  sys.exit('%s, %s or %s not found in the input' %
-(check_fixes_prefix, check_messages_prefix, check_notes_prefix))
-
-has_check_fixes = has_check_fixes or has_check_fix
-has_check_messages = has_check_messages or has_check_message
-has_check_notes = has_check_notes or has_check_note
-
-check_fixes_prefixes.append(check_fixes_prefix)
-check_messages_prefixes.append(check_messages_prefix)
-check_notes_prefixes.append(check_notes_prefix)
-
-  assert has_check_fixes or has_check_messages or has_check_notes
-  # Remove the contents of the CHECK lines to avoid CHECKs matching on
-  # themselves.  We need to keep the comments to preserve line numbers while
-  # avoiding empty lines which could potentially trigger formatting-related
-  # checks.
-  cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
-
-  write_file(temp_file_name, cleaned_test)
-
-  original_file_name = temp_file_name + ".orig"
-  write_file(original_file_name, cleaned_test)
-
-  args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
-clang_tidy_extra_args
-  if expect_clang_tidy_error:
-args.insert(0, 'not')
-  print('Running ' + repr(args) + '...')
-  try:
-clang_tidy_output = \
-subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
-  except subprocess.CalledProcessError as e:
-print('clang-tidy failed:\n' + e.output.decode())
-raise
-
-  print(' clang

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[MyDeveloperDay via Phabricator via cfe-commits]

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

From what I can tell this looks OK as you seem to be mainly only passing down 
the boolean, my guess is that while most people don't use the -lines directly 
that often but it probably gets called by some of the formatting tools that 
work over selections, maybe something like git-clang-format

Assuming you have tested some of those usescase and run the FormatTests, then 
we should be able to apply the "Beyonce rule" for everything else. (I've 
actually run the tests with your patch and they are fine)

If that is the case then this LGTM (but I'm not the code owner, just someone 
wanting to help with these stranded reviews)

I've landed this patch in my fork (which contains list of desired patches which 
seem to never get landed here..)

https://github.com/mydeveloperday/clang-experimental

IMHO @russellmcc clang-format needs people like you who understand the 
internals, to help move along some of the reviews that just get stuck in no 
mans land.


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

https://reviews.llvm.org/D54881



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Starting to look good i think.




Comment at: docs/ReleaseNotes.rst:88
 
+- New alias :doc:`abseil-make-unique
+  ` to :doc:`modernize-make-unique

Also add a new entry about the new option for `modernize-make-unique`.


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

https://reviews.llvm.org/D55044



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

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[MyDeveloperDay via Phabricator via cfe-commits]

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

So @russellmcc  you've been bumping along this road nicely for 6 months, doing 
what people say... pinging every week or so in order to get your code reviewed, 
and you are getting no response.

Was there anything you think people were objecting too other than the normal 
"its a high bar to get in" and "its complex in here"?

I think its fairer to the person submitting a revision if they don't want 
feature in, we should give feedback as to why, but are we to assume silence is 
acceptance? how long do we wait? who is the decision maker?

I've personally never met an Engineer who didn't like having more knobs to 
fiddle with so I don't believe the rational that having more options is bad as 
long as they don't interfere with each other, for that there is the "Beyonce 
rule", if adding an option breaks someone else then "if they liked it they 
should have put a test on it!"

As far as I can tell this LGTM (I'm not the code owner, just someone wanting to 
help)

In the meantime I have uploaded this patch to my fork, where I'm maintaining a 
clang-format with revisions that seem ok, but get stalled in the review process.

https://github.com/mydeveloperday/clang-experimental


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

https://reviews.llvm.org/D40988



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

[PATCH] D58882: [Diagnostics] Address of a standard library function

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Ping


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

https://reviews.llvm.org/D58882



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

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D40988#1430502 , @MyDeveloperDay 
wrote:

> So @russellmcc  you've been bumping along this road nicely for 6 months, 
> doing what people say... pinging every week or so in order to get your code 
> reviewed, and you are getting no response.
>
> Was there anything you think people were objecting too other than the normal 
> "its a high bar to get in" and "its complex in here"?
>
> I think its fairer to the person submitting a revision if they don't want 
> feature in, we should give feedback as to why, but are we to assume silence 
> is acceptance? how long do we wait? who is the decision maker?
>
> I've personally never met an Engineer who didn't like having more knobs to 
> fiddle with so I don't believe the rational that having more options is bad 
> as long as they don't interfere with each other, for that there is the 
> "Beyonce rule", if adding an option breaks someone else then "if they liked 
> it they should have put a test on it!"
>
> As far as I can tell this LGTM (I'm not the code owner, just someone wanting 
> to help)
>
> In the meantime I have uploaded this patch to my fork, where I'm maintaining 
> a clang-format with revisions that seem ok, but get stalled in the review 
> process.
>
> https://github.com/mydeveloperday/clang-experimental


I think in this case there were no objections to the knob, but djasper had 
concerns about the code.
The problem in this case is that there was a 3 month pause between the review 
comment and the response, and for those intricate changes (and yes, 
clang-format's design is problematic here in that it makes these changes really 
hard to get right) it take a long time to think oneself into the problem to 
fully understand where it could go wrong.

Regarding the abstract argument brought up about tests - this is not about 
whether specific things break, but whether the code gets harder to maintain 
over time, which in the end will lead to nobody being able to make changes, 
regardless whether there is a reviewer or not - I think it's important we don't 
underestimate that risk & cost.


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

https://reviews.llvm.org/D40988



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

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: djasper, klimek, reuk, JonasToth, alexfh, 
krasimir, russellmcc.
MyDeveloperDay added a project: clang-tools-extra.
Herald added a subscriber: jdoerfert.

An addendum to  D59087: [clang-format] [PR25010] 
AllowShortIfStatementsOnASingleLine not working if an "else" statement is 
present  following discussions with submitter 
of https://bugs.llvm.org/show_bug.cgi?id=25010

it didn't make sense that, we could only do short if and not short else if and 
short else if they didn't have compound statments

  if (true) return 0;
  
  if (true) return 0;
  else return 1;
  
  if (true) return 0;
  else if (false) return 1;
  
  if (true) return 0;
  else if (false) return 1;
  else  return 2;
  
  if (true) return 0;
  else if (false) return 1;
  else {
return 2;
  }

The following revision extends the capability form D59087: [clang-format] 
[PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" 
statement is present  to allow that if the 
correct setting is provided

existing true/false still honor the original meaning of 
AllowShortIfStatementsOnASingleLine

but the additional options now allow more fine grained control over how else/if 
and else should/could be handled.


https://reviews.llvm.org/D59408

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -505,6 +505,35 @@
"  g();\n",
AllowsMergedIf);
 
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_AlwaysNoElse;
+
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else\n"
+   "  g();\n",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else if (b) g();\n",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else if (b) g();\n"
+   "else\n"
+   "  h();\n",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+
   AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
 
   verifyFormat("if (a) f();\n"
@@ -521,6 +550,16 @@
"  g();\n"
"}",
AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else g();\n",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else if (b) g();\n",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else if (b) g();\n"
+   "else h();\n",
+   AllowsMergedIf);
 }
 
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -363,6 +363,18 @@
  ? tryMergeSimpleControlStatement(I, E, Limit)
  : 0;
 }
+if (TheLine->First->startsSequence(tok::kw_else, tok::kw_if)) {
+  return (Style.AllowShortIfStatementsOnASingleLine >=
+  FormatStyle::SIS_AlwaysNoElse)
+ ? tryMergeSimpleControlStatement(I, E, Limit)
+ : 0;
+}
+if (TheLine->First->is(tok::kw_else)) {
+  return Style.AllowShortIfStatementsOnASingleLine ==
+ FormatStyle::SIS_Always
+ ? tryMergeSimpleControlStatement(I, E, Limit)
+ : 0;
+}
 if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while)) {
   return Style.AllowShortLoopsOnASingleLine
  ? tryMergeSimpleControlStatement(I, E, Limit)
@@ -406,7 +418,8 @@
   return 0;
 Limit = limitConsideringMacros(I + 1, E, Limit);
 AnnotatedLine &Line = **I;
-if (Line.Last->isNot(tok::r_paren))
+if (Line.Last->isNot(tok::r_paren) &&
+Style.AllowShortIfStatementsOnASingleLine < FormatStyle::SIS_Always)
   return 0;
 if (1 + I[1]->Last->TotalLength > Limit)
   return 0;
@@ -414,7 +427,8 @@
  TT_LineComment))
   return 0;
 // Only inline simple if's (no nested if or else), unless specified
-if (Style.AllowShortIfStatementsOnASingleLine != FormatStyle::SIS_Always) {
+if (Style.AllowShortIfStatementsOnAS

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> nobody being able to make changes

Nobody IS able to make changes, but not because of complexity!

We discourage away potential contributors/maintainers by leaving their reviews 
for weeks/months/years, not just not letting them in, not even discussing them..

Most reviews are met with a sharp intake of breath, and a general comment about 
"how its a pretty big bar we have here and how complex it is and perhaps you 
shouldn't come in", rather than seeing it as a positive move forward in the 
right direction and a chance to bring someone on as a new contributor.

But we can't do anything to improve the complexity of the code if the changes 
to refactor code to be less complex is going to sit un-reviewed.

But I'm super glad that me giving an Accept and LGTM  after 6 months is 
catching peoples attention... job done.


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

https://reviews.llvm.org/D40988



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 5 inline comments as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:74
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 

ioeric wrote:
> could you explain this change?
We give an extra bonus point for a consecutive match now, so the maximum bonus 
is now higher.
This constant is used for score normalization to bring the final score into 
`[0,1]` range (IIUC, there's a special case when the score can be `2.0`, in 
case of a substring match, but the normalization is still applied to per-char 
scores)




Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:270
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;

ioeric wrote:
> If the word is "_something", do we still want to penalize skipping first char?
Yes and it should actually play out nicely in practice. Consider two 
possibilities:
1. the pattern starts with `_`. In that case we want the items starting with 
`_` to be ranked higher, penalizing other items achieves this.
2. the pattern does not start with `_`. In that case all the items, starting 
with `_` will get a penalty and we will prefer items starting with the first 
character of the pattern. Also looks desirable.

This penalty in some sense replaces the `prefix match` boost we had before.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:275
+  // match.
+  return 0;
 }

ioeric wrote:
> iiuc, there is no penalty by default because consecutive matches will gain 
> bonus. This is not trivial here. Maybe add a comment? Or consider apply 
> penalty for non-consecutive matches and no bonus for consecutive ones.
Added a comment. Adding a bonus instead of a penalty seems to produce smaller 
discrepancies between match and no-match cases for small patterns.

I.e. the non-consecutive match of a next segment gets a smaller hit in the 
final score.
I.e.  a match of `p` in `[u]nique_[p]tr` gets a score of `2 out of 4` and no 
penalties associated with it now, previously it would get a score of `2 out of 
3` but a penalty of `2` for a non-consecutive match, which just turned out to 
be too much.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;

ioeric wrote:
> could you explain the intention of this change? Is it relevant to other 
> changes in the patch? The original looks reasonable to me. 
The motivation is keeping a bonus for matching the head of a segment even in 
case of single-case patterns that lack segmentation signals.
Previously, `p` from `up` would not get this bonus when matching `unique_[p]tr` 
even though intuitively it should as we do want to encourage those matches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 190796.
ilya-biryukov added a comment.

- Added comments
- Add a test case for 'onmes'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300

Files:
  clang-tools-extra/clangd/FuzzyMatch.cpp
  clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
   ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));
+  EXPECT_THAT("onmes",
+  ranks("[onmes]sage", "[onMes]sage", "[on]This[M]ega[Es]capes"));
   EXPECT_THAT("CC", ranks("[C]amel[C]ase", "[c]amel[C]ase"));
   EXPECT_THAT("cC", ranks("[c]amel[C]ase", "[C]amel[C]ase"));
   EXPECT_THAT("p", ranks("[p]", "[p]arse", "[p]osix", "[p]afdsa", "[p]ath"));
@@ -270,12 +273,18 @@
 // Verify some bounds so we know scores fall in the right range.
 // Testing exact scores is fragile, so we prefer Ranking tests.
 TEST(FuzzyMatch, Scoring) {
-  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 0.f));
+  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 7.f / 12.f));
   EXPECT_THAT("abs", matches("[abs]l", 1.f));
   EXPECT_THAT("abs", matches("[abs]", 2.f));
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+TEST(FuzzyMatch, InitialismAndSegment) {
+  // We want these scores to be roughly the same.
+  EXPECT_THAT("up", matches("[u]nique_[p]tr", 3.f / 4.f));
+  EXPECT_THAT("up", matches("[up]per_bound", 1.f));
+}
+
 // Returns pretty-printed segmentation of Text.
 // e.g. std::basic_string --> +--  + +-
 std::string segment(llvm::StringRef Text) {
Index: clang-tools-extra/clangd/FuzzyMatch.cpp
===
--- clang-tools-extra/clangd/FuzzyMatch.cpp
+++ clang-tools-extra/clangd/FuzzyMatch.cpp
@@ -71,7 +71,7 @@
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
 static constexpr int AwfulScore = -(1 << 13);
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 
 FuzzyMatcher::FuzzyMatcher(llvm::StringRef Pattern)
 : PatN(std::min(MaxPat, Pattern.size())),
@@ -267,24 +267,30 @@
 }
 
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep it lower than the bonus for a consecutive
+  // match.
+  // Instead of penalizing non-consecutive matches, we give a bonus to a
+  // consecutive match in matchBonus. This produces a better score distribution
+  // than penalties in case of small patterns, e.g. 'up' for 'unique_ptr'.
+  return 0;
 }
 
 int FuzzyMatcher::matchBonus(int P, int W, Action Last) const {
   assert(LowPat[P] == LowWord[W]);
   int S = 1;
-  // Bonus: pattern so far is a (case-insensitive) prefix of the word.
-  if (P == W) // We can't skip pattern characters, so we must have matched all.
-++S;
+  bool IsPatSingleCase =
+  (PatTypeSet == 1 << Lower) || (PatTypeSet == 1 << Upper);
   // Bonus: case matches, or a Head in the pattern aligns with one in the word.
-  if ((Pat[P] == Word[W] && ((PatTypeSet & 1 << Upper) || P == W)) ||
-  (PatRole[P] == Head && WordRole[W] == Head))
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;
+  // Bonus: a consecutive match. First character match also gets a bonus to
+  // ensure prefix final match score normalizes to 1.0.
+  if (W == 0 || Last == Match)
+S += 2;
   // Penalty: matching inside a segment (and previous char wasn't matched).
   if (WordRole[W] == Tail && P && Last == Miss)
 S -= 3;


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
   ranks("[onMes]sage", "[onmes]sage", 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.h:62
+/// Find the record type references at \p Pos.
+const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos);
+

nridge wrote:
> ilya-biryukov wrote:
> > This method looks like an implementation detail and does not align with 
> > other methods in `XRefs.h` which are high-level methods that implement LSP 
> > functionality.
> > 
> > It would be more appropriate to move it to `AST.h` or directly into the 
> > `XRefs.cpp`. WDYT?
> @sammccall asked for this method to be exposed in the header and some of the 
> tests written in terms of it.
LG, thanks for the clarification. I haven't been following the review closely.
Having methods that return pointers in `XRefs.h` seems like the wrong layering 
to me, but in any case it's a minor detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



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

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM, thanks Nathan!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The only important comment is about test coverage




Comment at: clang-tools-extra/clangd/AST.cpp:84
+if (auto STL = TL.getAs()) {
+  std::vector ArgLocs;
+  for (unsigned I = 0; I < STL.getNumArgs(); I++)

kadircet wrote:
> ilya-biryukov wrote:
> > NIT: use `SmallVector<8>` or some other small-enough number to avoid most 
> > allocs.
> calling reserve beforehand
`SmallVector` is even better because it keeps the elements on the stack and 
won't need to do dynamic allocations (most of the time).
My suggestion would be to use it here anyway.

Up to you, though.



Comment at: clang-tools-extra/unittests/clangd/IndexTests.cpp:18
 #include "clang/Index/IndexSymbol.h"
+#include "gmock/gmock-generated-matchers.h"
 #include "gmock/gmock.h"

kadircet wrote:
> ilya-biryukov wrote:
> > NIT: maybe remove it? clangd keeps adding those, but I don't think we 
> > actually want it: `gmock.h` should be enough
> Should we add IWYU pragmas to those files? 
> https://github.com/google/googlemock/blob/master/googlemock/include/gmock/gmock-generated-matchers.h
I think we should, but some projects don't like those pragmas in their headers.
Trying to add this won't hurt for sure!



Comment at: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp:417
+ForCodeCompletion(true)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),

kadircet wrote:
> ilya-biryukov wrote:
> > Does it mean typing `bool` could potentially match `vector` in 
> > `workspaceSymbols` now?
> > If that's the case we might start producing some noise.
> unfortunately yes it does, what do you suggest? it seems like we can perform 
> a "isprefix" check for symbols with template specs kind?
Let's land this and see if this creates too much noise.
If it will, we might want to drop the argument lists from the words we match 
and only keep them for presentation to the user.

But let's not worry about this too much for now, the current behavior LG unless 
proven otherwise :-)



Comment at: clang/lib/AST/TypePrinter.cpp:1646
+break;
+  case TemplateArgument::ArgKind::Type:
+A.getTypeSourceInfo()->getType().print(OS, PP);

kadircet wrote:
> ilya-biryukov wrote:
> > Maybe simplify the switch to:
> > ```
> > if (A.getKind() == TemplateArgument::ArgKind::Type) {
> > A.getTypeSourceInfo()->getType().print(OS, PP);
> > return;
> > }
> > A.getArgument().print(PP, OS);
> > ```
> > 
> It was rather to catch any changes in the ArgKind at compile-time, still can 
> do if you think this should not cause any problems
I think listing all the cases here does not actually buy us anything since we 
call a function that should somehow print the arguments anyway.

But up to you.



Comment at: clang/lib/AST/TypePrinter.cpp:1655
+  case TemplateArgument::ArgKind::Expression:
+  case TemplateArgument::ArgKind::Pack:
+A.getArgument().print(PP, OS);

Now that you mentioned other kinds of parameters, could you add tests for other 
kinds of parameters?
In particular, I expect we'll definitely need to handle packs here and probably 
all other kinds of arguments too.
- Non-type and template template parameters.
```
template  class TT, template  class JJ>
struct Foo {};

template  class UU>
struct Foo {};
```
- Parameter packs.
```
template 
struct Bar {};

template 
struct Bar {};

template 
struct Baz {};

template 
struct Baz {};

template  class...TT>
struct Qux {};

template  class TT>
struct Qux {};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D57532: [Index] Make sure c-index-test finds libc++ on Mac

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Lost this one after vacation, sorry.
Still need to figure out why the test is failing. Any help appreaciated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57532



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

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 190798.
carwil added a comment.

Done. I've added an extra parameter over what you might expect as 
classifyArgumentTypes doesn't seem to consider AAPCS16, whereas 
classifyReturnTypes does.
I've also renamed the variable to make the distinction between it and the 
function clearer.


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

https://reviews.llvm.org/D59094

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCXX/arm-pcs.cpp

Index: clang/test/CodeGenCXX/arm-pcs.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/arm-pcs.cpp
@@ -0,0 +1,51 @@
+// Covers a bug fix for ABI selection with homogenous aggregates:
+//  See: https://bugs.llvm.org/show_bug.cgi?id=39982
+
+// REQUIRES: arm-registered-target
+// RUN: %clang -mfloat-abi=hard --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=HARD,CHECK
+// RUN: %clang -mfloat-abi=softfp --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFTFP,CHECK
+// RUN: %clang -mfloat-abi=soft --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFT,CHECK
+
+struct S {
+  float f;
+  float d;
+  float c;
+  float t;
+};
+
+// Variadic functions should always marshal for the base standard.
+// See section 5.5 (Parameter Passing) of the AAPCS.
+float __attribute__((pcs("aapcs-vfp"))) variadic(S s, ...) {
+  // CHECK-NOT: vmov s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
+
+float no_attribute(S s) {
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) baz(float x, float y) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return y;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) foo(S s) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs"))) bar(S s) {
+  // CHECK-NOT: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -5591,8 +5591,10 @@
   ABIKind getABIKind() const { return Kind; }
 
 private:
-  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic) const;
-  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic) const;
+  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic,
+unsigned functionCallConv) const;
+  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic,
+  unsigned functionCallConv) const;
   ABIArgInfo classifyHomogeneousAggregate(QualType Ty, const Type *Base,
   uint64_t Members) const;
   ABIArgInfo coerceIllegalVector(QualType Ty) const;
@@ -5602,6 +5604,8 @@
   bool isHomogeneousAggregateSmallEnough(const Type *Ty,
  uint64_t Members) const override;
 
+  bool isEffectivelyAAPCS_VFP(unsigned callConvention, bool acceptHalf) const;
+
   void computeInfo(CGFunctionInfo &FI) const override;
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
@@ -5722,11 +5726,13 @@
 
 void ARMABIInfo::computeInfo(CGFunctionInfo &FI) const {
   if (!::classifyReturnType(getCXXABI(), FI, *this))
-FI.getReturnInfo() =
-classifyReturnType(FI.getReturnType(), FI.isVariadic());
+FI.getReturnInfo() = classifyReturnType(FI.getReturnType(), FI.isVariadic(),
+FI.getCallingConvention());
 
   for (auto &I : FI.arguments())
-I.info = classifyArgumentType(I.type, FI.isVariadic());
+I.info = classifyArgumentType(I.type, FI.isVariadic(),
+  FI.getCallingConvention());
+
 
   // Always honor user-specified calling convention.
   if (FI.getCallingConvention() != llvm::CallingConv::C)
@@ -5805,8 +5811,8 @@
   return ABIArgInfo::getDirect(nullptr, 0, nullptr, false);
 }
 
-ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty,
-bool isVariadic) const {
+ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
+unsigned functionCallConv) const {
   // 6.1.2.1 The following argument types are VFP CPRCs:
   //   A single-precision floating-point type (including promoted
   //   half-precision types); A double-precision floating-point type;
@@ -5814,7 +5820,9 @@
   //   with 

[PATCH] D59354: [clangd] Print arguments in template specializations

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1644
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > ilya-biryukov wrote:
> > > > Maybe print the result of `getTypeLoc()` here, if it's available?
> > > > Would produce results closer to the ones written in the code.
> > > unfortunately it is not available.
> > you mean the function to print a type loc or the type loc itself?
> function to print a typeloc
Thanks for the clarification.
Intuitively, I've expected it to be there, but it seems that we can only print 
the types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;

ilya-biryukov wrote:
> ioeric wrote:
> > could you explain the intention of this change? Is it relevant to other 
> > changes in the patch? The original looks reasonable to me. 
> The motivation is keeping a bonus for matching the head of a segment even in 
> case of single-case patterns that lack segmentation signals.
> Previously, `p` from `up` would not get this bonus when matching 
> `unique_[p]tr` even though intuitively it should as we do want to encourage 
> those matches.
so, iiuc, when `IsPatSingleCase` is true, it means that any character in the 
pattern can potentially be a head? could you add comment for this?

We also had a stricter condition for `Pat[P] == Word[W]`, but now `ut` would 
get a bonus for `[u]nique_p[t]r`. Is this intended?



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:273
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep it lower than the bonus for a consecutive
+  // match.

nit: move this comment into into own line now that it's taking two?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Eric Liu via Phabricator via cfe-commits]

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

(The result looks great)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1655
+  case TemplateArgument::ArgKind::Expression:
+  case TemplateArgument::ArgKind::Pack:
+A.getArgument().print(PP, OS);

ilya-biryukov wrote:
> Now that you mentioned other kinds of parameters, could you add tests for 
> other kinds of parameters?
> In particular, I expect we'll definitely need to handle packs here and 
> probably all other kinds of arguments too.
> - Non-type and template template parameters.
> ```
> template  class TT, template  class JJ>
> struct Foo {};
> 
> template  class UU>
> struct Foo {};
> ```
> - Parameter packs.
> ```
> template 
> struct Bar {};
> 
> template 
> struct Bar {};
> 
> template 
> struct Baz {};
> 
> template 
> struct Baz {};
> 
> template  class...TT>
> struct Qux {};
> 
> template  class TT>
> struct Qux {};
> ```
I've already had tests for these cases, but adding a few more for partial specs 
as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 190800.
kadircet marked 11 inline comments as done.
kadircet added a comment.

- Add more tests
- Replace switch with if
- Use SmallVector


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/unittests/clangd/DexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,21 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+  llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+  const PrintingPolicy &PP, llvm::raw_ostream &OS) {
+  const auto &Kind = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+ "TemplateArgumentKind can not be null!");
+  if (Kind == TemplateArgument::ArgKind::Type)
+return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template
 static void printTo(raw_ostream &OS, ArrayRef Args,
 const PrintingPolicy &Policy, bool SkipBrackets) {
@@ -1653,7 +1668,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -394,23 +394,80 @@
   Annotations Header(R"(
 // Primary template and explicit specialization are indexed, instantiation
 // is not.
-template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
-template <> struct $specdecl[[Tmpl]] {};
-template  struct $partspecdecl[[Tmpl]] {};
-extern template struct Tmpl;
-template struct Tmpl;
+template  class $barclasstemp[[Bar]] {};
+template  class Z, int Q>
+struct [[Tmpl]] { T $xdecl[[x]] = 0; };
+
+// template-template, non-type and type full spec
+template <> struct $specdecl[[Tmpl]] {};
+
+// template-template, non-type and type partial spec
+template  struct $partspecdecl[[Tmpl]] {};
+// instantiation
+extern template struct Tmpl;
+// instantiation
+template struct Tmpl;
+
+template  class $fooclasstemp[[Foo]] {};
+// parameter-packs full spec
+template<> class $parampack[[Foo]], int, double> {};
+// parameter-packs partial spec
+template class $parampackpartial[[Foo]] {};
+
+template  class $bazclasstemp[[Baz]] {};
+// non-type parameter-packs full spec
+template<> class $parampacknontype[[Baz]]<3, 5, 8> {};
+// non-type parameter-packs partial spec
+template class $parampacknontypepartial[[Baz]] {};
+
+template  class ...> class $fozclasstemp[[Foz]] {};
+// template-template parameter-packs full spec
+template<> class $parampacktempltempl[[Foz]] {};
+// template-template parameter-packs partial spec
+template class T>
+class $parampacktempltemplpartial[[Foz]] {};
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(
-  AllOf(QName("Tmpl"), DeclRange(Header.range()),
-ForCodeCompletion(true)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("specdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("partspecdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
-ForCodeCompletion(false;
+  EXPECT_THAT(Symbols.size(), 14U);
+  EXPECT_THAT(
+  Symbols,
+  AllOf(
+  Contains(AllOf(QName("Tmpl"), DeclRange(Header.range()),
+ ForCodeCompletion(true))),
+  Contains(AllOf(QName("Bar"), DeclRange(Header.range("barclasstemp")),
+ ForCodeCompletion(true))),
+  Contains(AllOf(QName("Foo"), DeclRange(Header.range("fooclasstemp")),
+ ForCodeCompletion(true))),
+  Contains(AllOf(QName("Baz"), DeclRange(Header.range("bazclasstemp")),
+ ForCodeCompletion(true))),
+  Contains(AllOf(QName("Foz"), DeclRange(Header.range("fozclasstemp")),
+ ForCodeCompletion(t

r356250 - Make getFullyQualifiedName qualify both the pointee and class type for member ptr types

[Benjamin Kramer via cfe-commits]

Author: d0k
Date: Fri Mar 15 04:09:41 2019
New Revision: 356250

URL: http://llvm.org/viewvc/llvm-project?rev=356250&view=rev
Log:
Make getFullyQualifiedName qualify both the pointee and class type for member 
ptr types

We already handle pointers and references, member ptrs are just another
special case. Fixes PR40732.

Differential Revision: https://reviews.llvm.org/D59387

Modified:
cfe/trunk/lib/AST/QualTypeNames.cpp
cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp

Modified: cfe/trunk/lib/AST/QualTypeNames.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/QualTypeNames.cpp?rev=356250&r1=356249&r2=356250&view=diff
==
--- cfe/trunk/lib/AST/QualTypeNames.cpp (original)
+++ cfe/trunk/lib/AST/QualTypeNames.cpp Fri Mar 15 04:09:41 2019
@@ -379,6 +379,19 @@ QualType getFullyQualifiedType(QualType
 return QT;
   }
 
+  if (auto *MPT = dyn_cast(QT.getTypePtr())) {
+// Get the qualifiers.
+Qualifiers Quals = QT.getQualifiers();
+// Fully qualify the pointee and class types.
+QT = getFullyQualifiedType(QT->getPointeeType(), Ctx, WithGlobalNsPrefix);
+QualType Class = getFullyQualifiedType(QualType(MPT->getClass(), 0), Ctx,
+   WithGlobalNsPrefix);
+QT = Ctx.getMemberPointerType(QT, Class.getTypePtr());
+// Add back the qualifiers.
+QT = Ctx.getQualifiedType(QT, Quals);
+return QT;
+  }
+
   // In case of myType& we need to strip the reference first, fully
   // qualify and attach the reference once again.
   if (isa(QT.getTypePtr())) {

Modified: cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp?rev=356250&r1=356249&r2=356250&view=diff
==
--- cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp Fri Mar 15 04:09:41 2019
@@ -194,6 +194,7 @@ TEST(QualTypeNameTest, getFullyQualified
   GlobalNsPrefix.ExpectedQualTypeNames["ZVal"] = "::A::B::Y::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["GlobalZVal"] = "::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["CheckK"] = "D::aStruct";
+  GlobalNsPrefix.ExpectedQualTypeNames["YZMPtr"] = "::A::B::X ::A::B::Y::Z::*";
   GlobalNsPrefix.runOver(
   "namespace A {\n"
   "  namespace B {\n"
@@ -205,8 +206,9 @@ TEST(QualTypeNameTest, getFullyQualified
   "template \n"
   "using Alias = CCC;\n"
   "Alias IntAliasVal;\n"
-  "struct Y { struct Z {}; };\n"
+  "struct Y { struct Z { X YZIPtr; }; };\n"
   "Y::Z ZVal;\n"
+  "X Y::Z::*YZMPtr;\n"
   "  }\n"
   "}\n"
   "struct Z {};\n"


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

[PATCH] D59387: Make getFullyQualifiedName qualify both the pointee and class type for member ptr types

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356250: Make getFullyQualifiedName qualify both the pointee 
and class type for member… (authored by d0k, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59387?vs=190730&id=190801#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59387

Files:
  cfe/trunk/lib/AST/QualTypeNames.cpp
  cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp


Index: cfe/trunk/lib/AST/QualTypeNames.cpp
===
--- cfe/trunk/lib/AST/QualTypeNames.cpp
+++ cfe/trunk/lib/AST/QualTypeNames.cpp
@@ -379,6 +379,19 @@
 return QT;
   }
 
+  if (auto *MPT = dyn_cast(QT.getTypePtr())) {
+// Get the qualifiers.
+Qualifiers Quals = QT.getQualifiers();
+// Fully qualify the pointee and class types.
+QT = getFullyQualifiedType(QT->getPointeeType(), Ctx, WithGlobalNsPrefix);
+QualType Class = getFullyQualifiedType(QualType(MPT->getClass(), 0), Ctx,
+   WithGlobalNsPrefix);
+QT = Ctx.getMemberPointerType(QT, Class.getTypePtr());
+// Add back the qualifiers.
+QT = Ctx.getQualifiedType(QT, Quals);
+return QT;
+  }
+
   // In case of myType& we need to strip the reference first, fully
   // qualify and attach the reference once again.
   if (isa(QT.getTypePtr())) {
Index: cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
===
--- cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
+++ cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
@@ -194,6 +194,7 @@
   GlobalNsPrefix.ExpectedQualTypeNames["ZVal"] = "::A::B::Y::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["GlobalZVal"] = "::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["CheckK"] = "D::aStruct";
+  GlobalNsPrefix.ExpectedQualTypeNames["YZMPtr"] = "::A::B::X ::A::B::Y::Z::*";
   GlobalNsPrefix.runOver(
   "namespace A {\n"
   "  namespace B {\n"
@@ -205,8 +206,9 @@
   "template \n"
   "using Alias = CCC;\n"
   "Alias IntAliasVal;\n"
-  "struct Y { struct Z {}; };\n"
+  "struct Y { struct Z { X YZIPtr; }; };\n"
   "Y::Z ZVal;\n"
+  "X Y::Z::*YZMPtr;\n"
   "  }\n"
   "}\n"
   "struct Z {};\n"


Index: cfe/trunk/lib/AST/QualTypeNames.cpp
===
--- cfe/trunk/lib/AST/QualTypeNames.cpp
+++ cfe/trunk/lib/AST/QualTypeNames.cpp
@@ -379,6 +379,19 @@
 return QT;
   }
 
+  if (auto *MPT = dyn_cast(QT.getTypePtr())) {
+// Get the qualifiers.
+Qualifiers Quals = QT.getQualifiers();
+// Fully qualify the pointee and class types.
+QT = getFullyQualifiedType(QT->getPointeeType(), Ctx, WithGlobalNsPrefix);
+QualType Class = getFullyQualifiedType(QualType(MPT->getClass(), 0), Ctx,
+   WithGlobalNsPrefix);
+QT = Ctx.getMemberPointerType(QT, Class.getTypePtr());
+// Add back the qualifiers.
+QT = Ctx.getQualifiedType(QT, Quals);
+return QT;
+  }
+
   // In case of myType& we need to strip the reference first, fully
   // qualify and attach the reference once again.
   if (isa(QT.getTypePtr())) {
Index: cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
===
--- cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
+++ cfe/trunk/unittests/Tooling/QualTypeNamesTest.cpp
@@ -194,6 +194,7 @@
   GlobalNsPrefix.ExpectedQualTypeNames["ZVal"] = "::A::B::Y::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["GlobalZVal"] = "::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["CheckK"] = "D::aStruct";
+  GlobalNsPrefix.ExpectedQualTypeNames["YZMPtr"] = "::A::B::X ::A::B::Y::Z::*";
   GlobalNsPrefix.runOver(
   "namespace A {\n"
   "  namespace B {\n"
@@ -205,8 +206,9 @@
   "template \n"
   "using Alias = CCC;\n"
   "Alias IntAliasVal;\n"
-  "struct Y { struct Z {}; };\n"
+  "struct Y { struct Z { X YZIPtr; }; };\n"
   "Y::Z ZVal;\n"
+  "X Y::Z::*YZMPtr;\n"
   "  }\n"
   "}\n"
   "struct Z {};\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 190803.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.

Address comments:

- Shorten the comment to fit it into a single line.
- Added a comment about single-case patterns


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300

Files:
  clang-tools-extra/clangd/FuzzyMatch.cpp
  clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
   ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));
+  EXPECT_THAT("onmes",
+  ranks("[onmes]sage", "[onMes]sage", "[on]This[M]ega[Es]capes"));
   EXPECT_THAT("CC", ranks("[C]amel[C]ase", "[c]amel[C]ase"));
   EXPECT_THAT("cC", ranks("[c]amel[C]ase", "[C]amel[C]ase"));
   EXPECT_THAT("p", ranks("[p]", "[p]arse", "[p]osix", "[p]afdsa", "[p]ath"));
@@ -270,12 +273,18 @@
 // Verify some bounds so we know scores fall in the right range.
 // Testing exact scores is fragile, so we prefer Ranking tests.
 TEST(FuzzyMatch, Scoring) {
-  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 0.f));
+  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 7.f / 12.f));
   EXPECT_THAT("abs", matches("[abs]l", 1.f));
   EXPECT_THAT("abs", matches("[abs]", 2.f));
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+TEST(FuzzyMatch, InitialismAndSegment) {
+  // We want these scores to be roughly the same.
+  EXPECT_THAT("up", matches("[u]nique_[p]tr", 3.f / 4.f));
+  EXPECT_THAT("up", matches("[up]per_bound", 1.f));
+}
+
 // Returns pretty-printed segmentation of Text.
 // e.g. std::basic_string --> +--  + +-
 std::string segment(llvm::StringRef Text) {
Index: clang-tools-extra/clangd/FuzzyMatch.cpp
===
--- clang-tools-extra/clangd/FuzzyMatch.cpp
+++ clang-tools-extra/clangd/FuzzyMatch.cpp
@@ -71,7 +71,7 @@
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
 static constexpr int AwfulScore = -(1 << 13);
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 
 FuzzyMatcher::FuzzyMatcher(llvm::StringRef Pattern)
 : PatN(std::min(MaxPat, Pattern.size())),
@@ -267,24 +267,31 @@
 }
 
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep this lower than a consecutive match bonus.
+  // Instead of penalizing non-consecutive matches, we give a bonus to a
+  // consecutive match in matchBonus. This produces a better score distribution
+  // than penalties in case of small patterns, e.g. 'up' for 'unique_ptr'.
+  return 0;
 }
 
 int FuzzyMatcher::matchBonus(int P, int W, Action Last) const {
   assert(LowPat[P] == LowWord[W]);
   int S = 1;
-  // Bonus: pattern so far is a (case-insensitive) prefix of the word.
-  if (P == W) // We can't skip pattern characters, so we must have matched all.
-++S;
+  bool IsPatSingleCase =
+  (PatTypeSet == 1 << Lower) || (PatTypeSet == 1 << Upper);
   // Bonus: case matches, or a Head in the pattern aligns with one in the word.
-  if ((Pat[P] == Word[W] && ((PatTypeSet & 1 << Upper) || P == W)) ||
-  (PatRole[P] == Head && WordRole[W] == Head))
+  // Single-case patterns lack segmentation signals and we assume any character
+  // can be a head of a segment.
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;
+  // Bonus: a consecutive match. First character match also gets a bonus to
+  // ensure prefix final match score normalizes to 1.0.
+  if (W == 0 || Last == Match)
+S += 2;
   // Penalty: matching inside a segment (and previous char wasn't matched).
   if (WordRole[W] == Tail && P && Last == Miss)
 S -= 3;


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-m

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > could you explain the intention of this change? Is it relevant to other 
> > > changes in the patch? The original looks reasonable to me. 
> > The motivation is keeping a bonus for matching the head of a segment even 
> > in case of single-case patterns that lack segmentation signals.
> > Previously, `p` from `up` would not get this bonus when matching 
> > `unique_[p]tr` even though intuitively it should as we do want to encourage 
> > those matches.
> so, iiuc, when `IsPatSingleCase` is true, it means that any character in the 
> pattern can potentially be a head? could you add comment for this?
> 
> We also had a stricter condition for `Pat[P] == Word[W]`, but now `ut` would 
> get a bonus for `[u]nique_p[t]r`. Is this intended?
> so, iiuc, when IsPatSingleCase is true, it means that any character in the 
> pattern can potentially be a head? could you add comment for this?
Exactly, we don't have any segmentation signals in a pattern and we don't want 
small drops in scores on each match a segment head, e.g. there's no reason 
matching `c` should have smaller bonus than `r` when matching `My[StrC]at` with 
`strc`.
Added a comment.

> We also had a stricter condition for Pat[P] == Word[W], but now ut would get 
> a bonus for [u]nique_p[t]r. Is this intended?
This one is tricky and I'm less sure of it. I think the important bit is to 
give this bonus to `t` in case of `[u]nique_[pt]r`.
Otherwise we're making a match of the non-head character worse for no reason. I 
think the real problem in the previous implementation was its preference of 
prefix matches (the `P == W` condition), which gave an unfair advantage to 
`[up]per_bound` over `[u]nique_[p]tr`.

Matches in the middle of a segment are harshly penalized in the next line, so 
this small bonus does not seem to matter in practice for the case you 
mentioned. Maybe there's a better way to do this.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:273
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep it lower than the bonus for a consecutive
+  // match.

ioeric wrote:
> nit: move this comment into into own line now that it's taking two?
Shrinked it to fit into a single line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 190804.
ilya-biryukov added a comment.

- A better name for the new test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300

Files:
  clang-tools-extra/clangd/FuzzyMatch.cpp
  clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
   ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));
+  EXPECT_THAT("onmes",
+  ranks("[onmes]sage", "[onMes]sage", "[on]This[M]ega[Es]capes"));
   EXPECT_THAT("CC", ranks("[C]amel[C]ase", "[c]amel[C]ase"));
   EXPECT_THAT("cC", ranks("[c]amel[C]ase", "[C]amel[C]ase"));
   EXPECT_THAT("p", ranks("[p]", "[p]arse", "[p]osix", "[p]afdsa", "[p]ath"));
@@ -270,12 +273,18 @@
 // Verify some bounds so we know scores fall in the right range.
 // Testing exact scores is fragile, so we prefer Ranking tests.
 TEST(FuzzyMatch, Scoring) {
-  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 0.f));
+  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 7.f / 12.f));
   EXPECT_THAT("abs", matches("[abs]l", 1.f));
   EXPECT_THAT("abs", matches("[abs]", 2.f));
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+TEST(FuzzyMatch, InitialismAndPrefix) {
+  // We want these scores to be roughly the same.
+  EXPECT_THAT("up", matches("[u]nique_[p]tr", 3.f / 4.f));
+  EXPECT_THAT("up", matches("[up]per_bound", 1.f));
+}
+
 // Returns pretty-printed segmentation of Text.
 // e.g. std::basic_string --> +--  + +-
 std::string segment(llvm::StringRef Text) {
Index: clang-tools-extra/clangd/FuzzyMatch.cpp
===
--- clang-tools-extra/clangd/FuzzyMatch.cpp
+++ clang-tools-extra/clangd/FuzzyMatch.cpp
@@ -71,7 +71,7 @@
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
 static constexpr int AwfulScore = -(1 << 13);
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 
 FuzzyMatcher::FuzzyMatcher(llvm::StringRef Pattern)
 : PatN(std::min(MaxPat, Pattern.size())),
@@ -267,24 +267,31 @@
 }
 
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep this lower than a consecutive match bonus.
+  // Instead of penalizing non-consecutive matches, we give a bonus to a
+  // consecutive match in matchBonus. This produces a better score distribution
+  // than penalties in case of small patterns, e.g. 'up' for 'unique_ptr'.
+  return 0;
 }
 
 int FuzzyMatcher::matchBonus(int P, int W, Action Last) const {
   assert(LowPat[P] == LowWord[W]);
   int S = 1;
-  // Bonus: pattern so far is a (case-insensitive) prefix of the word.
-  if (P == W) // We can't skip pattern characters, so we must have matched all.
-++S;
+  bool IsPatSingleCase =
+  (PatTypeSet == 1 << Lower) || (PatTypeSet == 1 << Upper);
   // Bonus: case matches, or a Head in the pattern aligns with one in the word.
-  if ((Pat[P] == Word[W] && ((PatTypeSet & 1 << Upper) || P == W)) ||
-  (PatRole[P] == Head && WordRole[W] == Head))
+  // Single-case patterns lack segmentation signals and we assume any character
+  // can be a head of a segment.
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;
+  // Bonus: a consecutive match. First character match also gets a bonus to
+  // ensure prefix final match score normalizes to 1.0.
+  if (W == 0 || Last == Match)
+S += 2;
   // Penalty: matching inside a segment (and previous char wasn't matched).
   if (WordRole[W] == Tail && P && Last == Miss)
 S -= 3;


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@
   EXPECT_THAT("foo", ranks("[foo]", "[Foo

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D58418#1430490 , @jkorous wrote:

> In D58418#1430160 , @thakis wrote:
>
> > Why is this needed for index-while-building? My mental model for 
> > index-while-building is that that streams out build index metadata as part 
> > of the regular compile. Why does that require watching directories?
>
>
> You're right that this isn't necessary for the indexing phase. But we also 
> provide API so clients can consume the index. This functionality is used for 
> getting notifications about index data changes.
>
> You can see it for example here:
>  
> https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111


Is that code going to live in clang? This seems more like a tool built on top 
of the compiler rather than something core to the compiler itself (like the 
actual index-while-building feature). Maybe this could be in clang-tools-extra?


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

https://reviews.llvm.org/D58418



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

[clang-tools-extra] r356254 - Rename directory housing clang-change-namespace to be eponymous

[Nico Weber via cfe-commits]

Author: nico
Date: Fri Mar 15 04:54:01 2019
New Revision: 356254

URL: http://llvm.org/viewvc/llvm-project?rev=356254&view=rev
Log:
Rename directory housing clang-change-namespace to be eponymous

Makes the name of this directory consistent with the names of the other
directories in clang-tools-extra.

Differential Revision: https://reviews.llvm.org/D59382

Added:
clang-tools-extra/trunk/clang-change-namespace/
  - copied from r356253, clang-tools-extra/trunk/change-namespace/
clang-tools-extra/trunk/test/clang-change-namespace/
  - copied from r356253, clang-tools-extra/trunk/test/change-namespace/
clang-tools-extra/trunk/unittests/clang-change-namespace/
  - copied from r356253, clang-tools-extra/trunk/unittests/change-namespace/
Removed:
clang-tools-extra/trunk/change-namespace/
clang-tools-extra/trunk/test/change-namespace/
clang-tools-extra/trunk/unittests/change-namespace/
Modified:
clang-tools-extra/trunk/CMakeLists.txt
clang-tools-extra/trunk/unittests/CMakeLists.txt
clang-tools-extra/trunk/unittests/clang-change-namespace/CMakeLists.txt

Modified: clang-tools-extra/trunk/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/CMakeLists.txt?rev=356254&r1=356253&r2=356254&view=diff
==
--- clang-tools-extra/trunk/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/CMakeLists.txt Fri Mar 15 04:54:01 2019
@@ -9,7 +9,7 @@ add_subdirectory(modularize)
 add_subdirectory(clang-tidy)
 add_subdirectory(clang-tidy-vs)
 
-add_subdirectory(change-namespace)
+add_subdirectory(clang-change-namespace)
 add_subdirectory(clang-doc)
 add_subdirectory(clang-query)
 add_subdirectory(clang-move)

Modified: clang-tools-extra/trunk/unittests/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/CMakeLists.txt?rev=356254&r1=356253&r2=356254&view=diff
==
--- clang-tools-extra/trunk/unittests/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/unittests/CMakeLists.txt Fri Mar 15 04:54:01 2019
@@ -14,8 +14,8 @@ if(CLANG_BUILT_STANDALONE)
   endif()
 endif()
 
-add_subdirectory(change-namespace)
 add_subdirectory(clang-apply-replacements)
+add_subdirectory(clang-change-namespace)
 add_subdirectory(clang-doc)
 add_subdirectory(clang-move)
 add_subdirectory(clang-query)

Modified: 
clang-tools-extra/trunk/unittests/clang-change-namespace/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-change-namespace/CMakeLists.txt?rev=356254&r1=356253&r2=356254&view=diff
==
--- clang-tools-extra/trunk/unittests/clang-change-namespace/CMakeLists.txt 
(original)
+++ clang-tools-extra/trunk/unittests/clang-change-namespace/CMakeLists.txt Fri 
Mar 15 04:54:01 2019
@@ -3,7 +3,7 @@ set(LLVM_LINK_COMPONENTS
   )
 
 get_filename_component(CHANGE_NAMESPACE_SOURCE_DIR
-  ${CMAKE_CURRENT_SOURCE_DIR}/../../change-namespace REALPATH)
+  ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-change-namespace REALPATH)
 include_directories(
   ${CHANGE_NAMESPACE_SOURCE_DIR}
   )


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

[PATCH] D59361: [CUDA][Windows] Partial fix for bug 38811 (Step 1 of 3)

[Evgeny Mankov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356255: [CUDA][Windows] Partial fix for bug #38811 (Step 1 
of 3) (authored by emankov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59361?vs=190607&id=190809#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59361

Files:
  lib/Headers/__clang_cuda_device_functions.h


Index: lib/Headers/__clang_cuda_device_functions.h
===
--- lib/Headers/__clang_cuda_device_functions.h
+++ lib/Headers/__clang_cuda_device_functions.h
@@ -1563,7 +1563,7 @@
 __DEVICE__ float j1f(float __a) { return __nv_j1f(__a); }
 __DEVICE__ double jn(int __n, double __a) { return __nv_jn(__n, __a); }
 __DEVICE__ float jnf(int __n, float __a) { return __nv_jnf(__n, __a); }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long labs(long __a) { return llabs(__a); };
 #else
 __DEVICE__ long labs(long __a) { return __nv_abs(__a); };
@@ -1597,7 +1597,7 @@
 __DEVICE__ float logf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_logf, __nv_logf)(__a);
 }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long lrint(double __a) { return llrint(__a); }
 __DEVICE__ long lrintf(float __a) { return __float2ll_rn(__a); }
 __DEVICE__ long lround(double __a) { return llround(__a); }


Index: lib/Headers/__clang_cuda_device_functions.h
===
--- lib/Headers/__clang_cuda_device_functions.h
+++ lib/Headers/__clang_cuda_device_functions.h
@@ -1563,7 +1563,7 @@
 __DEVICE__ float j1f(float __a) { return __nv_j1f(__a); }
 __DEVICE__ double jn(int __n, double __a) { return __nv_jn(__n, __a); }
 __DEVICE__ float jnf(int __n, float __a) { return __nv_jnf(__n, __a); }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long labs(long __a) { return llabs(__a); };
 #else
 __DEVICE__ long labs(long __a) { return __nv_abs(__a); };
@@ -1597,7 +1597,7 @@
 __DEVICE__ float logf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_logf, __nv_logf)(__a);
 }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long lrint(double __a) { return llrint(__a); }
 __DEVICE__ long lrintf(float __a) { return __float2ll_rn(__a); }
 __DEVICE__ long lround(double __a) { return llround(__a); }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r356255 - [CUDA][Windows] Partial fix for bug #38811 (Step 1 of 3)

[Evgeny Mankov via cfe-commits]

Author: emankov
Date: Fri Mar 15 05:05:36 2019
New Revision: 356255

URL: http://llvm.org/viewvc/llvm-project?rev=356255&view=rev
Log:
[CUDA][Windows] Partial fix for bug #38811 (Step 1 of 3)

Partial fix for the clang Bug https://bugs.llvm.org/show_bug.cgi?id=38811 
"Clang fails to compile with CUDA-9.x on Windows".

Adding defined(_WIN64) check along with existing #if defined(__LP64__) 
eliminates the below clang (64-bit) compilation error on Windows.

C:/GIT/LLVM/trunk/llvm-64-release-vs2017/dist/lib/clang/9.0.0\include\__clang_cuda_device_functions.h(1609,45):
 error GEF7559A7: no matching function for call to 'roundf'
 __DEVICE__ long lroundf(float __a) { return roundf(__a); }

Reviewed by: Artem Belevich

Differential Revision: http://reviews.llvm.org/D59361

Modified:
cfe/trunk/lib/Headers/__clang_cuda_device_functions.h

Modified: cfe/trunk/lib/Headers/__clang_cuda_device_functions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/__clang_cuda_device_functions.h?rev=356255&r1=356254&r2=356255&view=diff
==
--- cfe/trunk/lib/Headers/__clang_cuda_device_functions.h (original)
+++ cfe/trunk/lib/Headers/__clang_cuda_device_functions.h Fri Mar 15 05:05:36 
2019
@@ -1563,7 +1563,7 @@ __DEVICE__ double j1(double __a) { retur
 __DEVICE__ float j1f(float __a) { return __nv_j1f(__a); }
 __DEVICE__ double jn(int __n, double __a) { return __nv_jn(__n, __a); }
 __DEVICE__ float jnf(int __n, float __a) { return __nv_jnf(__n, __a); }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long labs(long __a) { return llabs(__a); };
 #else
 __DEVICE__ long labs(long __a) { return __nv_abs(__a); };
@@ -1597,7 +1597,7 @@ __DEVICE__ float logbf(float __a) { retu
 __DEVICE__ float logf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_logf, __nv_logf)(__a);
 }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long lrint(double __a) { return llrint(__a); }
 __DEVICE__ long lrintf(float __a) { return __float2ll_rn(__a); }
 __DEVICE__ long lround(double __a) { return llround(__a); }


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

[PATCH] D59407: [clangd] Add RelationSlab

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

This mostly looks good, one high level comment:
I believe it makes sense to deduplicate SymbolIDs for RelationSlab.
Up until now, we mostly had only one occurence of a SymbolID in a Slab, but 
RelationSlab does not follow that assumption.

Also can you add a few tests after the above mentioned check to make sure 
interning SymbolIDs works as expected?




Comment at: clang-tools-extra/clangd/index/Index.h:25
 
+enum class RelationKind { Subtype };
+

nridge wrote:
> One thing I'm wondering about is: would it be better to just use 
> `clang::index::SymbolRole` here (which has `Relation___Of` entries that cover 
> what we're interested in) instead of having our own enum?
I totally agree, but that struct is 4 bytes. I am not sure it is worth the 
trade off when storing the relationslab, but since this patch is not related to 
serialization let's get rid of RelationKind and make use of 
clang::index::SymbolRole.

When landing serialization part we can either split clang::index::SymbolRole 
into two parts or add some custom mapping to serialize only relevant bits etc.



Comment at: clang-tools-extra/clangd/index/Index.h:59
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.size();
+  }

use capacity instead of size



Comment at: clang-tools-extra/clangd/index/Index.h:67
+Builder() {}
+// Adds a relation to the slab. Deep copy: Strings will be owned by the
+// slab.

I am not sure comment currently applies to RelationSlab internals, since 
everything is of value type anyways, there are no references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:27
+/// be invoked even if the directory is empty.
+class DirectoryWatcher {
+public:

I feel like since there's nothing Clang-specific about it, it should go into 
libSupport in LLVM, what do you think?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:33
+/// A file gets moved into the directory and replaces an existing file
+/// with the same name will trigger an 'Added' event but no 'Removed' 
event.
+/// If a file gets replaced multiple times within a short time period, it

"A file being moved into ... and replacing ... "



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:43
+Removed,
+/// A file was modified.
+Modified,

"File content was modified." ?  In other words, metadata was not modified?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:46
+/// The watched directory got deleted. No more events will follow.
+DirectoryDeleted,
+  };

`DirectoryRemoved` (for consistency with `Removed`)

Also, how about `TopDirectoryRemoved`?  Otherwise it sounds like some random 
directory was removed.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:51
+EventKind Kind;
+std::string Filename;
+  };

Please document if it is going to be absolute or relative path, or just the 
file name.




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:55
+  typedef std::function Events, bool isInitial)>
+  EventReceiver;
+

Users are not going to use this typedef in their code, so I feel like it is 
obfuscating code -- could you expand it in places where it is used?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:61
+  EventReceiver Receiver,
+  bool waitInitialSync,
+  std::string &Error);

`WaitForInitialSync` (everywhere in the patch)



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:62
+  bool waitInitialSync,
+  std::string &Error);
+

Why not return `llvm::ErrorOr>`?

I also don't understand why we need unique_ptr.  If you change `Implementation 
&Impl;` to `std::unique_ptr Impl;`, `DirectoryWatcher` becomes 
a move-only type, and `create` can return `llvm::ErrorOr`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:1
+//===- DirectoryWatcher-linux.inc.h - Linux-platform directory listening 
--===//
+//

Please drop the `.h` from the name, for consistency with the rest of LLVM 
(there are no `.inc.h` files, only `.inc`).



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:93
+  30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  char buf[EventBufferLength] __attribute__((aligned(8)));
+

Please use `AlignedCharArray` from `llvm/include/llvm/Support/AlignOf.h`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:93
+  30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  char buf[EventBufferLength] __attribute__((aligned(8)));
+

gribozavr wrote:
> Please use `AlignedCharArray` from `llvm/include/llvm/Support/AlignOf.h`.
NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get 
almost 8 Kb on the stack.  Should we allocate on the heap instead?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:137
+
+bool DirectoryWatcher::Implementation::initialize(StringRef Path,
+  EventReceiver Receiver,

Return `llvm::Error`?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:162
+  std::thread watchThread(
+  std::bind(runWatcher, pathToWatch, inotifyFD, evtQueue));
+  watchThread.detach();

Use a lambda instead of bind to improve readability?  
https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:175
+std::thread scanThread(runScan);
+scanThread.detach();
+  }

Same comments as for macOS: we need to buffer events from inotify until the 
initial scan completes.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h:1
+//===- DirectoryWatcher-mac.inc.h - Mac-platform director

[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.cpp:35
+auto *Array = Arena.Allocate(Rels.size());
+std::uninitialized_copy(Rels.begin(), Rels.end(), Array);
+Result.emplace_back(Entry.first,

Use `ArrayRef::copy()`, for example: https://reviews.llvm.org/D58782




Comment at: clang-tools-extra/clangd/index/Index.h:43
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;

`struct Relation`?  And in the comments for it, please explain which way the 
relationship is directed (is the SymbolID in the key the subtype?  or is the 
SymbolID in the ArrayRef the subtype?).



Comment at: clang-tools-extra/clangd/index/Index.h:88
+  size_t NumRelations = 0;
+};
+

Please move all new declarations into `Relation.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D59407: [clangd] Add RelationSlab

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.h:43
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;

gribozavr wrote:
> `struct Relation`?  And in the comments for it, please explain which way the 
> relationship is directed (is the SymbolID in the key the subtype?  or is the 
> SymbolID in the ArrayRef the subtype?).
Ah exactly my thoughts, forget to mention this.

I believe current usage is the counter-intuitive one. For example, we will most 
likely query with something like:
`getRelations(SymbolID, baseOf)` to get all relations where `SymbolID` is 
`baseOf` something else(which says get children of `SymbolID`)

So that this valueType can be read like, 
```
`SymbolID` is `RelationKind` every `SymbolID inside array`
```
WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D59413: Fix isInSystemMacro in presence of macro and pasted token

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When a warning is raised from the expansion of a  system macro that involves 
pasted token, there was still situations were they were not skipped, as 
showcased by this issue: https://bugzilla.redhat.com/show_bug.cgi?id=1472437


Repository:
  rC Clang

https://reviews.llvm.org/D59413

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/test/Misc/no-warn-in-system-macro.c


Index: clang/test/Misc/no-warn-in-system-macro.c
===
--- clang/test/Misc/no-warn-in-system-macro.c
+++ clang/test/Misc/no-warn-in-system-macro.c
@@ -3,11 +3,16 @@
 
 #include 
 
+#define MACRO(x) x
+
 int main(void)
 {
double foo = 1.0;
 
if (isnan(foo))
return 1;
+
+   MACRO(isnan(foo));
+
return 0;
 }
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1463,8 +1463,11 @@
 
 // This happens when the macro is the result of a paste, in that case
 // its spelling is the scratch memory, so we take the parent context.
-if (isWrittenInScratchSpace(getSpellingLoc(loc)))
-  return isInSystemHeader(getSpellingLoc(getImmediateMacroCallerLoc(loc)));
+// There can be several level of token pasting.
+if (isWrittenInScratchSpace(getSpellingLoc(loc))) {
+  do { loc = getImmediateMacroCallerLoc(loc); } while 
(isWrittenInScratchSpace(getSpellingLoc(loc)));
+  return isInSystemMacro(loc);
+}
 
 return isInSystemHeader(getSpellingLoc(loc));
   }


Index: clang/test/Misc/no-warn-in-system-macro.c
===
--- clang/test/Misc/no-warn-in-system-macro.c
+++ clang/test/Misc/no-warn-in-system-macro.c
@@ -3,11 +3,16 @@
 
 #include 
 
+#define MACRO(x) x
+
 int main(void)
 {
 	double foo = 1.0;
 
 	if (isnan(foo))
 		return 1;
+
+	MACRO(isnan(foo));
+
 	return 0;
 }
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1463,8 +1463,11 @@
 
 // This happens when the macro is the result of a paste, in that case
 // its spelling is the scratch memory, so we take the parent context.
-if (isWrittenInScratchSpace(getSpellingLoc(loc)))
-  return isInSystemHeader(getSpellingLoc(getImmediateMacroCallerLoc(loc)));
+// There can be several level of token pasting.
+if (isWrittenInScratchSpace(getSpellingLoc(loc))) {
+  do { loc = getImmediateMacroCallerLoc(loc); } while (isWrittenInScratchSpace(getSpellingLoc(loc)));
+  return isInSystemMacro(loc);
+}
 
 return isInSystemHeader(getSpellingLoc(loc));
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r356261 - [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Fri Mar 15 07:00:49 2019
New Revision: 356261

URL: http://llvm.org/viewvc/llvm-project?rev=356261&view=rev
Log:
[clangd] Tune the fuzzy-matching algorithm

Summary:
To reduce the gap between prefix and initialism matches.

The motivation is producing better scoring in one particular example,
but the change does not seem to cause large regressions in other cases.

The examples is matching 'up' against 'unique_ptr' and 'upper_bound'.
Before the change, we had:
  - "[u]nique_[p]tr" with a score of 0.3,
  - "[up]per_bound" with a score of 1.0.

A 3x difference meant that symbol quality signals were almost always ignored
and 'upper_bound' was always ranked higher.

However, intuitively, the match scores should be very close for the two.

After the change we have the following scores:
- "[u]nique_[p]tr" with a score of 0.75,
- "[up]per_bound" with a score of 1.0.

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59300

Modified:
clang-tools-extra/trunk/clangd/FuzzyMatch.cpp
clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp

Modified: clang-tools-extra/trunk/clangd/FuzzyMatch.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FuzzyMatch.cpp?rev=356261&r1=356260&r2=356261&view=diff
==
--- clang-tools-extra/trunk/clangd/FuzzyMatch.cpp (original)
+++ clang-tools-extra/trunk/clangd/FuzzyMatch.cpp Fri Mar 15 07:00:49 2019
@@ -71,7 +71,7 @@ static char lower(char C) { return C >=
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
 static constexpr int AwfulScore = -(1 << 13);
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 
 FuzzyMatcher::FuzzyMatcher(llvm::StringRef Pattern)
 : PatN(std::min(MaxPat, Pattern.size())),
@@ -267,24 +267,31 @@ bool FuzzyMatcher::allowMatch(int P, int
 }
 
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep this lower than a consecutive match bonus.
+  // Instead of penalizing non-consecutive matches, we give a bonus to a
+  // consecutive match in matchBonus. This produces a better score distribution
+  // than penalties in case of small patterns, e.g. 'up' for 'unique_ptr'.
+  return 0;
 }
 
 int FuzzyMatcher::matchBonus(int P, int W, Action Last) const {
   assert(LowPat[P] == LowWord[W]);
   int S = 1;
-  // Bonus: pattern so far is a (case-insensitive) prefix of the word.
-  if (P == W) // We can't skip pattern characters, so we must have matched all.
-++S;
+  bool IsPatSingleCase =
+  (PatTypeSet == 1 << Lower) || (PatTypeSet == 1 << Upper);
   // Bonus: case matches, or a Head in the pattern aligns with one in the word.
-  if ((Pat[P] == Word[W] && ((PatTypeSet & 1 << Upper) || P == W)) ||
-  (PatRole[P] == Head && WordRole[W] == Head))
+  // Single-case patterns lack segmentation signals and we assume any character
+  // can be a head of a segment.
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;
+  // Bonus: a consecutive match. First character match also gets a bonus to
+  // ensure prefix final match score normalizes to 1.0.
+  if (W == 0 || Last == Match)
+S += 2;
   // Penalty: matching inside a segment (and previous char wasn't matched).
   if (WordRole[W] == Tail && P && Last == Miss)
 S -= 3;

Modified: clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp?rev=356261&r1=356260&r2=356261&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp Fri Mar 15 
07:00:49 2019
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@ TEST(FuzzyMatch, Ranking) {
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
   ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));
+  EXPECT_THAT("onmes",
+  ranks("[onmes]sage", "[onMes]sage", "[on]This[M]ega[Es]capes"));
   EXPECT_THAT("CC", ranks("[C]amel[C]ase", "[c]amel[C]ase"));
   EXPECT_THAT("cC", ranks("[c]amel[C]ase", "[C]amel[C]ase"));
   EXPE

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356261: [clangd] Tune the fuzzy-matching algorithm (authored 
by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59300?vs=190804&id=190821#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59300

Files:
  clang-tools-extra/trunk/clangd/FuzzyMatch.cpp
  clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -247,6 +248,8 @@
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
   ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));
+  EXPECT_THAT("onmes",
+  ranks("[onmes]sage", "[onMes]sage", "[on]This[M]ega[Es]capes"));
   EXPECT_THAT("CC", ranks("[C]amel[C]ase", "[c]amel[C]ase"));
   EXPECT_THAT("cC", ranks("[c]amel[C]ase", "[C]amel[C]ase"));
   EXPECT_THAT("p", ranks("[p]", "[p]arse", "[p]osix", "[p]afdsa", "[p]ath"));
@@ -270,12 +273,18 @@
 // Verify some bounds so we know scores fall in the right range.
 // Testing exact scores is fragile, so we prefer Ranking tests.
 TEST(FuzzyMatch, Scoring) {
-  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 0.f));
+  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 7.f / 12.f));
   EXPECT_THAT("abs", matches("[abs]l", 1.f));
   EXPECT_THAT("abs", matches("[abs]", 2.f));
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+TEST(FuzzyMatch, InitialismAndPrefix) {
+  // We want these scores to be roughly the same.
+  EXPECT_THAT("up", matches("[u]nique_[p]tr", 3.f / 4.f));
+  EXPECT_THAT("up", matches("[up]per_bound", 1.f));
+}
+
 // Returns pretty-printed segmentation of Text.
 // e.g. std::basic_string --> +--  + +-
 std::string segment(llvm::StringRef Text) {
Index: clang-tools-extra/trunk/clangd/FuzzyMatch.cpp
===
--- clang-tools-extra/trunk/clangd/FuzzyMatch.cpp
+++ clang-tools-extra/trunk/clangd/FuzzyMatch.cpp
@@ -71,7 +71,7 @@
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
 static constexpr int AwfulScore = -(1 << 13);
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 
 FuzzyMatcher::FuzzyMatcher(llvm::StringRef Pattern)
 : PatN(std::min(MaxPat, Pattern.size())),
@@ -267,24 +267,31 @@
 }
 
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep this lower than a consecutive match bonus.
+  // Instead of penalizing non-consecutive matches, we give a bonus to a
+  // consecutive match in matchBonus. This produces a better score distribution
+  // than penalties in case of small patterns, e.g. 'up' for 'unique_ptr'.
+  return 0;
 }
 
 int FuzzyMatcher::matchBonus(int P, int W, Action Last) const {
   assert(LowPat[P] == LowWord[W]);
   int S = 1;
-  // Bonus: pattern so far is a (case-insensitive) prefix of the word.
-  if (P == W) // We can't skip pattern characters, so we must have matched all.
-++S;
+  bool IsPatSingleCase =
+  (PatTypeSet == 1 << Lower) || (PatTypeSet == 1 << Upper);
   // Bonus: case matches, or a Head in the pattern aligns with one in the word.
-  if ((Pat[P] == Word[W] && ((PatTypeSet & 1 << Upper) || P == W)) ||
-  (PatRole[P] == Head && WordRole[W] == Head))
+  // Single-case patterns lack segmentation signals and we assume any character
+  // can be a head of a segment.
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;
+  // Bonus: a consecutive match. First character match also gets a bonus to
+  // ensure prefix final match score normalizes to 1.0.
+  if (W == 0 || Last == Match)
+S += 2;
   // Penalty: matching inside a segment (and previous char wasn't matched).
   if (WordRole[W] == Tail && P && Last == Miss)
 S -= 3;


Index: clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/trunk/

[PATCH] D59336: [clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/google-runtime-int.m:1
+// RUN: clang-tidy -checks=-*,google-runtime-int %s 2>&1 -- -x objective-c | 
not grep 'warning:\|error:'
+

We typically use `| count 0` instead of an explicit grep. Also, do you really 
need to pass `-x objective-c`?



Comment at: clang-tools-extra/test/clang-tidy/google-runtime-int.mm:1
+// RUN: clang-tidy -checks=-*,google-runtime-int %s -- -x objective-c++ 2>&1 | 
not grep 'warning:\|error:'
+

Given that the contents of the file are the same, I would just add this RUN 
line to the previous test case (changing to `| count 0`). You will still need 
the `-x objective-c++` in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59336



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

[clang-tools-extra] r356264 - [clangd] Remove includes of "gmock-matchers.h". NFC

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Fri Mar 15 07:30:07 2019
New Revision: 356264

URL: http://llvm.org/viewvc/llvm-project?rev=356264&view=rev
Log:
[clangd] Remove includes of "gmock-matchers.h". NFC

For consistency with most of the test code.

Modified:
clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp
clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp?rev=356264&r1=356263&r2=356264&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp Fri Mar 15 
07:30:07 2019
@@ -12,7 +12,6 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "llvm/ADT/StringRef.h"
-#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 

Modified: clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp?rev=356264&r1=356263&r2=356264&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/FuzzyMatchTests.cpp Fri Mar 15 
07:30:07 2019
@@ -9,7 +9,6 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
-#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 


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

[PATCH] D59318: [clang-tidy] add an overload for diag method that allows users to provide a diagnostic name rather than using the check name when building a diagnostic.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

What is the reason you want this change to happen? I think this gives the 
chance to create inconsistencies which we should avoid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59318



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

[PATCH] D59296: [pp-trace] Delete -ignore and add generalized -callbacks

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think the release notes should be updated to mention that a 
previously-supported option has been removed and that there's a new option 
available as a replacement; I don't think we have any other docs for pp-trace 
to worry about updating, do we?




Comment at: pp-trace/PPTrace.cpp:70-71
+ "to output. Globs are processed in order of appearance. Globs "
+ "with the '-' "
+ "prefix remove callbacks from the set. e.g. '*,-Macro*'."));
 

Can re-flow the string literal.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59296



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

[PATCH] D56343: [clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

From my side its LGTM, but I would let @serge-sans-paille accept, as he is 
probably more familiar with python then I am.


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

https://reviews.llvm.org/D56343



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

[PATCH] D59219: [PR41007][OpenCL] Allow printf and toolchain reserved variadic functions in C++

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

> May be test/Driver/include-default-header.cl would make more sense to show 
> the intent?

test/Headers/opencl-c-header.cl sounds like good candidate.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59219



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

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

ikudrin created this revision.
ikudrin added reviewers: ioeric, bkramer, hans, thakis, rsmith, zturner.
Herald added a project: clang.

This patch partially reverts D46942 .

If the source file path contains directory junctions, and we resolve them when 
printing
diagnostic messages, these paths look independent for an IDE. For example, both 
Visual
Studio and Visual Studio Code open separate editors for such paths, which is 
not only
inconvenient but might even result in loss of the changes made in one of them.


Repository:
  rC Clang

https://reviews.llvm.org/D59415

Files:
  lib/Basic/FileManager.cpp
  test/Frontend/absolute-paths-windows.test
  test/Frontend/lit.local.cfg


Index: test/Frontend/lit.local.cfg
===
--- test/Frontend/lit.local.cfg
+++ test/Frontend/lit.local.cfg
@@ -1 +1 @@
-config.suffixes = ['.c', '.cpp', '.m', '.mm', '.ll', '.cl']
+config.suffixes = ['.c', '.cpp', '.m', '.mm', '.ll', '.cl', '.test']
Index: test/Frontend/absolute-paths-windows.test
===
--- /dev/null
+++ test/Frontend/absolute-paths-windows.test
@@ -0,0 +1,9 @@
+// REQUIRES: system-windows
+// RUN: rm -rf %t.dir
+// RUN: mkdir -p %t.dir\real
+// RUN: cmd /c mklink /j %t.dir\junc %t.dir\real
+// RUN: echo "wrong code" > %t.dir\real\foo.cpp
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-absolute-paths 
%t.dir\junc\foo.cpp 2>&1 | FileCheck %s
+
+// CHECK-NOT: .dir\real\foo.cpp
+// CHECK: .dir\junc\foo.cpp
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -497,8 +497,22 @@
   StringRef CanonicalName(Dir->getName());
 
   SmallString<4096> CanonicalNameBuf;
+#ifdef _WIN32
+  CanonicalNameBuf = CanonicalName;
+  llvm::sys::fs::make_absolute(CanonicalNameBuf);
+  llvm::sys::path::native(CanonicalNameBuf);
+  // We've run into needing to remove '..' here in the wild though, so
+  // remove it.
+  // On Windows, symlinks are significantly less prevalent, so removing
+  // '..' is pretty safe.
+  // Ideally we'd have an equivalent of `realpath` and could implement
+  // sys::fs::canonical across all the platforms.
+  llvm::sys::path::remove_dots(CanonicalNameBuf, /* remove_dot_dot */ true);
+  CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+#else
   if (!FS->getRealPath(Dir->getName(), CanonicalNameBuf))
 CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+#endif
 
   CanonicalDirNames.insert({Dir, CanonicalName});
   return CanonicalName;


Index: test/Frontend/lit.local.cfg
===
--- test/Frontend/lit.local.cfg
+++ test/Frontend/lit.local.cfg
@@ -1 +1 @@
-config.suffixes = ['.c', '.cpp', '.m', '.mm', '.ll', '.cl']
+config.suffixes = ['.c', '.cpp', '.m', '.mm', '.ll', '.cl', '.test']
Index: test/Frontend/absolute-paths-windows.test
===
--- /dev/null
+++ test/Frontend/absolute-paths-windows.test
@@ -0,0 +1,9 @@
+// REQUIRES: system-windows
+// RUN: rm -rf %t.dir
+// RUN: mkdir -p %t.dir\real
+// RUN: cmd /c mklink /j %t.dir\junc %t.dir\real
+// RUN: echo "wrong code" > %t.dir\real\foo.cpp
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-absolute-paths %t.dir\junc\foo.cpp 2>&1 | FileCheck %s
+
+// CHECK-NOT: .dir\real\foo.cpp
+// CHECK: .dir\junc\foo.cpp
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -497,8 +497,22 @@
   StringRef CanonicalName(Dir->getName());
 
   SmallString<4096> CanonicalNameBuf;
+#ifdef _WIN32
+  CanonicalNameBuf = CanonicalName;
+  llvm::sys::fs::make_absolute(CanonicalNameBuf);
+  llvm::sys::path::native(CanonicalNameBuf);
+  // We've run into needing to remove '..' here in the wild though, so
+  // remove it.
+  // On Windows, symlinks are significantly less prevalent, so removing
+  // '..' is pretty safe.
+  // Ideally we'd have an equivalent of `realpath` and could implement
+  // sys::fs::canonical across all the platforms.
+  llvm::sys::path::remove_dots(CanonicalNameBuf, /* remove_dot_dot */ true);
+  CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+#else
   if (!FS->getRealPath(Dir->getName(), CanonicalNameBuf))
 CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+#endif
 
   CanonicalDirNames.insert({Dir, CanonicalName});
   return CanonicalName;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59233: libclang/CIndexer.cpp: Use loadquery() on AIX for path to library

[Xing Xue via Phabricator via cfe-commits]

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

LGTM.  Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59233



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

[PATCH] D59418: [OpenMP][Offloading] Extract common functionality

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: ABataev, arpith-jacob, guraypp, gtbercea, hfinkel.
jdoerfert added a project: OpenMP.
Herald added a project: clang.

This patch introduces the CGOpenMPRuntimeTarget class to collect helpers
and functionality common to all target offloading code generation
schemes. All initial members have been taken from the NVPTX code
generation and removed there.

This is a preperation patch for https://reviews.llvm.org/D59328


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59418

Files:
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.h
  clang/lib/CodeGen/CGOpenMPRuntimeTarget.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
  clang/lib/CodeGen/CMakeLists.txt

Index: clang/lib/CodeGen/CMakeLists.txt
===
--- clang/lib/CodeGen/CMakeLists.txt
+++ clang/lib/CodeGen/CMakeLists.txt
@@ -69,6 +69,7 @@
   CGOpenCLRuntime.cpp
   CGOpenMPRuntime.cpp
   CGOpenMPRuntimeNVPTX.cpp
+  CGOpenMPRuntimeTarget.cpp
   CGRecordLayoutBuilder.cpp
   CGStmt.cpp
   CGStmtOpenMP.cpp
Index: clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
===
--- /dev/null
+++ clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
@@ -0,0 +1,104 @@
+//===-- CGOpenMPRuntimeTarget.h --- Common OpenMP target codegen --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Code common to all OpenMP target codegens.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_CODEGEN_CGOPENMPRUNTIMETARGET_H
+#define LLVM_CLANG_LIB_CODEGEN_CGOPENMPRUNTIMETARGET_H
+
+#include "CGOpenMPRuntime.h"
+
+namespace clang {
+namespace CodeGen {
+
+struct CGOpenMPRuntimeTarget : public CGOpenMPRuntime {
+
+  explicit CGOpenMPRuntimeTarget(CodeGenModule &CGM);
+
+  /// Defines the execution mode.
+  enum ExecutionMode {
+/// SPMD execution mode (all threads are worker threads).
+EM_SPMD,
+/// Non-SPMD execution mode (1 master thread, others are workers).
+EM_NonSPMD,
+/// Unknown execution mode (orphaned directive).
+EM_Unknown,
+  };
+
+  /// Return the execution mode, if not overloaded this is always Unknown.
+  virtual ExecutionMode getExecutionMode() const { return EM_Unknown; }
+
+  /// Return the value decleration encapsulated in the expression \p E.
+  static const ValueDecl *getUnderlyingVar(const Expr *E);
+
+  //
+  // Base class overrides.
+  //
+
+  /// Creates offloading entry for the provided entry ID \a ID,
+  /// address \a Addr, size \a Size, and flags \a Flags.
+  void createOffloadEntry(llvm::Constant *ID, llvm::Constant *Addr,
+  uint64_t Size, int32_t Flags,
+  llvm::GlobalValue::LinkageTypes Linkage) override;
+
+  /// Emit call to void __kmpc_push_proc_bind(ident_t *loc, kmp_int32
+  /// global_tid, int proc_bind) to generate code for 'proc_bind' clause.
+  virtual void emitProcBindClause(CodeGenFunction &CGF,
+  OpenMPProcBindClauseKind ProcBind,
+  SourceLocation Loc) override;
+
+  /// Emits call to void __kmpc_push_num_threads(ident_t *loc, kmp_int32
+  /// global_tid, kmp_int32 num_threads) to generate code for 'num_threads'
+  /// clause.
+  /// \param NumThreads An integer value of threads.
+  virtual void emitNumThreadsClause(CodeGenFunction &CGF,
+llvm::Value *NumThreads,
+SourceLocation Loc) override;
+
+  /// Set the number of teams to \p NumTeams and the thread limit to
+  /// \p ThreadLimit.
+  ///
+  /// \param NumTeams An integer expression of teams.
+  /// \param ThreadLimit An integer expression of threads.
+  void emitNumTeamsClause(CodeGenFunction &CGF, const Expr *NumTeams,
+  const Expr *ThreadLimit, SourceLocation Loc) override;
+
+  /// Choose a default value for the schedule clause.
+  void getDefaultScheduleAndChunk(CodeGenFunction &CGF,
+  const OMPLoopDirective &S,
+  OpenMPScheduleClauseKind &ScheduleKind,
+  const Expr *&ChunkExpr) const override;
+
+  /// Emits code for teams call of the \a OutlinedFn with
+  /// variables captured in a record which address is stored in \a
+  /// CapturedStruct.
+  /// \param OutlinedFn Outlined function to be run by team masters. Type of
+  /// this function is void(*)(kmp_int32 *, kmp_int32, struct context_vars*).
+  /// \param CapturedVars A pointer to the record with the references to
+  /// variables used in \a OutlinedFn function.
+  ///
+  

[PATCH] D59420: [NFC][OpenMP] Move runtime function generation to the target codegen

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: ABataev, arpith-jacob, guraypp, gtbercea, hfinkel.
jdoerfert added projects: OpenMP, clang.
jdoerfert added a parent revision: D59418: [OpenMP][Offloading] Extract common 
functionality.

This commit simply moves the runtime function generation from the NVPTX
to the common target code generation.

This is a preperation patch for https://reviews.llvm.org/D59328


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59420

Files:
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.h
  clang/lib/CodeGen/CGOpenMPRuntimeTarget.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeTarget.h

Index: clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
===
--- clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
+++ clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
@@ -38,6 +38,95 @@
   /// Return the value decleration encapsulated in the expression \p E.
   static const ValueDecl *getUnderlyingVar(const Expr *E);
 
+  enum OpenMPRTLTargetFunctions {
+/// Call to void __kmpc_kernel_init(kmp_int32 thread_limit,
+/// int16_t RequiresOMPRuntime);
+OMPRTL_NVPTX__kmpc_kernel_init,
+/// Call to void __kmpc_kernel_deinit(int16_t IsOMPRuntimeInitialized);
+OMPRTL_NVPTX__kmpc_kernel_deinit,
+/// Call to void __kmpc_spmd_kernel_init(kmp_int32 thread_limit,
+/// int16_t RequiresOMPRuntime, int16_t RequiresDataSharing);
+OMPRTL_NVPTX__kmpc_spmd_kernel_init,
+/// Call to void __kmpc_spmd_kernel_deinit_v2(int16_t RequiresOMPRuntime);
+OMPRTL_NVPTX__kmpc_spmd_kernel_deinit_v2,
+/// Call to void __kmpc_kernel_prepare_parallel(void
+/// *outlined_function, int16_t
+/// IsOMPRuntimeInitialized);
+OMPRTL_NVPTX__kmpc_kernel_prepare_parallel,
+/// Call to bool __kmpc_kernel_parallel(void **outlined_function,
+/// int16_t IsOMPRuntimeInitialized);
+OMPRTL_NVPTX__kmpc_kernel_parallel,
+/// Call to void __kmpc_kernel_end_parallel();
+OMPRTL_NVPTX__kmpc_kernel_end_parallel,
+/// Call to void __kmpc_serialized_parallel(ident_t *loc, kmp_int32
+/// global_tid);
+OMPRTL_NVPTX__kmpc_serialized_parallel,
+/// Call to void __kmpc_end_serialized_parallel(ident_t *loc, kmp_int32
+/// global_tid);
+OMPRTL_NVPTX__kmpc_end_serialized_parallel,
+/// Call to int32_t __kmpc_shuffle_int32(int32_t element,
+/// int16_t lane_offset, int16_t warp_size);
+OMPRTL_NVPTX__kmpc_shuffle_int32,
+/// Call to int64_t __kmpc_shuffle_int64(int64_t element,
+/// int16_t lane_offset, int16_t warp_size);
+OMPRTL_NVPTX__kmpc_shuffle_int64,
+/// Call to __kmpc_nvptx_parallel_reduce_nowait_v2(ident_t *loc, kmp_int32
+/// global_tid, kmp_int32 num_vars, size_t reduce_size, void* reduce_data,
+/// void (*kmp_ShuffleReductFctPtr)(void *rhsData, int16_t lane_id, int16_t
+/// lane_offset, int16_t shortCircuit),
+/// void (*kmp_InterWarpCopyFctPtr)(void* src, int32_t warp_num));
+OMPRTL_NVPTX__kmpc_nvptx_parallel_reduce_nowait_v2,
+/// Call to __kmpc_nvptx_teams_reduce_nowait_v2(ident_t *loc, kmp_int32
+/// global_tid, void *global_buffer, int32_t num_of_records, void*
+/// reduce_data,
+/// void (*kmp_ShuffleReductFctPtr)(void *rhsData, int16_t lane_id, int16_t
+/// lane_offset, int16_t shortCircuit),
+/// void (*kmp_InterWarpCopyFctPtr)(void* src, int32_t warp_num), void
+/// (*kmp_ListToGlobalCpyFctPtr)(void *buffer, int idx, void *reduce_data),
+/// void (*kmp_GlobalToListCpyFctPtr)(void *buffer, int idx,
+/// void *reduce_data), void (*kmp_GlobalToListCpyPtrsFctPtr)(void *buffer,
+/// int idx, void *reduce_data), void (*kmp_GlobalToListRedFctPtr)(void
+/// *buffer, int idx, void *reduce_data));
+OMPRTL_NVPTX__kmpc_nvptx_teams_reduce_nowait_v2,
+/// Call to __kmpc_nvptx_end_reduce_nowait(int32_t global_tid);
+OMPRTL_NVPTX__kmpc_end_reduce_nowait,
+/// Call to void __kmpc_data_sharing_init_stack();
+OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+/// Call to void __kmpc_data_sharing_init_stack_spmd();
+OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
+/// Call to void* __kmpc_data_sharing_coalesced_push_stack(size_t size,
+/// int16_t UseSharedMemory);
+OMPRTL_NVPTX__kmpc_data_sharing_coalesced_push_stack,
+/// Call to void __kmpc_data_sharing_pop_stack(void *a);
+OMPRTL_NVPTX__kmpc_data_sharing_pop_stack,
+/// Call to void __kmpc_begin_sharing_variables(void ***args,
+/// size_t n_args);
+OMPRTL_NVPTX__kmpc_begin_sharing_variables,
+/// Call to void __kmpc_end_sharing_variables();
+OMPRTL_NVPTX__kmpc_end_sharing_variables,
+/// Call to void __kmpc_get_shared_variables(void ***GlobalArgs)
+OMPRTL_NVPTX__kmpc_get_shared_variables,
+/// Call to uint16_t __kmpc_parallel_level(ident_t *loc, kmp_int32
+/// global_tid);
+OMPRTL_NVPTX__kmpc_parallel_level,
+/// Call to int8_t __kmpc_is

[PATCH] D59421: [OpenMP][Offloading] Allow to build the TRegion interface functions

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: ABataev, arpith-jacob, guraypp, gtbercea, hfinkel.
jdoerfert added projects: OpenMP, clang.

This patch adds the TRegion interface functions to the ones we can build
through the common OpenMP target offloading class.

This is a preperation patch for https://reviews.llvm.org/D59328


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59421

Files:
  clang/lib/CodeGen/CGOpenMPRuntimeTarget.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeTarget.h

Index: clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
===
--- clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
+++ clang/lib/CodeGen/CGOpenMPRuntimeTarget.h
@@ -122,6 +122,35 @@
 /// Call to void __kmpc_barrier_simple_spmd(ident_t *loc, kmp_int32
 /// global_tid);
 OMPRTL__kmpc_barrier_simple_spmd,
+
+/// Target Region (TREgion) Kernel interface
+///
+///{
+
+/// char __kmpc_target_region_kernel_init(ident_t *Ident,
+///   bool UseSPMDMode,
+///   bool UseStateMachine,
+///   bool RequiresOMPRuntime,
+///   bool RequiresDataSharing);
+OMPRTL__kmpc_target_region_kernel_init,
+
+/// void __kmpc_target_region_kernel_deinit(ident_t *Ident,
+/// bool UseSPMDMode,
+/// bool RequiredOMPRuntime);
+OMPRTL__kmpc_target_region_kernel_deinit,
+
+/// void __kmpc_target_region_kernel_parallel(ident_t *Ident,
+///   bool UseSPMDMode,
+///   bool RequiredOMPRuntime,
+///   ParallelWorkFnTy WorkFn,
+///   void *SharedVars,
+///   uint16_t SharedVarsBytes,
+///   void *PrivateVars,
+///   uint16_t PrivateVarsBytes,
+///   bool SharedPointers);
+OMPRTL__kmpc_target_region_kernel_parallel,
+
+///}
   };
 
   /// Returns the OpenMP runtime function identified by \p ID.
Index: clang/lib/CodeGen/CGOpenMPRuntimeTarget.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntimeTarget.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntimeTarget.cpp
@@ -52,6 +52,7 @@
 llvm::FunctionCallee CGOpenMPRuntimeTarget::createTargetRuntimeFunction(
 OpenMPRTLTargetFunctions ID) {
   llvm::FunctionCallee RTLFn = nullptr;
+  auto *I1Ty = llvm::IntegerType::getInt1Ty(CGM.getLLVMContext());
   switch (ID) {
   case OMPRTL_NVPTX__kmpc_kernel_init: {
 // Build void __kmpc_kernel_init(kmp_int32 thread_limit, int16_t
@@ -343,7 +344,96 @@
 ->addFnAttr(llvm::Attribute::Convergent);
 break;
   }
+  case OMPRTL__kmpc_target_region_kernel_init: {
+// char __kmpc_target_region_kernel_init(ident_t *Ident,
+//   bool UseSPMDMode,
+//   bool UseStateMachine,
+//   bool RequiresOMPRuntime,
+//   bool RequiresDataSharing);
+llvm::Type *TypeParams[] = {getIdentTyPointerTy(), I1Ty, I1Ty, I1Ty, I1Ty};
+auto *FnTy =
+llvm::FunctionType::get(CGM.Int8Ty, TypeParams, /* isVarArg */ false);
+RTLFn =
+CGM.CreateRuntimeFunction(FnTy, "__kmpc_target_region_kernel_init");
+
+llvm::Function *RTFn = cast(RTLFn.getCallee());
+RTFn->addParamAttr(0, llvm::Attribute::NoCapture);
+break;
+  }
+  case OMPRTL__kmpc_target_region_kernel_deinit: {
+// void __kmpc_target_region_kernel_deinit(ident_t *Ident,
+// bool UseSPMDMode,
+// bool RequiredOMPRuntime);
+llvm::Type *TypeParams[] = {getIdentTyPointerTy(), I1Ty, I1Ty};
+auto *FnTy =
+llvm::FunctionType::get(CGM.VoidTy, TypeParams, /* isVarArg */ false);
+RTLFn =
+CGM.CreateRuntimeFunction(FnTy, "__kmpc_target_region_kernel_deinit");
+
+llvm::Function *RTFn = cast(RTLFn.getCallee());
+RTFn->addParamAttr(0, llvm::Attribute::NoCapture);
+break;
+  }
+  case OMPRTL__kmpc_target_region_kernel_parallel: {
+// typedef void (*ParallelWorkFnTy)(void *, void *);
+auto *ParWorkFnTy =
+llvm::FunctionType::get(CGM.VoidTy, {CGM.VoidPtrTy, CGM.VoidPtrTy},
+/* isVarArg */ false);
+
+// void __kmpc_target_region_kernel_parallel(ident_t *Ident,
+//   bool UseSPMDMode,
+//   bool RequiredOMPRuntime,
+//  

[PATCH] D59328: [OpenMP][Offloading][2/3] Codegen for target regions (TRegions)

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 190840.
jdoerfert added a comment.

Remove code extracted into separate commits, see D59418 
, D59420 , 
and D59421 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59328

Files:
  clang/lib/CodeGen/CGOpenMPRuntimeTRegion.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeTRegion.h
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/OpenMP/target_tregion_no_SPMD_mode.c

Index: clang/test/OpenMP/target_tregion_no_SPMD_mode.c
===
--- /dev/null
+++ clang/test/OpenMP/target_tregion_no_SPMD_mode.c
@@ -0,0 +1,72 @@
+// RUN: %clang_cc1 -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
+// RUN: %clang_cc1 -fopenmp -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -mllvm -openmp-tregion-runtime -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s
+
+// CHECK: loop_in_loop_in_tregion
+// CHECK:  %0 = call i8 @__kmpc_target_region_kernel_init(i1 false, i1 true, i1 true, i1 true)
+// CHECK:  call void @__kmpc_target_region_kernel_deinit(i1 false, i1 true)
+void loop_in_loop_in_tregion(int *A, int *B) {
+#pragma omp target
+  for (int i = 0; i < 512; i++) {
+for (int j = 0; j < 1024; j++)
+  A[j] += B[i + j];
+  }
+}
+
+// CHECK: parallel_loops_and_accesses_in_tregion
+// CHECK:  %0 = call i8 @__kmpc_target_region_kernel_init(i1 false, i1 true, i1 true, i1 true)
+// CHECK:  call void @__kmpc_target_region_kernel_parallel(i1 false, i1 true, void (i8*, i8*)* @.omp_TRegion._wrapper, i8* undef, i16 0, i8* %2, i16 16, i1 false)
+// CHECK:  call void @__kmpc_target_region_kernel_parallel(i1 false, i1 true, void (i8*, i8*)* @.omp_TRegion.1_wrapper, i8* undef, i16 0, i8* %5, i16 16, i1 false)
+// CHECK:  call void @__kmpc_target_region_kernel_parallel(i1 false, i1 true, void (i8*, i8*)* @.omp_TRegion.2_wrapper, i8* undef, i16 0, i8* %8, i16 16, i1 false)
+// CHECK:  call void @__kmpc_target_region_kernel_deinit(i1 false, i1 true)
+void parallel_loops_and_accesses_in_tregion(int *A, int *B) {
+#pragma omp target
+  {
+#pragma omp parallel for
+for (int j = 0; j < 1024; j++)
+  A[j] += B[0 + j];
+#pragma omp parallel for
+for (int j = 0; j < 1024; j++)
+  A[j] += B[1 + j];
+#pragma omp parallel for
+for (int j = 0; j < 1024; j++)
+  A[j] += B[2 + j];
+
+// This needs a guard in SPMD mode
+A[0] = B[0];
+  }
+}
+
+void extern_func();
+static void parallel_loop(int *A, int *B, int i) {
+#pragma omp parallel for
+  for (int j = 0; j < 1024; j++)
+A[j] += B[i + j];
+}
+
+// CHECK: parallel_loop_in_function_in_loop_with_global_acc_in_tregion
+// CHECK:  %1 = call i8 @__kmpc_target_region_kernel_init(i1 false, i1 true, i1 true, i1 true)
+// CHECK:  call void @__kmpc_target_region_kernel_deinit(i1 false, i1 true)
+int Global[512];
+void parallel_loop_in_function_in_loop_with_global_acc_in_tregion(int *A, int *B) {
+#pragma omp target
+  for (int i = 0; i < 512; i++) {
+parallel_loop(A, B, i);
+Global[i]++;
+  }
+}
+
+// CHECK: parallel_loop
+// CHECK:  call void @__kmpc_target_region_kernel_parallel(i1 false, i1 true, void (i8*, i8*)* @.omp_TRegion.3_wrapper, i8* undef, i16 0, i8* %0, i16 24, i1 false)
+
+// CHECK: parallel_loops_in_functions_and_extern_func_in_tregion
+// CHECK:  %0 = call i8 @__kmpc_target_region_kernel_init(i1 false, i1 true, i1 true, i1 true)
+// CHECK:  call void @__kmpc_target_region_kernel_deinit(i1 false, i1 true)
+void parallel_loops_in_functions_and_extern_func_in_tregion(int *A, int *B) {
+#pragma omp target
+  {
+parallel_loop(A, B, 1);
+parallel_loop(A, B, 2);
+extern_func();
+parallel_loop(A, B, 3);
+  }
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -20,6 +20,7 @@
 #include "CGOpenCLRuntime.h"
 #include "CGOpenMPRuntime.h"
 #include "CGOpenMPRuntimeNVPTX.h"
+#include "CGOpenMPRuntimeTRegion.h"
 #include "CodeGenFunction.h"
 #include "CodeGenPGO.h"
 #include "ConstantEmitter.h"
@@ -67,6 +68,11 @@
 llvm::cl::desc("Emit limited coverage mapping information (experimental)"),
 llvm::cl::init(false));
 
+static llvm::cl::opt UseGenericTRegionInterface(
+"openmp-tregion-runtime", llvm::cl::ZeroOrMore, llvm::cl::Hidden,
+llvm::cl::desc("Use the generic target region OpenMP runtime interface"),
+llvm::cl::init(false));
+
 static const char AnnotationSection[] = "llvm.metadata";
 
 static CGCXXABI *createCXXABI(CodeGenModule &CGM) {
@@ -206,7 +212,10 @@
   case llvm::Triple::nvptx64:
 assert(getLangOpts().OpenMPIsDevice &&
"OpenMP NVPTX is only prepared to deal with devi

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

I realize this isn't the correct solution, but would any would-be reviewers 
like to comment on the problem? Whether it's here or on the Bugzilla report 
https://bugs.llvm.org/show_bug.cgi?id=39287, as a newcomer to Clang modules I 
could use some help understanding whether this sort of behavior is expected, or 
if there are known workarounds. Any and all comments appreciated!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Gor Nishanov via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D59076



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

[PATCH] D57855: [analyzer] Reimplement checker options

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus planned changes to this revision.
Szelethus marked 3 inline comments as done.
Szelethus added a comment.

Alright, I'll de-clutter this patch a bit.


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

https://reviews.llvm.org/D57855



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

r356277 - [HIP-Clang] propagate -mllvm options to opt and llc

[Aaron Enye Shi via cfe-commits]

Author: aaronenyeshi
Date: Fri Mar 15 10:31:51 2019
New Revision: 356277

URL: http://llvm.org/viewvc/llvm-project?rev=356277&view=rev
Log:
[HIP-Clang] propagate -mllvm options to opt and llc

Change the HIP Toolchain to pass the OPT_mllvm options into OPT and LLC stages. 
Added a lit test to verify the command args.

Reviewers: yaxunl

Differential Revision: https://reviews.llvm.org/D59316



Added:
cfe/trunk/test/Driver/hip-toolchain-mllvm.hip
Modified:
cfe/trunk/lib/Driver/ToolChains/HIP.cpp

Modified: cfe/trunk/lib/Driver/ToolChains/HIP.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/HIP.cpp?rev=356277&r1=356276&r2=356277&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/HIP.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/HIP.cpp Fri Mar 15 10:31:51 2019
@@ -140,6 +140,11 @@ const char *AMDGCN::Linker::constructOpt
   }
   OptArgs.push_back("-mtriple=amdgcn-amd-amdhsa");
   OptArgs.push_back(Args.MakeArgString("-mcpu=" + SubArchName));
+
+  for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+OptArgs.push_back(A->getValue(0));
+  }
+
   OptArgs.push_back("-o");
   std::string TmpFileName = C.getDriver().GetTemporaryPath(
   OutputFilePrefix.str() + "-optimized", "bc");
@@ -177,6 +182,10 @@ const char *AMDGCN::Linker::constructLlc
   if(!Features.empty())
 LlcArgs.push_back(Args.MakeArgString(MAttrString));
 
+  for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+LlcArgs.push_back(A->getValue(0));
+  }
+
   // Add output filename
   LlcArgs.push_back("-o");
   std::string LlcOutputFileName =

Added: cfe/trunk/test/Driver/hip-toolchain-mllvm.hip
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/hip-toolchain-mllvm.hip?rev=356277&view=auto
==
--- cfe/trunk/test/Driver/hip-toolchain-mllvm.hip (added)
+++ cfe/trunk/test/Driver/hip-toolchain-mllvm.hip Fri Mar 15 10:31:51 2019
@@ -0,0 +1,36 @@
+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// RUN: %clang -### -target x86_64-linux-gnu \
+// RUN:   -x hip --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 \
+// RUN:   -mllvm -amdgpu-function-calls=0 \
+// RUN:   %s 2>&1 | FileCheck %s
+
+// CHECK: [[CLANG:".*clang.*"]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
+// CHECK-SAME: {{.*}} "-target-cpu" "gfx803"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-function-calls=0" {{.*}}
+
+// CHECK: [[OPT:".*opt"]] {{".*-gfx803-linked.*bc"}} 
"-mtriple=amdgcn-amd-amdhsa"
+// CHECK-SAME: "-mcpu=gfx803" "-amdgpu-function-calls=0"
+// CHECK-SAME: "-o" [[OPT_803_BC:".*-gfx803-optimized.*bc"]]
+
+// CHECK: [[LLC: ".*llc"]] [[OPT_803_BC]]
+// CHECK-SAME: "-mtriple=amdgcn-amd-amdhsa" "-filetype=obj"
+// CHECK-SAME: {{.*}} "-mcpu=gfx803"
+// CHECK-SAME: "-amdgpu-function-calls=0" "-o" {{".*-gfx803-.*o"}}
+
+// CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
+// CHECK-SAME: {{.*}} "-target-cpu" "gfx900"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-function-calls=0" {{.*}}
+
+// CHECK: [[OPT]] {{".*-gfx900-linked.*bc"}} "-mtriple=amdgcn-amd-amdhsa"
+// CHECK-SAME: "-mcpu=gfx900" "-amdgpu-function-calls=0"
+// CHECK-SAME: "-o" [[OPT_900_BC:".*-gfx900-optimized.*bc"]]
+
+// CHECK: [[LLC]] [[OPT_900_BC]]
+// CHECK-SAME: "-mtriple=amdgcn-amd-amdhsa" "-filetype=obj"
+// CHECK-SAME: {{.*}} "-mcpu=gfx900"
+// CHECK-SAME: "-amdgpu-function-calls=0" "-o" {{".*-gfx900-.*o"}}


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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I'll look more closely into the details, but just a high-level question now: 
why would we want to make this optional and not simply surface these extra 
diagnostics?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356277: [HIP-Clang] propagate -mllvm options to opt and llc 
(authored by aaronenyeshi, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59316?vs=190474&id=190850#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59316

Files:
  lib/Driver/ToolChains/HIP.cpp
  test/Driver/hip-toolchain-mllvm.hip


Index: test/Driver/hip-toolchain-mllvm.hip
===
--- test/Driver/hip-toolchain-mllvm.hip
+++ test/Driver/hip-toolchain-mllvm.hip
@@ -0,0 +1,36 @@
+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// RUN: %clang -### -target x86_64-linux-gnu \
+// RUN:   -x hip --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 \
+// RUN:   -mllvm -amdgpu-function-calls=0 \
+// RUN:   %s 2>&1 | FileCheck %s
+
+// CHECK: [[CLANG:".*clang.*"]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
+// CHECK-SAME: {{.*}} "-target-cpu" "gfx803"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-function-calls=0" {{.*}}
+
+// CHECK: [[OPT:".*opt"]] {{".*-gfx803-linked.*bc"}} 
"-mtriple=amdgcn-amd-amdhsa"
+// CHECK-SAME: "-mcpu=gfx803" "-amdgpu-function-calls=0"
+// CHECK-SAME: "-o" [[OPT_803_BC:".*-gfx803-optimized.*bc"]]
+
+// CHECK: [[LLC: ".*llc"]] [[OPT_803_BC]]
+// CHECK-SAME: "-mtriple=amdgcn-amd-amdhsa" "-filetype=obj"
+// CHECK-SAME: {{.*}} "-mcpu=gfx803"
+// CHECK-SAME: "-amdgpu-function-calls=0" "-o" {{".*-gfx803-.*o"}}
+
+// CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
+// CHECK-SAME: {{.*}} "-target-cpu" "gfx900"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-function-calls=0" {{.*}}
+
+// CHECK: [[OPT]] {{".*-gfx900-linked.*bc"}} "-mtriple=amdgcn-amd-amdhsa"
+// CHECK-SAME: "-mcpu=gfx900" "-amdgpu-function-calls=0"
+// CHECK-SAME: "-o" [[OPT_900_BC:".*-gfx900-optimized.*bc"]]
+
+// CHECK: [[LLC]] [[OPT_900_BC]]
+// CHECK-SAME: "-mtriple=amdgcn-amd-amdhsa" "-filetype=obj"
+// CHECK-SAME: {{.*}} "-mcpu=gfx900"
+// CHECK-SAME: "-amdgpu-function-calls=0" "-o" {{".*-gfx900-.*o"}}
Index: lib/Driver/ToolChains/HIP.cpp
===
--- lib/Driver/ToolChains/HIP.cpp
+++ lib/Driver/ToolChains/HIP.cpp
@@ -140,6 +140,11 @@
   }
   OptArgs.push_back("-mtriple=amdgcn-amd-amdhsa");
   OptArgs.push_back(Args.MakeArgString("-mcpu=" + SubArchName));
+
+  for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+OptArgs.push_back(A->getValue(0));
+  }
+
   OptArgs.push_back("-o");
   std::string TmpFileName = C.getDriver().GetTemporaryPath(
   OutputFilePrefix.str() + "-optimized", "bc");
@@ -177,6 +182,10 @@
   if(!Features.empty())
 LlcArgs.push_back(Args.MakeArgString(MAttrString));
 
+  for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+LlcArgs.push_back(A->getValue(0));
+  }
+
   // Add output filename
   LlcArgs.push_back("-o");
   std::string LlcOutputFileName =


Index: test/Driver/hip-toolchain-mllvm.hip
===
--- test/Driver/hip-toolchain-mllvm.hip
+++ test/Driver/hip-toolchain-mllvm.hip
@@ -0,0 +1,36 @@
+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// RUN: %clang -### -target x86_64-linux-gnu \
+// RUN:   -x hip --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 \
+// RUN:   -mllvm -amdgpu-function-calls=0 \
+// RUN:   %s 2>&1 | FileCheck %s
+
+// CHECK: [[CLANG:".*clang.*"]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
+// CHECK-SAME: {{.*}} "-target-cpu" "gfx803"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-function-calls=0" {{.*}}
+
+// CHECK: [[OPT:".*opt"]] {{".*-gfx803-linked.*bc"}} "-mtriple=amdgcn-amd-amdhsa"
+// CHECK-SAME: "-mcpu=gfx803" "-amdgpu-function-calls=0"
+// CHECK-SAME: "-o" [[OPT_803_BC:".*-gfx803-optimized.*bc"]]
+
+// CHECK: [[LLC: ".*llc"]] [[OPT_803_BC]]
+// CHECK-SAME: "-mtriple=amdgcn-amd-amdhsa" "-filetype=obj"
+// CHECK-SAME: {{.*}} "-mcpu=gfx803"
+// CHECK-SAME: "-amdgpu-function-calls=0" "-o" {{".*-gfx803-.*o"}}
+
+// CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu" "-emit-llvm-bc"
+// CHECK-SAME: {{.*}} "-target-cpu" "gfx900"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-function-calls=0" {{.*}}
+
+// CHECK: [[OPT]] {{".*-gfx900-linked.*bc"}} "-mtriple=amdgcn-amd-amdhsa"
+// CHECK-SAME: "-mcpu=gfx900" "-amdgpu-function-calls=0"
+// CHECK-SAME: "-o" [[OPT_900_BC:".*-gfx900-optimized.*bc"]]
+
+// CHECK: [[LLC]] [[OPT_900_BC]]
+// CHECK-SAME: "-mtriple=amdgcn-amd-amdhsa" "-filetype=obj"
+// CHECK-SAME: {{.*}} "-mcpu=gfx900"
+// CHECK-SAME: "-amdgpu-function-calls=0" "-o" {{".*-gfx900-.*

[PATCH] D45978: dllexport const variables must have external linkage.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


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

https://reviews.llvm.org/D45978



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool 
UseSPMDMode,
+   bool RequiresOMPRuntime,

If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` parameters are 
not needed anymore. They are passed in `ident_t` structure.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omp_data.cu:70
+
+__device__ __shared__ target_region_shared_buffer _target_region_shared_memory;
+

What is this buffer used for? Transferring pointers to the shread variables to 
the parallel regions? If so, it must be handled by the compiler. There are 
several reasons to do this:
1. You're using malloc/free functions for large buffers. The fact is that the 
size of this buffer is known at the compile time and compiler can generate the 
fixed size buffer in the global memory if required. We already have similar 
implementation for target regions, globalized variables etc. You can take a 
look and adapt it for your purpose.
2. Malloc/free are not very fast on the GPU, so it will get an additional 
performance with the preallocated buffers.
3. Another one problem with malloc/free is that they are using preallocated 
memory and the size of this memory is limited by 8Mb (if I do recall 
correctly). This memory is required for the correct support of the local 
variables globalization and we alredy ran into the situation when malloc could 
not allocate enough memory for it with some previous implementations.
4. You can reused the shared memory buffers already generated by the compiler 
and save  shared memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@ilya-biryukov could you please give details about the quality metric you are 
using and some description of posted measurements?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59300



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

Re: r352930 - [WebAssembly] Add an import_field function attribute

[Aaron Ballman via cfe-commits]

On Fri, Mar 8, 2019 at 5:06 PM Aaron Ballman  wrote:
>
> On Wed, Feb 6, 2019 at 12:43 AM Dan Gohman  wrote:
> >
> >
> >
> > On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman  
> > wrote:
> >>
> >> On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman  wrote:
> >> >
> >> >
> >> >  Indeed, and the existing import_module attribute needs these tests as 
> >> > well. I'll write some and add them in a follow-up patch.
> >> >
> >> > Thanks for the review!
> >>
> >> Any time! I haven't seen that follow-up patch yet though; did it fall
> >> off your radar?
> >
> >
> > Yes, it's still on my radar.
>
> It's been a month and it still seems like this hasn't been taken care
> of. The usual expectation is to handle post-commit feedback
> "immediately". Can you please address these concerns?

Ping.

~Aaron

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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks very polished, thanks!
Will have to sink the change in a bit, will come back with more comments on 
Monday.
In the meantime, a few initial comments and suggestions.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:

Intuitively, it feels that any filtering should be possible at the level of the 
AST matchers. Is that not the case?
Could you provide some motivating examples where AST matchers cannot be used to 
nail down the matching nodes and we need `MatchFilter`? 

Please note I have limited experience with AST matchers, so there might be some 
obvious things that I'm missing.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

Maybe consider separating the fluent API to build the rewrite rule from the 
rewrite rule itself?

Not opposed to having the fluent builder API, this does look nice and seems to 
nicely align with the matcher APIs.
However, it is somewhat hard to figure out what can `RewriteRule` do **after** 
it was built when looking at the code right now.
```
class RewriteRule {
public:
  RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator 
Explanation);

  DynTypedMatcher matcher() const;
  Expected replacement() const;
  Expected explanation() const;
};

struct RewriteRuleBuilder { // Having a better name than 'Builder' would be 
nice.
  RewriteRule finish() &&; // produce the final RewriteRule.

  template 
  RewriteRuleBuilder &change(const TypedNodeId &Target,
  NodePart Part = NodePart::Node) &;
  RewriteRuleBuilder &replaceWith(TextGenerator Replacement) &;
  RewriteRuleBuilder &because(TextGenerator Explanation) &;
private:
  RewriteRule RuleInProgress;
};
RewriteRuleBuilder makeRewriteRule();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376



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

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from some minor nits.




Comment at: clang/lib/Sema/SemaChecking.cpp:338
+  case Builtin::BI__builtin___vsnprintf_chk: {
+DiagID = diag::warn_memcpy_chk_overflow;
+IsChkVariant = true;

I feel like this diagnostic name should be updated -- `snprintf()` and 
`memcpy()` are pretty distinct things. Maybe `warn_builtin_chk_overflow`?



Comment at: clang/lib/Sema/SemaChecking.cpp:400
+int BOSType = 0;
+if (auto *POS =
+FD->getParamDecl(ObjectIndex)->getAttr())

`const auto *`


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

https://reviews.llvm.org/D58797



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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: ABataev, arpith-jacob, guraypp, gtbercea, hfinkel.
jdoerfert added a project: OpenMP.

This commit implements the existing void** buffer used to share
arguments between threads in a team with a byte-wise buffer. For now,
the void** buffer is kept for compatibility.

The byte-wise buffer, if used directly, allows to save memory when small
arguments are shared between team threads. It does also allow to track
an additional offset that differentiates two distinct back-to-back
memory regions, e.g., for shared (copy in & out) and firstprivate (copy
in only) variables.

This is a preparation patch for https://reviews.llvm.org/D59319


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59424

Files:
  openmp/libomptarget/deviceRTLs/nvptx/src/omp_data.cu
  openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
  openmp/libomptarget/deviceRTLs/nvptx/src/option.h

Index: openmp/libomptarget/deviceRTLs/nvptx/src/option.h
===
--- openmp/libomptarget/deviceRTLs/nvptx/src/option.h
+++ openmp/libomptarget/deviceRTLs/nvptx/src/option.h
@@ -27,9 +27,9 @@
 // region to synchronize with each other.
 #define L1_BARRIER (1)
 
-// Maximum number of preallocated arguments to an outlined parallel/simd function.
-// Anything more requires dynamic memory allocation.
-#define MAX_SHARED_ARGS 20
+// Maximum number of preallocated bytes that can be passed to an outlined
+// parallel/simd function before dynamic memory allocation is required.
+#define PRE_SHARED_BYTES 128
 
 // Maximum number of omp state objects per SM allocated statically in global
 // memory.
Index: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
===
--- openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
+++ openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
@@ -63,42 +63,84 @@
 #define __SYNCTHREADS_N(n) asm volatile("bar.sync %0;" : : "r"(n) : "memory");
 #define __SYNCTHREADS() __SYNCTHREADS_N(0)
 
+/// Helper structure to manage the memory shared by the threads in a team.
+///
+/// This buffer can manage two adjacent byte-wise objects by tracking the
+/// beginning of the second, as an offset, in addition to the beginning of the
+/// first, as a pointer.
+///
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+
+  INLINE void init() {
+_ptr = &_data[0];
+_size = PRE_SHARED_BYTES;
+_offset = 0;
+  }
+
+  /// Release any dynamic allocated memory.
+  INLINE void release() {
+if (_size == PRE_SHARED_BYTES)
+  return;
+SafeFree(_ptr, (char *)"free shared dynamic buffer");
+init();
+  }
+
+  INLINE void set(void *ptr, size_t offset) {
+release();
+_ptr = (char *)ptr;
+_offset = offset;
+  }
+
+  INLINE void resize(size_t size, size_t offset) {
+_offset = offset;
+
+if (size <= _size)
+  return;
+
+if (_size != PRE_SHARED_BYTES)
+  SafeFree(_ptr, (char *)"free shared dynamic buffer");
+
+_size = size;
+_ptr = (char *)SafeMalloc(_size, (char *)"new shared buffer");
+  }
+
+  // Called by all threads.
+  INLINE void *begin() const { return _ptr; };
+  INLINE size_t size() const { return _size; };
+  INLINE size_t get_offset() const { return _offset; };
+
+private:
+  // Pre-allocated space that holds PRE_SHARED_BYTES many bytes.
+  char _data[PRE_SHARED_BYTES];
+
+  // Pointer to the currently used buffer.
+  char *_ptr;
+
+  // Size of the currently used buffer.
+  uint32_t _size;
+
+  // Offset into the currently used buffer.
+  uint32_t _offset;
+};
+
+extern __device__ __shared__ shared_bytes_buffer _shared_bytes_buffer_memory;
+
 // arguments needed for L0 parallelism only.
+//
+// NOTE: Deprecated, use shared_byte_buffer instead.
 class omptarget_nvptx_SharedArgs {
 public:
   // All these methods must be called by the master thread only.
-  INLINE void Init() {
-args  = buffer;
-nArgs = MAX_SHARED_ARGS;
-  }
-  INLINE void DeInit() {
-// Free any memory allocated for outlined parallel function with a large
-// number of arguments.
-if (nArgs > MAX_SHARED_ARGS) {
-  SafeFree(args, (char *)"new extended args");
-  Init();
-}
-  }
+  INLINE void Init() { _shared_bytes_buffer_memory.init(); }
+  INLINE void DeInit() { _shared_bytes_buffer_memory.release(); }
   INLINE void EnsureSize(size_t size) {
-if (size > nArgs) {
-  if (nArgs > MAX_SHARED_ARGS) {
-SafeFree(args, (char *)"new extended args");
-  }
-  args = (void **) SafeMalloc(size * sizeof(void *),
-  (char *)"new extended args");
-  nArgs = size;
-}
+_shared_bytes_buffer_memory.resize(size * sizeof(void *), 0);
   }
   // Called by all threads.
-  INLINE void **GetArgs() const { return args; };
-private:
-  // buffer of pre-allocated arguments.
-  void *buffer[MAX_SH

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 190861.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Rebase onto D59424  and fix errors caused by 
the wrong use of ident_t


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319

Files:
  openmp/libomptarget/deviceRTLs/common/target_region.h
  openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
  openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu

Index: openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu
===
--- /dev/null
+++ openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu
@@ -0,0 +1,195 @@
+//===-- target_region.cu  CUDA impl. of the target region interface -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains the implementation of the common target region interface.
+//
+//===--===//
+
+// Include the native definitions first as certain defines might be needed in
+// the common interface definition below.
+#include "omptarget-nvptx.h"
+#include "interface.h"
+
+#include "../../common/target_region.h"
+
+EXTERN void *__kmpc_target_region_kernel_get_shared_memory() {
+  return _shared_bytes_buffer_memory.begin();
+}
+EXTERN void *__kmpc_target_region_kernel_get_private_memory() {
+  return ((char *)_shared_bytes_buffer_memory.begin()) +
+ _shared_bytes_buffer_memory.get_offset();
+}
+
+/// Simple generic state machine for worker threads.
+INLINE static void
+__kmpc_target_region_state_machine(ident_t *Ident,
+   bool IsOMPRuntimeInitialized) {
+
+  do {
+void *WorkFn = 0;
+
+// Wait for the signal that we have a new work function.
+__kmpc_barrier_simple_spmd(Ident, 0);
+
+// Retrieve the work function from the runtime.
+bool IsActive = __kmpc_kernel_parallel(&WorkFn, IsOMPRuntimeInitialized);
+
+// If there is nothing more to do, break out of the state machine by
+// returning to the caller.
+if (!WorkFn)
+  return;
+
+if (IsActive) {
+  void *SharedVars = __kmpc_target_region_kernel_get_shared_memory();
+  void *PrivateVars = __kmpc_target_region_kernel_get_private_memory();
+
+  ((ParallelWorkFnTy)WorkFn)(SharedVars, PrivateVars);
+
+  __kmpc_kernel_end_parallel();
+}
+
+__kmpc_barrier_simple_spmd(Ident, 0);
+
+  } while (true);
+}
+
+/// Filter threads into masters and workers. If \p UseStateMachine is true,
+/// required workers will enter a state machine through and be trapped there.
+/// Master and surplus worker threads will return from this function immediately
+/// while required workers will only return once there is no more work. The
+/// return value indicates if the thread is a master (1), a surplus worker (0),
+/// or a finished required worker released from the state machine (-1).
+INLINE static int8_t
+__kmpc_target_region_thread_filter(ident_t *Ident, unsigned ThreadLimit,
+   bool UseStateMachine,
+   bool IsOMPRuntimeInitialized) {
+
+  unsigned TId = GetThreadIdInBlock();
+  bool IsWorker = TId < ThreadLimit;
+
+  if (IsWorker) {
+if (UseStateMachine)
+  __kmpc_target_region_state_machine(Ident, IsOMPRuntimeInitialized);
+return -1;
+  }
+
+  return TId == GetMasterThreadID();
+}
+
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool UseSPMDMode,
+   bool UseStateMachine,
+   bool RequiresOMPRuntime,
+   bool RequiresDataSharing) {
+  unsigned NumThreads = GetNumberOfThreadsInBlock();
+
+  // Handle the SPMD case first.
+  if (UseSPMDMode) {
+
+__kmpc_spmd_kernel_init(NumThreads, RequiresOMPRuntime,
+RequiresDataSharing);
+
+if (RequiresDataSharing)
+  __kmpc_data_sharing_init_stack_spmd();
+
+return 1;
+  }
+
+  // Reserve one WARP in non-SPMD mode for the masters.
+  unsigned ThreadLimit = NumThreads - WARPSIZE;
+  int8_t FilterVal = __kmpc_target_region_thread_filter(
+  Ident, ThreadLimit, UseStateMachine, RequiresOMPRuntime);
+
+  // If the filter returns 1 the executing thread is a team master which will
+  // initialize the kernel in the following.
+  if (FilterVal == 1) {
+__kmpc_kernel_init(ThreadLimit, RequiresOMPRuntime);
+__kmpc_data_sharing_init_stack();
+_shared_bytes_buffer_memory.init();
+  }
+
+  return FilterVal;
+}
+
+EXTERN void __kmpc_target_region_kernel_deinit(ident_t *Ident

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

> What is this buffer used for? [...]

I'll copy your comment and respond in this review D59424 
.




Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool 
UseSPMDMode,
+   bool RequiresOMPRuntime,

ABataev wrote:
> If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` parameters 
> are not needed anymore. They are passed in `ident_t` structure.
> If you're using ident_t UseSPMDMode and RequiresOMPRuntime parameters are not 
> needed anymore. They are passed in ident_t structure.

They are not in the TRegion interface, at least not by the TRegion code 
generation. If required, we can add that or require the 
`__kmpc_target_region_kernel_init` implementation to store the values in the 
`ident_t`. Regardless, we do not want to hide the variables in the `ident_t` 
because that would result in worse analysis results and cause optimizations to 
be harder. The main point of all these changes is, after all, to make 
optimizations easy. Given that we expect these functions to be inlined, there 
is also no harm done wrt. runtime costs.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59425: Explicitly Craft a Path to Compiler-RT Builtins on Bare Metal Targets

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: georgemorgan, mehdi_amini, kristina.
Herald added subscribers: cfe-commits, jdoerfert, kristof.beyls, javed.absar, 
dberris.
Herald added a project: clang.

D33259  hard-coded a linker flag relative to a 
linker library search path that should have allowed it to find 
`libclang_rt.builtins-$ARCH.a` under the resource directory automatically.  
Unfortunately, both the flag and the search path are incorrect, which leads to 
workarounds like double-suffixes 

 in linker invocations.

The bare metal toolchain now uses a more standard hook to get the right 
arch-specific compiler-rt, and constructs a full path relative to the resource 
directory for the linker input.


Repository:
  rC Clang

https://reviews.llvm.org/D59425

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/BareMetal.h
  clang/test/Driver/baremetal.cpp

Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -13,7 +13,8 @@
 // CHECK-V6M-C-NEXT: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-C-SAME: "-L[[RESOURCE_DIR:[^"]+]]{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-C-SAME: "-T" "semihosted.lds" "-Lsome{{[/\\]+}}directory{{[/\\]+}}user{{[/\\]+}}asked{{[/\\]+}}for"
-// CHECK-V6M-C-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-C-SAME: "-lc" "-lm"
+// CHECK-V6M-C-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-C-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -35,7 +36,8 @@
 // CHECK-V6M-DEFAULTCXX: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-DEFAULTCXX-SAME: "-L{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-DEFAULTCXX-SAME: "-lc++" "-lc++abi" "-lunwind"
-// CHECK-V6M-DEFAULTCXX-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-DEFAULTCXX-SAME: "-lc" "-lm"
+// CHECK-V6M-DEFAULTCXX-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-DEFAULTCXX-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -48,7 +50,8 @@
 // CHECK-V6M-LIBCXX: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-LIBCXX-SAME: "-L{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-LIBCXX-SAME: "-lc++" "-lc++abi" "-lunwind"
-// CHECK-V6M-LIBCXX-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-LIBCXX-SAME: "-lc" "-lm"
+// CHECK-V6M-LIBCXX-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-LIBCXX-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -61,7 +64,8 @@
 // CHECK-V6M-LIBSTDCXX: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-LIBSTDCXX-SAME: "-L{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-LIBSTDCXX-SAME: "-lstdc++" "-lsupc++" "-lunwind"
-// CHECK-V6M-LIBSTDCXX-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-LIBSTDCXX-SAME: "-lc" "-lm"
+// CHECK-V6M-LIBSTDCXX-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-LIBSTDCXX-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
Index: clang/lib/Driver/ToolChains/BareMetal.h
===
--- clang/lib/Driver/ToolChains/BareMetal.h
+++ clang/lib/Driver/ToolChains/BareMetal.h
@@ -57,8 +57,10 @@
   llvm::opt::ArgStringList &CC1Args) const override;
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs) const override;
-  void AddLinkRuntimeLib(const llvm::opt::ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs) const;
+
+  std::string getCompilerRT(const llvm::opt::ArgList &Args,
+StringRef Component,
+FileType Type = ToolChain::FT_Static) const override;
 };
 
 } // namespace toolchains
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -154,10 +154,31 @@
   CmdArgs.push_back("-lunwind");
 }
 
-void BareMetal::AddLinkRuntimeLib(const ArgList &Args,
-

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D59377#1430002 , @dexonsmith wrote:

> ... since I noticed that FileManager ...


This kind of implies that we should move the comment from `FileManager` 
constructor implementation to the header thus making it an explicit part of the 
interface contract.

  // If the caller doesn't provide a virtual file system, just grab the real
  // file system.

Maybe https://reviews.llvm.org/D59388 would be the right place to do it.


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

https://reviews.llvm.org/D59377



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool 
UseSPMDMode,
+   bool RequiresOMPRuntime,

jdoerfert wrote:
> ABataev wrote:
> > If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` parameters 
> > are not needed anymore. They are passed in `ident_t` structure.
> > If you're using ident_t UseSPMDMode and RequiresOMPRuntime parameters are 
> > not needed anymore. They are passed in ident_t structure.
> 
> They are not in the TRegion interface, at least not by the TRegion code 
> generation. If required, we can add that or require the 
> `__kmpc_target_region_kernel_init` implementation to store the values in the 
> `ident_t`. Regardless, we do not want to hide the variables in the `ident_t` 
> because that would result in worse analysis results and cause optimizations 
> to be harder. The main point of all these changes is, after all, to make 
> optimizations easy. Given that we expect these functions to be inlined, there 
> is also no harm done wrt. runtime costs.
> 
> 
> 
> 
This is why we used them. Those `ident_t`s  are constant and it allows us to 
perform an additional optimization in the functions, that do not have 
`isSPMDMpde` and `RequiresFullRuntime`. Because of this parameter, we gained a 
significant performance boost. LLVM knows how to deal with the structures, 
don't worry about the optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59423: [CUDA][Windows] Partial fix for bug 38811 (Step 2 of 3)

[Evgeny Mankov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356291: [CUDA][Windows] Partial fix for bug 38811 (Step 2 of 
3) (authored by emankov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59423?vs=190859&id=190867#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59423

Files:
  lib/Headers/__clang_cuda_device_functions.h
  lib/Headers/__clang_cuda_libdevice_declares.h


Index: lib/Headers/__clang_cuda_libdevice_declares.h
===
--- lib/Headers/__clang_cuda_libdevice_declares.h
+++ lib/Headers/__clang_cuda_libdevice_declares.h
@@ -141,7 +141,7 @@
 __device__ float __nv_fast_log2f(float __a);
 __device__ float __nv_fast_logf(float __a);
 __device__ float __nv_fast_powf(float __a, float __b);
-__device__ void __nv_fast_sincosf(float __a, float *__sptr, float *__cptr);
+__device__ void __nv_fast_sincosf(float __a, float *__s, float *__c);
 __device__ float __nv_fast_sinf(float __a);
 __device__ float __nv_fast_tanf(float __a);
 __device__ double __nv_fdim(double __a, double __b);
Index: lib/Headers/__clang_cuda_device_functions.h
===
--- lib/Headers/__clang_cuda_device_functions.h
+++ lib/Headers/__clang_cuda_device_functions.h
@@ -520,8 +520,8 @@
 __DEVICE__ float __saturatef(float __a) { return __nv_saturatef(__a); }
 __DEVICE__ int __signbitd(double __a) { return __nv_signbitd(__a); }
 __DEVICE__ int __signbitf(float __a) { return __nv_signbitf(__a); }
-__DEVICE__ void __sincosf(float __a, float *__sptr, float *__cptr) {
-  return __nv_fast_sincosf(__a, __sptr, __cptr);
+__DEVICE__ void __sincosf(float __a, float *__s, float *__c) {
+  return __nv_fast_sincosf(__a, __s, __c);
 }
 __DEVICE__ float __sinf(float __a) { return __nv_fast_sinf(__a); }
 __DEVICE__ int __syncthreads_and(int __a) { return __nvvm_bar0_and(__a); }
@@ -1713,17 +1713,17 @@
   return scalbnf(__a, (int)__b);
 }
 __DEVICE__ double sin(double __a) { return __nv_sin(__a); }
-__DEVICE__ void sincos(double __a, double *__sptr, double *__cptr) {
-  return __nv_sincos(__a, __sptr, __cptr);
+__DEVICE__ void sincos(double __a, double *__s, double *__c) {
+  return __nv_sincos(__a, __s, __c);
 }
-__DEVICE__ void sincosf(float __a, float *__sptr, float *__cptr) {
-  return __FAST_OR_SLOW(__nv_fast_sincosf, __nv_sincosf)(__a, __sptr, __cptr);
+__DEVICE__ void sincosf(float __a, float *__s, float *__c) {
+  return __FAST_OR_SLOW(__nv_fast_sincosf, __nv_sincosf)(__a, __s, __c);
 }
-__DEVICE__ void sincospi(double __a, double *__sptr, double *__cptr) {
-  return __nv_sincospi(__a, __sptr, __cptr);
+__DEVICE__ void sincospi(double __a, double *__s, double *__c) {
+  return __nv_sincospi(__a, __s, __c);
 }
-__DEVICE__ void sincospif(float __a, float *__sptr, float *__cptr) {
-  return __nv_sincospif(__a, __sptr, __cptr);
+__DEVICE__ void sincospif(float __a, float *__s, float *__c) {
+  return __nv_sincospif(__a, __s, __c);
 }
 __DEVICE__ float sinf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_sinf, __nv_sinf)(__a);


Index: lib/Headers/__clang_cuda_libdevice_declares.h
===
--- lib/Headers/__clang_cuda_libdevice_declares.h
+++ lib/Headers/__clang_cuda_libdevice_declares.h
@@ -141,7 +141,7 @@
 __device__ float __nv_fast_log2f(float __a);
 __device__ float __nv_fast_logf(float __a);
 __device__ float __nv_fast_powf(float __a, float __b);
-__device__ void __nv_fast_sincosf(float __a, float *__sptr, float *__cptr);
+__device__ void __nv_fast_sincosf(float __a, float *__s, float *__c);
 __device__ float __nv_fast_sinf(float __a);
 __device__ float __nv_fast_tanf(float __a);
 __device__ double __nv_fdim(double __a, double __b);
Index: lib/Headers/__clang_cuda_device_functions.h
===
--- lib/Headers/__clang_cuda_device_functions.h
+++ lib/Headers/__clang_cuda_device_functions.h
@@ -520,8 +520,8 @@
 __DEVICE__ float __saturatef(float __a) { return __nv_saturatef(__a); }
 __DEVICE__ int __signbitd(double __a) { return __nv_signbitd(__a); }
 __DEVICE__ int __signbitf(float __a) { return __nv_signbitf(__a); }
-__DEVICE__ void __sincosf(float __a, float *__sptr, float *__cptr) {
-  return __nv_fast_sincosf(__a, __sptr, __cptr);
+__DEVICE__ void __sincosf(float __a, float *__s, float *__c) {
+  return __nv_fast_sincosf(__a, __s, __c);
 }
 __DEVICE__ float __sinf(float __a) { return __nv_fast_sinf(__a); }
 __DEVICE__ int __syncthreads_and(int __a) { return __nvvm_bar0_and(__a); }
@@ -1713,17 +1713,17 @@
   return scalbnf(__a, (int)__b);
 }
 __DEVICE__ double sin(double __a) { return __nv_sin(__a); }
-__DEVICE__ void sincos(double __a, double *__sptr, double *__cptr) {
-  return __nv_sincos(__a, __sptr, __cptr);
+__DEVICE__ void sincos(double __a, 

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 190866.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Fix the set/release use case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424

Files:
  openmp/libomptarget/deviceRTLs/nvptx/src/omp_data.cu
  openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
  openmp/libomptarget/deviceRTLs/nvptx/src/option.h

Index: openmp/libomptarget/deviceRTLs/nvptx/src/option.h
===
--- openmp/libomptarget/deviceRTLs/nvptx/src/option.h
+++ openmp/libomptarget/deviceRTLs/nvptx/src/option.h
@@ -27,9 +27,9 @@
 // region to synchronize with each other.
 #define L1_BARRIER (1)
 
-// Maximum number of preallocated arguments to an outlined parallel/simd function.
-// Anything more requires dynamic memory allocation.
-#define MAX_SHARED_ARGS 20
+// Maximum number of preallocated bytes that can be passed to an outlined
+// parallel/simd function before dynamic memory allocation is required.
+#define PRE_SHARED_BYTES 128
 
 // Maximum number of omp state objects per SM allocated statically in global
 // memory.
Index: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
===
--- openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
+++ openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h
@@ -63,42 +63,87 @@
 #define __SYNCTHREADS_N(n) asm volatile("bar.sync %0;" : : "r"(n) : "memory");
 #define __SYNCTHREADS() __SYNCTHREADS_N(0)
 
+/// Helper structure to manage the memory shared by the threads in a team.
+///
+/// This buffer can manage two adjacent byte-wise objects by tracking the
+/// beginning of the second, as an offset, in addition to the beginning of the
+/// first, as a pointer.
+///
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+
+  INLINE void init() {
+_ptr = &_data[0];
+_size = PRE_SHARED_BYTES;
+_offset = 0;
+  }
+
+  /// Release any dynamic allocated memory.
+  INLINE void release() {
+if (_size != PRE_SHARED_BYTES)
+  SafeFree(_ptr, (char *)"free shared dynamic buffer");
+// Always perform an init, it is cheap and required after a set call was
+// performed during the last use of the buffer.
+init();
+  }
+
+  INLINE void set(void *ptr, size_t offset) {
+// Note that release will set _size to PRE_SHARED_BYTES, thereby avoiding
+// the next release call from freeing the associated memory.
+release();
+_ptr = (char *)ptr;
+_offset = offset;
+  }
+
+  INLINE void resize(size_t size, size_t offset) {
+_offset = offset;
+
+if (size <= _size)
+  return;
+
+if (_size != PRE_SHARED_BYTES)
+  SafeFree(_ptr, (char *)"free shared dynamic buffer");
+
+_size = size;
+_ptr = (char *)SafeMalloc(_size, (char *)"new shared buffer");
+  }
+
+  // Called by all threads.
+  INLINE void *begin() const { return _ptr; };
+  INLINE size_t size() const { return _size; };
+  INLINE size_t get_offset() const { return _offset; };
+
+private:
+  // Pre-allocated space that holds PRE_SHARED_BYTES many bytes.
+  char _data[PRE_SHARED_BYTES];
+
+  // Pointer to the currently used buffer.
+  char *_ptr;
+
+  // Size of the currently used buffer.
+  uint32_t _size;
+
+  // Offset into the currently used buffer.
+  uint32_t _offset;
+};
+
+extern __device__ __shared__ shared_bytes_buffer _shared_bytes_buffer_memory;
+
 // arguments needed for L0 parallelism only.
+//
+// NOTE: Deprecated, use shared_byte_buffer instead.
 class omptarget_nvptx_SharedArgs {
 public:
   // All these methods must be called by the master thread only.
-  INLINE void Init() {
-args  = buffer;
-nArgs = MAX_SHARED_ARGS;
-  }
-  INLINE void DeInit() {
-// Free any memory allocated for outlined parallel function with a large
-// number of arguments.
-if (nArgs > MAX_SHARED_ARGS) {
-  SafeFree(args, (char *)"new extended args");
-  Init();
-}
-  }
+  INLINE void Init() { _shared_bytes_buffer_memory.init(); }
+  INLINE void DeInit() { _shared_bytes_buffer_memory.release(); }
   INLINE void EnsureSize(size_t size) {
-if (size > nArgs) {
-  if (nArgs > MAX_SHARED_ARGS) {
-SafeFree(args, (char *)"new extended args");
-  }
-  args = (void **) SafeMalloc(size * sizeof(void *),
-  (char *)"new extended args");
-  nArgs = size;
-}
+_shared_bytes_buffer_memory.resize(size * sizeof(void *), 0);
   }
   // Called by all threads.
-  INLINE void **GetArgs() const { return args; };
-private:
-  // buffer of pre-allocated arguments.
-  void *buffer[MAX_SHARED_ARGS];
-  // pointer to arguments buffer.
-  // starts off as a pointer to 'buffer' but can be dynamically allocated.
-  void **args;
-  // starts off as MAX_SHARED_ARGS but can increase in size.
-  uint32_t nArg

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as not done.
jdoerfert added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

> What is this buffer used for? Transferring pointers to the shread variables 
> to the parallel regions? If so, it must be handled by the compiler. There are 
> several reasons to do this:
> 1) You're using malloc/free functions for large buffers. The fact is that the 
> size of this buffer is known at the compile time and compiler can generate 
> the fixed size buffer in the global memory if required. We already have 
> similar implementation for target regions, globalized variables etc. You can 
> take a look and adapt it for your purpose.
> 2) Malloc/free are not very fast on the GPU, so it will get an additional 
> performance with the preallocated buffers.
> 3) Another one problem with malloc/free is that they are using preallocated 
> memory and the size of this memory is limited by 8Mb (if I do recall 
> correctly). This memory is required for the correct support of the local 
> variables globalization and we alredy ran into the situation when malloc 
> could not allocate enough memory for it with some previous implementations.
> 4) You can reused the shared memory buffers already generated by the compiler 
> and save shared memory.

[Quote by ABataev copied from 
https://reviews.llvm.org/D59319?id=190767#inline-525900 after the patch was 
split.]


This buffer is supposed to be used to communicate variables in shared and 
firstprivate clauses between threads in a team. In this patch it is simply used 
to implement the old `void**` buffer. How, when, if we use it is part of the 
interface implementation. For now, this buffer simply serves the users of the 
`omptarget_nvptx_globalArgs` global.

If you want to provide compiler allocated memory to avoid the buffer use, no 
problem,
the `__kmpc_target_region_kernel_parallel` function allows to do so, see the 
`SharedMemPointers` flag. I wouldn't want to put the logic to generate these 
buffers in the front-end though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424



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

r356291 - [CUDA][Windows] Partial fix for bug 38811 (Step 2 of 3)

[Evgeny Mankov via cfe-commits]

Author: emankov
Date: Fri Mar 15 12:04:46 2019
New Revision: 356291

URL: http://llvm.org/viewvc/llvm-project?rev=356291&view=rev
Log:
[CUDA][Windows] Partial fix for bug 38811 (Step 2 of 3)

Partial fix for the clang Bug 38811 "Clang fails to compile with CUDA-9.x on 
Windows".

[Synopsis]
__sptr is a new Microsoft specific modifier 
(https://docs.microsoft.com/en-us/cpp/cpp/sptr-uptr?view=vs-2017).

[Solution]
Replace all `__sptr` occurrences with `__s` (and all `__cptr` with `__c` as 
well) to eliminate the below clang compilation error on Windows.

In file included from 
C:\GIT\LLVM\trunk\llvm-64-release-vs2017-15.9.5\dist\lib\clang\9.0.0\include\__clang_cuda_runtime_wrapper.h:162:
C:\GIT\LLVM\trunk\llvm-64-release-vs2017-15.9.5\dist\lib\clang\9.0.0\include\__clang_cuda_device_functions.h:524:33:
 error: expected expression
  return __nv_fast_sincosf(__a, __sptr, __cptr);
^
Reviewed by: Artem Belevich

Differential Revision: http://reviews.llvm.org/D59423

Modified:
cfe/trunk/lib/Headers/__clang_cuda_device_functions.h
cfe/trunk/lib/Headers/__clang_cuda_libdevice_declares.h

Modified: cfe/trunk/lib/Headers/__clang_cuda_device_functions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/__clang_cuda_device_functions.h?rev=356291&r1=356290&r2=356291&view=diff
==
--- cfe/trunk/lib/Headers/__clang_cuda_device_functions.h (original)
+++ cfe/trunk/lib/Headers/__clang_cuda_device_functions.h Fri Mar 15 12:04:46 
2019
@@ -520,8 +520,8 @@ __DEVICE__ unsigned int __sad(int __a, i
 __DEVICE__ float __saturatef(float __a) { return __nv_saturatef(__a); }
 __DEVICE__ int __signbitd(double __a) { return __nv_signbitd(__a); }
 __DEVICE__ int __signbitf(float __a) { return __nv_signbitf(__a); }
-__DEVICE__ void __sincosf(float __a, float *__sptr, float *__cptr) {
-  return __nv_fast_sincosf(__a, __sptr, __cptr);
+__DEVICE__ void __sincosf(float __a, float *__s, float *__c) {
+  return __nv_fast_sincosf(__a, __s, __c);
 }
 __DEVICE__ float __sinf(float __a) { return __nv_fast_sinf(__a); }
 __DEVICE__ int __syncthreads_and(int __a) { return __nvvm_bar0_and(__a); }
@@ -1713,17 +1713,17 @@ __DEVICE__ float scalblnf(float __a, lon
   return scalbnf(__a, (int)__b);
 }
 __DEVICE__ double sin(double __a) { return __nv_sin(__a); }
-__DEVICE__ void sincos(double __a, double *__sptr, double *__cptr) {
-  return __nv_sincos(__a, __sptr, __cptr);
+__DEVICE__ void sincos(double __a, double *__s, double *__c) {
+  return __nv_sincos(__a, __s, __c);
 }
-__DEVICE__ void sincosf(float __a, float *__sptr, float *__cptr) {
-  return __FAST_OR_SLOW(__nv_fast_sincosf, __nv_sincosf)(__a, __sptr, __cptr);
+__DEVICE__ void sincosf(float __a, float *__s, float *__c) {
+  return __FAST_OR_SLOW(__nv_fast_sincosf, __nv_sincosf)(__a, __s, __c);
 }
-__DEVICE__ void sincospi(double __a, double *__sptr, double *__cptr) {
-  return __nv_sincospi(__a, __sptr, __cptr);
+__DEVICE__ void sincospi(double __a, double *__s, double *__c) {
+  return __nv_sincospi(__a, __s, __c);
 }
-__DEVICE__ void sincospif(float __a, float *__sptr, float *__cptr) {
-  return __nv_sincospif(__a, __sptr, __cptr);
+__DEVICE__ void sincospif(float __a, float *__s, float *__c) {
+  return __nv_sincospif(__a, __s, __c);
 }
 __DEVICE__ float sinf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_sinf, __nv_sinf)(__a);

Modified: cfe/trunk/lib/Headers/__clang_cuda_libdevice_declares.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/__clang_cuda_libdevice_declares.h?rev=356291&r1=356290&r2=356291&view=diff
==
--- cfe/trunk/lib/Headers/__clang_cuda_libdevice_declares.h (original)
+++ cfe/trunk/lib/Headers/__clang_cuda_libdevice_declares.h Fri Mar 15 12:04:46 
2019
@@ -141,7 +141,7 @@ __device__ float __nv_fast_log10f(float
 __device__ float __nv_fast_log2f(float __a);
 __device__ float __nv_fast_logf(float __a);
 __device__ float __nv_fast_powf(float __a, float __b);
-__device__ void __nv_fast_sincosf(float __a, float *__sptr, float *__cptr);
+__device__ void __nv_fast_sincosf(float __a, float *__s, float *__c);
 __device__ float __nv_fast_sinf(float __a);
 __device__ float __nv_fast_tanf(float __a);
 __device__ double __nv_fdim(double __a, double __b);


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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

jdoerfert wrote:
> > What is this buffer used for? Transferring pointers to the shread variables 
> > to the parallel regions? If so, it must be handled by the compiler. There 
> > are several reasons to do this:
> > 1) You're using malloc/free functions for large buffers. The fact is that 
> > the size of this buffer is known at the compile time and compiler can 
> > generate the fixed size buffer in the global memory if required. We already 
> > have similar implementation for target regions, globalized variables etc. 
> > You can take a look and adapt it for your purpose.
> > 2) Malloc/free are not very fast on the GPU, so it will get an additional 
> > performance with the preallocated buffers.
> > 3) Another one problem with malloc/free is that they are using preallocated 
> > memory and the size of this memory is limited by 8Mb (if I do recall 
> > correctly). This memory is required for the correct support of the local 
> > variables globalization and we alredy ran into the situation when malloc 
> > could not allocate enough memory for it with some previous implementations.
> > 4) You can reused the shared memory buffers already generated by the 
> > compiler and save shared memory.
> 
> [Quote by ABataev copied from 
> https://reviews.llvm.org/D59319?id=190767#inline-525900 after the patch was 
> split.]
> 
> 
> This buffer is supposed to be used to communicate variables in shared and 
> firstprivate clauses between threads in a team. In this patch it is simply 
> used to implement the old `void**` buffer. How, when, if we use it is part of 
> the interface implementation. For now, this buffer simply serves the users of 
> the `omptarget_nvptx_globalArgs` global.
> 
> If you want to provide compiler allocated memory to avoid the buffer use, no 
> problem,
> the `__kmpc_target_region_kernel_parallel` function allows to do so, see the 
> `SharedMemPointers` flag. I wouldn't want to put the logic to generate these 
> buffers in the front-end though.
Why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424



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

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D59316#1429580 , @yaxunl wrote:

> Here we are looking at the code which emulates a "linker" for HIP toolchain. 
> The offloading action builder requests the offloading toolchain have a 
> linker, but amdgpu does not have a real linker (ISA level linker), so we have 
> to emulate that. If we have an ISA level linker we can get rid of all these 
> stuff, but I don't think that will happen in short time.


This isn't really true. We do run lld to link the final executable. It also 
doesn't change that opt and llc should never be involved in the process


Repository:
  rC Clang

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

https://reviews.llvm.org/D59316



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 190868.
jdoerfert added a comment.

Fix a typo (use of wrong variable) and improve comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319

Files:
  openmp/libomptarget/deviceRTLs/common/target_region.h
  openmp/libomptarget/deviceRTLs/nvptx/CMakeLists.txt
  openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu

Index: openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu
===
--- /dev/null
+++ openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu
@@ -0,0 +1,205 @@
+//===-- target_region.cu  CUDA impl. of the target region interface -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains the implementation of the common target region interface.
+//
+//===--===//
+
+// Include the native definitions first as certain defines might be needed in
+// the common interface definition below.
+#include "interface.h"
+#include "omptarget-nvptx.h"
+
+#include "../../common/target_region.h"
+
+EXTERN void *__kmpc_target_region_kernel_get_shared_memory() {
+  return _shared_bytes_buffer_memory.begin();
+}
+EXTERN void *__kmpc_target_region_kernel_get_private_memory() {
+  return ((char *)_shared_bytes_buffer_memory.begin()) +
+ _shared_bytes_buffer_memory.get_offset();
+}
+
+/// Simple generic state machine for worker threads.
+INLINE static void
+__kmpc_target_region_state_machine(ident_t *Ident,
+   bool IsOMPRuntimeInitialized) {
+
+  do {
+void *WorkFn = 0;
+
+// Wait for the signal that we have a new work function.
+__kmpc_barrier_simple_spmd(Ident, 0);
+
+// Retrieve the work function from the runtime.
+bool IsActive = __kmpc_kernel_parallel(&WorkFn, IsOMPRuntimeInitialized);
+
+// If there is nothing more to do, break out of the state machine by
+// returning to the caller.
+if (!WorkFn)
+  return;
+
+if (IsActive) {
+  void *SharedVars = __kmpc_target_region_kernel_get_shared_memory();
+  void *PrivateVars = __kmpc_target_region_kernel_get_private_memory();
+
+  ((ParallelWorkFnTy)WorkFn)(SharedVars, PrivateVars);
+
+  __kmpc_kernel_end_parallel();
+}
+
+__kmpc_barrier_simple_spmd(Ident, 0);
+
+  } while (true);
+}
+
+/// Filter threads into masters and workers. If \p UseStateMachine is true,
+/// required workers will enter a state machine through and be trapped there.
+/// Master and surplus worker threads will return from this function immediately
+/// while required workers will only return once there is no more work. The
+/// return value indicates if the thread is a master (1), a surplus worker (0),
+/// or a finished required worker released from the state machine (-1).
+INLINE static int8_t
+__kmpc_target_region_thread_filter(ident_t *Ident, unsigned ThreadLimit,
+   bool UseStateMachine,
+   bool IsOMPRuntimeInitialized) {
+
+  unsigned TId = GetThreadIdInBlock();
+  bool IsWorker = TId < ThreadLimit;
+
+  if (IsWorker) {
+if (UseStateMachine)
+  __kmpc_target_region_state_machine(Ident, IsOMPRuntimeInitialized);
+return -1;
+  }
+
+  return TId == GetMasterThreadID();
+}
+
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool UseSPMDMode,
+   bool UseStateMachine,
+   bool RequiresOMPRuntime,
+   bool RequiresDataSharing) {
+  unsigned NumThreads = GetNumberOfThreadsInBlock();
+
+  // Handle the SPMD case first.
+  if (UseSPMDMode) {
+
+__kmpc_spmd_kernel_init(NumThreads, RequiresOMPRuntime,
+RequiresDataSharing);
+
+if (RequiresDataSharing)
+  __kmpc_data_sharing_init_stack_spmd();
+
+return 1;
+  }
+
+  // Reserve one WARP in non-SPMD mode for the masters.
+  unsigned ThreadLimit = NumThreads - WARPSIZE;
+  int8_t FilterVal = __kmpc_target_region_thread_filter(
+  Ident, ThreadLimit, UseStateMachine, RequiresOMPRuntime);
+
+  // If the filter returns 1 the executing thread is a team master which will
+  // initialize the kernel in the following.
+  if (FilterVal == 1) {
+__kmpc_kernel_init(ThreadLimit, RequiresOMPRuntime);
+__kmpc_data_sharing_init_stack();
+_shared_bytes_buffer_memory.init();
+  }
+
+  return FilterVal;
+}
+
+EXTERN void __kmpc_target_region_kernel_deinit(ident_t *Ident, bool UseSPMDMode,
+   bool RequiredOMPRuntime)

[PATCH] D59426: [PR41010][OpenCL] Allow OpenCL C style vector initialization in C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: rjmccall, bader.
Herald added subscribers: jdoerfert, ebevhan, yaxunl.

Not sure it's the right approach but the idea is that we would like to accept 
vector contraction syntax from OpenCL C.

This commit fixes Sema checks when creating a vector using compound vector 
syntax. We should pass the type of initializing expression consistently, to 
avoid mismatch with the initializer.

This change also improves testing for the vector creation.


https://reviews.llvm.org/D59426

Files:
  lib/Sema/SemaInit.cpp
  test/CodeGenOpenCL/vector_literals_valid.cl

Index: test/CodeGenOpenCL/vector_literals_valid.cl
===
--- test/CodeGenOpenCL/vector_literals_valid.cl
+++ test/CodeGenOpenCL/vector_literals_valid.cl
@@ -1,22 +1,61 @@
-// RUN: %clang_cc1 -emit-llvm %s -o %t
+// RUN: %clang_cc1 -emit-llvm %s -o - -O0 | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -cl-std=c++ -O0 | FileCheck %s
 
-typedef __attribute__(( ext_vector_type(2) ))  int int2;
-typedef __attribute__(( ext_vector_type(3) ))  int int3;
+typedef __attribute__((ext_vector_type(2))) int int2;
+typedef __attribute__((ext_vector_type(3))) int int3;
 typedef __attribute__(( ext_vector_type(4) ))  int int4;
 typedef __attribute__(( ext_vector_type(8) ))  int int8;
-typedef __attribute__(( ext_vector_type(4) ))  float float4;
+typedef __attribute__((ext_vector_type(4))) float float4;
 
 void vector_literals_valid() {
-  int4 a_1_1_1_1 = (int4)(1,2,3,4);
-  int4 a_2_1_1 = (int4)((int2)(1,2),3,4);
-  int4 a_1_2_1 = (int4)(1,(int2)(2,3),4);
-  int4 a_1_1_2 = (int4)(1,2,(int2)(3,4));
-  int4 a_2_2 = (int4)((int2)(1,2),(int2)(3,4));
-  int4 a_3_1 = (int4)((int3)(1,2,3),4);
-  int4 a_1_3 = (int4)(1,(int3)(2,3,4));
+  //CHECK: store <4 x i32> , <4 x i32>*
+
+  int4 a_1_1_1_1 = (int4)(1, 2, 3, 4);
+
+  //CHECK: store <2 x i32> , <2 x i32>*
+  //CHECK: shufflevector <2 x i32> %{{[0-9]+}}, <2 x i32> undef, <4 x i32> 
+  //CHECK: shufflevector <4 x i32> %{{.+}}, <4 x i32> undef, <4 x i32> 
+  //CHECK: insertelement <4 x i32> %{{.+}}, i32 3, i32 2
+  //CHECK: insertelement <4 x i32> %{{.+}}, i32 4, i32 3
+  int4 a_2_1_1 = (int4)((int2)(1, 2), 3, 4);
+
+  //CHECK: store <2 x i32> , <2 x i32>*
+  //CHECK: shufflevector <2 x i32> %{{[0-9]+}}, <2 x i32> undef, <4 x i32> 
+  //CHECK: shufflevector <4 x i32> , <4 x i32> %{{.+}}, <4 x i32> 
+  //CHECK: insertelement <4 x i32> %{{.+}}, i32 4, i32 3
+  int4 a_1_2_1 = (int4)(1, (int2)(2, 3), 4);
+
+  //CHECK: store <2 x i32> , <2 x i32>*
+  //CHECK: shufflevector <2 x i32> %{{[0-9]+}}, <2 x i32> undef, <4 x i32> 
+  //CHECK: shufflevector <4 x i32> , <4 x i32> %{{.+}}, <4 x i32> 
+  int4 a_1_1_2 = (int4)(1, 2, (int2)(3, 4));
+
+  //CHECK: store <2 x i32> , <2 x i32>*
+  //CHECK: shufflevector <2 x i32> %{{[0-9]+}}, <2 x i32> undef, <4 x i32> 
+  //CHECK: shufflevector <4 x i32> %{{.+}}, <4 x i32> undef, <4 x i32> 
+  //CHECK: store <2 x i32> , <2 x i32>*
+  //CHECK: shufflevector <2 x i32> %{{[0-9]+}}, <2 x i32> undef, <4 x i32> 
+  //CHECK: shufflevector <4 x i32> %{{.+}}, <4 x i32> %{{.+}}, <4 x i32> 
+  int4 a_2_2 = (int4)((int2)(1, 2), (int2)(3, 4));
+
+  //CHECK: store <4 x i32> , <4 x i32>*
+  //CHECK: shufflevector <4 x i32> %{{.+}}, <4 x i32> undef, <3 x i32> 
+  //CHECK: shufflevector <3 x i32> %{{.+}}, <3 x i32> undef, <4 x i32> 
+  //CHECK: shufflevector <4 x i32> , <4 x i32> %{{.+}}, <4 x i32> 
+  int4 a_1_3 = (int4)(1, (int3)(2, 3, 4));
+
+  //CHECK: store <4 x i32> , <4 x i32>* %a
   int4 a = (int4)(1);
-  int8 b = (int8)(1,2,a.xy,a);
-  float4 V2 = (float4) (1);
-}
 
+  //CHECK: load <4 x i32>, <4 x i32>* %a, align 16
+  //CHECK: shufflevector <4 x i32> %{{[0-9]+}}, <4 x i32> undef, <2 x i32> 
+  //CHECK: shufflevector <2 x i32> %{{[0-9]+}}, <2 x i32> undef, <8 x i32> 
+  //CHECK: shufflevector <8 x i32> , <8 x i32> %{{.+}}, <8 x i32> 
+  //CHECK: load <4 x i32>, <4 x i32>* %a, align 16
+  //CHECK: shufflevector <4 x i32> %{{[0-9]+}}, <4 x i32> undef, <8 x i32> 
+  //CHECK: shufflevector <8 x i32> %{{.+}}, <8 x i32> %{{.+}}, <8 x i32> 
+  int8 b = (int8)(1, 2, a.xy, a);
 
+  //CHECK: store <4 x float> , <4 x float>* %V2
+  float4 V2 = (float4)(1);
+}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -1289,7 +1289,12 @@
 // FIXME: Better EqualLoc?
 InitializationKind Kind =
 InitializationKind::CreateCopy(expr->getBeginLoc(), SourceLocation());
-InitializationSequence Seq(SemaRef, Entity, Kind, expr,
+auto TmpEntity =
+(ElemType->isExtVectorType() && !Entity.getType()->isExtVectorType())
+? InitializedEntity::InitializeTemporary(ElemType)
+: Entity;
+
+InitializationSequence Seq(SemaRef, TmpEntity, Kind, expr,
/*TopLevelOfInitList*/ true);
 
 // C++14 [dcl.init.aggr]p13:
@@ -1300,8 +1305,7 @@
   

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D59316#1431238 , @arsenm wrote:

> In D59316#1429580 , @yaxunl wrote:
>
> > Here we are looking at the code which emulates a "linker" for HIP 
> > toolchain. The offloading action builder requests the offloading toolchain 
> > have a linker, but amdgpu does not have a real linker (ISA level linker), 
> > so we have to emulate that. If we have an ISA level linker we can get rid 
> > of all these stuff, but I don't think that will happen in short time.
>
>
> This isn't really true. We do run lld to link the final executable. It also 
> doesn't change that opt and llc should never be involved in the process


Can lld do ISA level linking? That is, one device function in one object file 
calls another device function in a different object file, and we let lld link 
them together?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59316



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool 
UseSPMDMode,
+   bool RequiresOMPRuntime,

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` 
> > > parameters are not needed anymore. They are passed in `ident_t` structure.
> > > If you're using ident_t UseSPMDMode and RequiresOMPRuntime parameters are 
> > > not needed anymore. They are passed in ident_t structure.
> > 
> > They are not in the TRegion interface, at least not by the TRegion code 
> > generation. If required, we can add that or require the 
> > `__kmpc_target_region_kernel_init` implementation to store the values in 
> > the `ident_t`. Regardless, we do not want to hide the variables in the 
> > `ident_t` because that would result in worse analysis results and cause 
> > optimizations to be harder. The main point of all these changes is, after 
> > all, to make optimizations easy. Given that we expect these functions to be 
> > inlined, there is also no harm done wrt. runtime costs.
> > 
> > 
> > 
> > 
> This is why we used them. Those `ident_t`s  are constant and it allows us to 
> perform an additional optimization in the functions, that do not have 
> `isSPMDMpde` and `RequiresFullRuntime`. Because of this parameter, we gained 
> a significant performance boost. LLVM knows how to deal with the structures, 
> don't worry about the optimization.
> This is why we used them. Those ident_ts are constant and it allows us to 
> perform an additional optimization in the functions, that do not have 
> isSPMDMpde and RequiresFullRuntime.

The boolean parameters are (currently) also constant. The main point however is 
that in our expected use case, an inlined device RTL, there is literally no 
cost to pay by having the flags explicit as parameters.


> Because of this parameter, we gained a significant performance boost.

Compared to what? Not having information about the execution mode, etc. at all? 
How would that become worse? 




> LLVM knows how to deal with the structures, don't worry about the 
> optimization.

I am (painfully) aware of LLVM's capability to promote arguments (that is what 
is needed if we do not inline or perform IP-SCCP). However, using a pointer 
does allow the use of non-constant `ident_t` values, which are problematic. 
They might actually be useful for the original purpose of `ident_t`, namely 
location information. Think function merging that will cause a call with one of 
multiple different `ident_t` pointers. Making sure we can promote the values in 
that case is already much harder than checking if all potential values are the 
same boolean constant.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59094: [ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a reviewer: t.p.northover.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/CodeGen/TargetInfo.cpp:6023
+  // Variadic functions should always marshal to the base standard.
+  bool isAAPCS_VFP =
+  !isVariadic && isEffectivelyAAPCS_VFP(functionCallConv, /* AAPCS16 */ 
true);

Variable name should be capitalized.


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

https://reviews.llvm.org/D59094



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D59302#1431045 , @ilya-biryukov 
wrote:

> I'll look more closely into the details, but just a high-level question now: 
> why would we want to make this optional and not simply surface these extra 
> diagnostics?


The optional part is rather limiting number of diagnostics that is going to be 
surfaced. So if you have thousands of errors inside preamble only first 100 of 
them will appear inside main file by default.
But looking at it again, currently there is no way for user to say "do not 
limit number of diagnostics" :D They can only change this limit to different 
values.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

ABataev wrote:
> jdoerfert wrote:
> > > What is this buffer used for? Transferring pointers to the shread 
> > > variables to the parallel regions? If so, it must be handled by the 
> > > compiler. There are several reasons to do this:
> > > 1) You're using malloc/free functions for large buffers. The fact is that 
> > > the size of this buffer is known at the compile time and compiler can 
> > > generate the fixed size buffer in the global memory if required. We 
> > > already have similar implementation for target regions, globalized 
> > > variables etc. You can take a look and adapt it for your purpose.
> > > 2) Malloc/free are not very fast on the GPU, so it will get an additional 
> > > performance with the preallocated buffers.
> > > 3) Another one problem with malloc/free is that they are using 
> > > preallocated memory and the size of this memory is limited by 8Mb (if I 
> > > do recall correctly). This memory is required for the correct support of 
> > > the local variables globalization and we alredy ran into the situation 
> > > when malloc could not allocate enough memory for it with some previous 
> > > implementations.
> > > 4) You can reused the shared memory buffers already generated by the 
> > > compiler and save shared memory.
> > 
> > [Quote by ABataev copied from 
> > https://reviews.llvm.org/D59319?id=190767#inline-525900 after the patch was 
> > split.]
> > 
> > 
> > This buffer is supposed to be used to communicate variables in shared and 
> > firstprivate clauses between threads in a team. In this patch it is simply 
> > used to implement the old `void**` buffer. How, when, if we use it is part 
> > of the interface implementation. For now, this buffer simply serves the 
> > users of the `omptarget_nvptx_globalArgs` global.
> > 
> > If you want to provide compiler allocated memory to avoid the buffer use, 
> > no problem,
> > the `__kmpc_target_region_kernel_parallel` function allows to do so, see 
> > the `SharedMemPointers` flag. I wouldn't want to put the logic to generate 
> > these buffers in the front-end though.
> Why?
Why what?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424



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

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D59316#1431253 , @yaxunl wrote:

> In D59316#1431238 , @arsenm wrote:
>
> > In D59316#1429580 , @yaxunl wrote:
> >
> > > Here we are looking at the code which emulates a "linker" for HIP 
> > > toolchain. The offloading action builder requests the offloading 
> > > toolchain have a linker, but amdgpu does not have a real linker (ISA 
> > > level linker), so we have to emulate that. If we have an ISA level linker 
> > > we can get rid of all these stuff, but I don't think that will happen in 
> > > short time.
> >
> >
> > This isn't really true. We do run lld to link the final executable. It also 
> > doesn't change that opt and llc should never be involved in the process
>
>
> Can lld do ISA level linking? That is, one device function in one object file 
> calls another device function in a different object file, and we let lld link 
> them together?


We can't link multiple objects, but we do need to link the single object with 
lld. The relocations even for functions in the same module are 0 until lld 
fixes them up. Do we have execution tests for function calls using HIP? Since 
it looks like lld isn't getting used here, I suspect they aren't working


Repository:
  rC Clang

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

https://reviews.llvm.org/D59316



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool 
UseSPMDMode,
+   bool RequiresOMPRuntime,

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` 
> > > > parameters are not needed anymore. They are passed in `ident_t` 
> > > > structure.
> > > > If you're using ident_t UseSPMDMode and RequiresOMPRuntime parameters 
> > > > are not needed anymore. They are passed in ident_t structure.
> > > 
> > > They are not in the TRegion interface, at least not by the TRegion code 
> > > generation. If required, we can add that or require the 
> > > `__kmpc_target_region_kernel_init` implementation to store the values in 
> > > the `ident_t`. Regardless, we do not want to hide the variables in the 
> > > `ident_t` because that would result in worse analysis results and cause 
> > > optimizations to be harder. The main point of all these changes is, after 
> > > all, to make optimizations easy. Given that we expect these functions to 
> > > be inlined, there is also no harm done wrt. runtime costs.
> > > 
> > > 
> > > 
> > > 
> > This is why we used them. Those `ident_t`s  are constant and it allows us 
> > to perform an additional optimization in the functions, that do not have 
> > `isSPMDMpde` and `RequiresFullRuntime`. Because of this parameter, we 
> > gained a significant performance boost. LLVM knows how to deal with the 
> > structures, don't worry about the optimization.
> > This is why we used them. Those ident_ts are constant and it allows us to 
> > perform an additional optimization in the functions, that do not have 
> > isSPMDMpde and RequiresFullRuntime.
> 
> The boolean parameters are (currently) also constant. The main point however 
> is that in our expected use case, an inlined device RTL, there is literally 
> no cost to pay by having the flags explicit as parameters.
> 
> 
> > Because of this parameter, we gained a significant performance boost.
> 
> Compared to what? Not having information about the execution mode, etc. at 
> all? How would that become worse? 
> 
> 
> 
> 
> > LLVM knows how to deal with the structures, don't worry about the 
> > optimization.
> 
> I am (painfully) aware of LLVM's capability to promote arguments (that is 
> what is needed if we do not inline or perform IP-SCCP). However, using a 
> pointer does allow the use of non-constant `ident_t` values, which are 
> problematic. They might actually be useful for the original purpose of 
> `ident_t`, namely location information. Think function merging that will 
> cause a call with one of multiple different `ident_t` pointers. Making sure 
> we can promote the values in that case is already much harder than checking 
> if all potential values are the same boolean constant.
> 
1. This is the data duplication.
2. Compared to the previous implementation.
3. It allows, yes, but the compiler generates constant `ident_t`. This 
structure used not only for the location information, but it used also for 
other purposes. There are no problems with the code inlining and optimization 
for `ident_t`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > > What is this buffer used for? Transferring pointers to the shread 
> > > > variables to the parallel regions? If so, it must be handled by the 
> > > > compiler. There are several reasons to do this:
> > > > 1) You're using malloc/free functions for large buffers. The fact is 
> > > > that the size of this buffer is known at the compile time and compiler 
> > > > can generate the fixed size buffer in the global memory if required. We 
> > > > already have similar implementation for target regions, globalized 
> > > > variables etc. You can take a look and adapt it for your purpose.
> > > > 2) Malloc/free are not very fast on the GPU, so it will get an 
> > > > additional performance with the preallocated buffers.
> > > > 3) Another one problem with malloc/free is that they are using 
> > > > preallocated memory and the size of this memory is limited by 8Mb (if I 
> > > > do recall correctly). This memory is required for the correct support 
> > > > of the local variables globalization and we alredy ran into the 
> > > > situation when malloc could not allocate enough memory for it with some 
> > > > previous implementations.
> > > > 4) You can reused the shared memory buffers already generated by the 
> > > > compiler and save shared memory.
> > > 
> > > [Quote by ABataev copied from 
> > > https://reviews.llvm.org/D59319?id=190767#inline-525900 after the patch 
> > > was split.]
> > > 
> > > 
> > > This buffer is supposed to be used to communicate variables in shared and 
> > > firstprivate clauses between threads in a team. In this patch it is 
> > > simply used to implement the old `void**` buffer. How, when, if we use it 
> > > is part of the interface implementation. For now, this buffer simply 
> > > serves the users of the `omptarget_nvptx_globalArgs` global.
> > > 
> > > If you want to provide compiler allocated memory to avoid the buffer use, 
> > > no problem,
> > > the `__kmpc_target_region_kernel_parallel` function allows to do so, see 
> > > the `SharedMemPointers` flag. I wouldn't want to put the logic to 
> > > generate these buffers in the front-end though.
> > Why?
> Why what?
Why you don't want to put the buffers generation to the compiler?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424



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

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D59316#1431276 , @arsenm wrote:

> In D59316#1431253 , @yaxunl wrote:
>
> > In D59316#1431238 , @arsenm wrote:
> >
> > > In D59316#1429580 , @yaxunl 
> > > wrote:
> > >
> > > > Here we are looking at the code which emulates a "linker" for HIP 
> > > > toolchain. The offloading action builder requests the offloading 
> > > > toolchain have a linker, but amdgpu does not have a real linker (ISA 
> > > > level linker), so we have to emulate that. If we have an ISA level 
> > > > linker we can get rid of all these stuff, but I don't think that will 
> > > > happen in short time.
> > >
> > >
> > > This isn't really true. We do run lld to link the final executable. It 
> > > also doesn't change that opt and llc should never be involved in the 
> > > process
> >
> >
> > Can lld do ISA level linking? That is, one device function in one object 
> > file calls another device function in a different object file, and we let 
> > lld link them together?
>
>
> We can't link multiple objects, but we do need to link the single object with 
> lld. The relocations even for functions in the same module are 0 until lld 
> fixes them up. Do we have execution tests for function calls using HIP? Since 
> it looks like lld isn't getting used here, I suspect they aren't workingh




In D59316#1431276 , @arsenm wrote:

> In D59316#1431253 , @yaxunl wrote:
>
> > In D59316#1431238 , @arsenm wrote:
> >
> > > In D59316#1429580 , @yaxunl 
> > > wrote:
> > >
> > > > Here we are looking at the code which emulates a "linker" for HIP 
> > > > toolchain. The offloading action builder requests the offloading 
> > > > toolchain have a linker, but amdgpu does not have a real linker (ISA 
> > > > level linker), so we have to emulate that. If we have an ISA level 
> > > > linker we can get rid of all these stuff, but I don't think that will 
> > > > happen in short time.
> > >
> > >
> > > This isn't really true. We do run lld to link the final executable. It 
> > > also doesn't change that opt and llc should never be involved in the 
> > > process
> >
> >
> > Can lld do ISA level linking? That is, one device function in one object 
> > file calls another device function in a different object file, and we let 
> > lld link them together?
>
>
> We can't link multiple objects, but we do need to link the single object with 
> lld. The relocations even for functions in the same module are 0 until lld 
> fixes them up. Do we have execution tests for function calls using HIP? Since 
> it looks like lld isn't getting used here, I suspect they aren't working


hip-clang has been used for compiling and running real ML applications without 
issue. We do have lld step, see line 281. We do need to link different objects 
for this LinkerJob.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59316



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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > > What is this buffer used for? Transferring pointers to the shread 
> > > > > variables to the parallel regions? If so, it must be handled by the 
> > > > > compiler. There are several reasons to do this:
> > > > > 1) You're using malloc/free functions for large buffers. The fact is 
> > > > > that the size of this buffer is known at the compile time and 
> > > > > compiler can generate the fixed size buffer in the global memory if 
> > > > > required. We already have similar implementation for target regions, 
> > > > > globalized variables etc. You can take a look and adapt it for your 
> > > > > purpose.
> > > > > 2) Malloc/free are not very fast on the GPU, so it will get an 
> > > > > additional performance with the preallocated buffers.
> > > > > 3) Another one problem with malloc/free is that they are using 
> > > > > preallocated memory and the size of this memory is limited by 8Mb (if 
> > > > > I do recall correctly). This memory is required for the correct 
> > > > > support of the local variables globalization and we alredy ran into 
> > > > > the situation when malloc could not allocate enough memory for it 
> > > > > with some previous implementations.
> > > > > 4) You can reused the shared memory buffers already generated by the 
> > > > > compiler and save shared memory.
> > > > 
> > > > [Quote by ABataev copied from 
> > > > https://reviews.llvm.org/D59319?id=190767#inline-525900 after the patch 
> > > > was split.]
> > > > 
> > > > 
> > > > This buffer is supposed to be used to communicate variables in shared 
> > > > and firstprivate clauses between threads in a team. In this patch it is 
> > > > simply used to implement the old `void**` buffer. How, when, if we use 
> > > > it is part of the interface implementation. For now, this buffer simply 
> > > > serves the users of the `omptarget_nvptx_globalArgs` global.
> > > > 
> > > > If you want to provide compiler allocated memory to avoid the buffer 
> > > > use, no problem,
> > > > the `__kmpc_target_region_kernel_parallel` function allows to do so, 
> > > > see the `SharedMemPointers` flag. I wouldn't want to put the logic to 
> > > > generate these buffers in the front-end though.
> > > Why?
> > Why what?
> Why you don't want to put the buffers generation to the compiler?
> Why you don't want to put the buffers generation to the compiler?

I did never say that. I explicitly explained how the new interface allows you 
to do exactly that. Maybe you confuse it with me not wanting the generation to 
be part of the front-end (=Clang). That is because it is an implementation 
choice for performance, as such it should not be done by Clang if it might 
obstruct analyses later on, or could be done better with more analysis support.

If we conclude we actually need to share values, after we tried to eliminate 
the need for sharing altogether, we can decide if a global buffer is 
preferable. If so, we can check if
there is already one that is unused or if we would need to create a new one.

//Regarding this patch:// It is not supposed to change the behavior at all. 
This patch just introduces a more general buffer which is then used to 
implement the old buffer. How/when the buffer is used is not affected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424



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

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D59316#1431284 , @yaxunl wrote:

> In D59316#1431276 , @arsenm wrote:
>
> > In D59316#1431253 , @yaxunl wrote:
> >
> > > In D59316#1431238 , @arsenm 
> > > wrote:
> > >
> > > > In D59316#1429580 , @yaxunl 
> > > > wrote:
> > > >
> > > > > Here we are looking at the code which emulates a "linker" for HIP 
> > > > > toolchain. The offloading action builder requests the offloading 
> > > > > toolchain have a linker, but amdgpu does not have a real linker (ISA 
> > > > > level linker), so we have to emulate that. If we have an ISA level 
> > > > > linker we can get rid of all these stuff, but I don't think that will 
> > > > > happen in short time.
> > > >
> > > >
> > > > This isn't really true. We do run lld to link the final executable. It 
> > > > also doesn't change that opt and llc should never be involved in the 
> > > > process
> > >
> > >
> > > Can lld do ISA level linking? That is, one device function in one object 
> > > file calls another device function in a different object file, and we let 
> > > lld link them together?
> >
> >
> > We can't link multiple objects, but we do need to link the single object 
> > with lld. The relocations even for functions in the same module are 0 until 
> > lld fixes them up. Do we have execution tests for function calls using HIP? 
> > Since it looks like lld isn't getting used here, I suspect they aren't 
> > workingh
>
>
>
>
> In D59316#1431276 , @arsenm wrote:
>
> > In D59316#1431253 , @yaxunl wrote:
> >
> > > In D59316#1431238 , @arsenm 
> > > wrote:
> > >
> > > > In D59316#1429580 , @yaxunl 
> > > > wrote:
> > > >
> > > > > Here we are looking at the code which emulates a "linker" for HIP 
> > > > > toolchain. The offloading action builder requests the offloading 
> > > > > toolchain have a linker, but amdgpu does not have a real linker (ISA 
> > > > > level linker), so we have to emulate that. If we have an ISA level 
> > > > > linker we can get rid of all these stuff, but I don't think that will 
> > > > > happen in short time.
> > > >
> > > >
> > > > This isn't really true. We do run lld to link the final executable. It 
> > > > also doesn't change that opt and llc should never be involved in the 
> > > > process
> > >
> > >
> > > Can lld do ISA level linking? That is, one device function in one object 
> > > file calls another device function in a different object file, and we let 
> > > lld link them together?
> >
> >
> > We can't link multiple objects, but we do need to link the single object 
> > with lld. The relocations even for functions in the same module are 0 until 
> > lld fixes them up. Do we have execution tests for function calls using HIP? 
> > Since it looks like lld isn't getting used here, I suspect they aren't 
> > working
>
>
> hip-clang has been used for compiling and running real ML applications 
> without issue. We do have lld step, see line 281. We do need to link 
> different objects for this LinkerJob.


ML workloads are extremely unlikely to use a call. We should have an execution 
tests with noinline somewhere to stress this


Repository:
  rC Clang

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

https://reviews.llvm.org/D59316



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

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Duncan, thanks for working on better interfaces in clang!

I am just wondering - is it safe to have the lifetime of a single object on 
heap managed by two different `IntrusiveRefCntPtr` instances?




Comment at: clang/lib/Frontend/ASTUnit.cpp:1693
   PrecompilePreambleAfterNParses,
-  AST->FileMgr->getVirtualFileSystem()))
+  &AST->FileMgr->getVirtualFileSystem()))
 return nullptr;

Is this safe?



Comment at: clang/lib/Frontend/ASTUnit.cpp:1800
 assert(FileMgr && "FileMgr is null on Reparse call");
-VFS = FileMgr->getVirtualFileSystem();
+VFS = &FileMgr->getVirtualFileSystem();
   }

Is this safe?



Comment at: clang/lib/Frontend/ASTUnit.cpp:2236
 
-auto VFS = FileMgr.getVirtualFileSystem();
+IntrusiveRefCntPtr VFS =
+&FileMgr.getVirtualFileSystem();

Is this safe?



Comment at: clang/lib/Frontend/CompilerInstance.cpp:299
   if (!VFS)
-VFS = FileMgr ? FileMgr->getVirtualFileSystem()
+VFS = FileMgr ? &FileMgr->getVirtualFileSystem()
   : createVFSFromCompilerInvocation(getInvocation(),

Is this safe?



Comment at: clang/lib/Tooling/Tooling.cpp:304
   const std::unique_ptr Driver(
-  newDriver(&Diagnostics, BinaryName, Files->getVirtualFileSystem()));
+  newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
   // The "input file not found" diagnostics from the driver are useful.

The `Driver` constructor takes `IntrusiveRefCntPtr< llvm::vfs::FileSystem >` as 
an argument. Is this safe?

https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#a1930cae44e647c1983d11d6a61ce14ed


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

https://reviews.llvm.org/D59388



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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > > What is this buffer used for? Transferring pointers to the shread 
> > > > > > variables to the parallel regions? If so, it must be handled by the 
> > > > > > compiler. There are several reasons to do this:
> > > > > > 1) You're using malloc/free functions for large buffers. The fact 
> > > > > > is that the size of this buffer is known at the compile time and 
> > > > > > compiler can generate the fixed size buffer in the global memory if 
> > > > > > required. We already have similar implementation for target 
> > > > > > regions, globalized variables etc. You can take a look and adapt it 
> > > > > > for your purpose.
> > > > > > 2) Malloc/free are not very fast on the GPU, so it will get an 
> > > > > > additional performance with the preallocated buffers.
> > > > > > 3) Another one problem with malloc/free is that they are using 
> > > > > > preallocated memory and the size of this memory is limited by 8Mb 
> > > > > > (if I do recall correctly). This memory is required for the correct 
> > > > > > support of the local variables globalization and we alredy ran into 
> > > > > > the situation when malloc could not allocate enough memory for it 
> > > > > > with some previous implementations.
> > > > > > 4) You can reused the shared memory buffers already generated by 
> > > > > > the compiler and save shared memory.
> > > > > 
> > > > > [Quote by ABataev copied from 
> > > > > https://reviews.llvm.org/D59319?id=190767#inline-525900 after the 
> > > > > patch was split.]
> > > > > 
> > > > > 
> > > > > This buffer is supposed to be used to communicate variables in shared 
> > > > > and firstprivate clauses between threads in a team. In this patch it 
> > > > > is simply used to implement the old `void**` buffer. How, when, if we 
> > > > > use it is part of the interface implementation. For now, this buffer 
> > > > > simply serves the users of the `omptarget_nvptx_globalArgs` global.
> > > > > 
> > > > > If you want to provide compiler allocated memory to avoid the buffer 
> > > > > use, no problem,
> > > > > the `__kmpc_target_region_kernel_parallel` function allows to do so, 
> > > > > see the `SharedMemPointers` flag. I wouldn't want to put the logic to 
> > > > > generate these buffers in the front-end though.
> > > > Why?
> > > Why what?
> > Why you don't want to put the buffers generation to the compiler?
> > Why you don't want to put the buffers generation to the compiler?
> 
> I did never say that. I explicitly explained how the new interface allows you 
> to do exactly that. Maybe you confuse it with me not wanting the generation 
> to be part of the front-end (=Clang). That is because it is an implementation 
> choice for performance, as such it should not be done by Clang if it might 
> obstruct analyses later on, or could be done better with more analysis 
> support.
> 
> If we conclude we actually need to share values, after we tried to eliminate 
> the need for sharing altogether, we can decide if a global buffer is 
> preferable. If so, we can check if
> there is already one that is unused or if we would need to create a new one.
> 
> //Regarding this patch:// It is not supposed to change the behavior at all. 
> This patch just introduces a more general buffer which is then used to 
> implement the old buffer. How/when the buffer is used is not affected.
Only Clang can do better analysis for the shared variables, runtime is not. 
This part must be implemented in Clang, not in the runtime. I had no time to 
fix the existing implementation of parameters passing in non-SPMD mode. But if 
you working on this, you should definetely implement this in the compiler, not 
in the library.
It does not brea the analysis or anything else. It really produces better code 
with the better performance and less memory use.
If you're going to implement it, you need to implement it in the best possible 
way. Who else is going to fix this later, when we ran into the problems with 
the shared memory and/or `malloc`ed memory?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59424



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:104
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(ident_t *Ident, bool 
UseSPMDMode,
+   bool RequiresOMPRuntime,

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > If you're using `ident_t` `UseSPMDMode` and `RequiresOMPRuntime` 
> > > > > parameters are not needed anymore. They are passed in `ident_t` 
> > > > > structure.
> > > > > If you're using ident_t UseSPMDMode and RequiresOMPRuntime parameters 
> > > > > are not needed anymore. They are passed in ident_t structure.
> > > > 
> > > > They are not in the TRegion interface, at least not by the TRegion code 
> > > > generation. If required, we can add that or require the 
> > > > `__kmpc_target_region_kernel_init` implementation to store the values 
> > > > in the `ident_t`. Regardless, we do not want to hide the variables in 
> > > > the `ident_t` because that would result in worse analysis results and 
> > > > cause optimizations to be harder. The main point of all these changes 
> > > > is, after all, to make optimizations easy. Given that we expect these 
> > > > functions to be inlined, there is also no harm done wrt. runtime costs.
> > > > 
> > > > 
> > > > 
> > > > 
> > > This is why we used them. Those `ident_t`s  are constant and it allows us 
> > > to perform an additional optimization in the functions, that do not have 
> > > `isSPMDMpde` and `RequiresFullRuntime`. Because of this parameter, we 
> > > gained a significant performance boost. LLVM knows how to deal with the 
> > > structures, don't worry about the optimization.
> > > This is why we used them. Those ident_ts are constant and it allows us to 
> > > perform an additional optimization in the functions, that do not have 
> > > isSPMDMpde and RequiresFullRuntime.
> > 
> > The boolean parameters are (currently) also constant. The main point 
> > however is that in our expected use case, an inlined device RTL, there is 
> > literally no cost to pay by having the flags explicit as parameters.
> > 
> > 
> > > Because of this parameter, we gained a significant performance boost.
> > 
> > Compared to what? Not having information about the execution mode, etc. at 
> > all? How would that become worse? 
> > 
> > 
> > 
> > 
> > > LLVM knows how to deal with the structures, don't worry about the 
> > > optimization.
> > 
> > I am (painfully) aware of LLVM's capability to promote arguments (that is 
> > what is needed if we do not inline or perform IP-SCCP). However, using a 
> > pointer does allow the use of non-constant `ident_t` values, which are 
> > problematic. They might actually be useful for the original purpose of 
> > `ident_t`, namely location information. Think function merging that will 
> > cause a call with one of multiple different `ident_t` pointers. Making sure 
> > we can promote the values in that case is already much harder than checking 
> > if all potential values are the same boolean constant.
> > 
> 1. This is the data duplication.
> 2. Compared to the previous implementation.
> 3. It allows, yes, but the compiler generates constant `ident_t`. This 
> structure used not only for the location information, but it used also for 
> other purposes. There are no problems with the code inlining and optimization 
> for `ident_t`s.
> 1. This is the data duplication.

What is? Having explicit constant boolean parameters? There is no "duplication" 
if they are constant and the functions are inlined. If you //really think 
otherwise//, I'm afraid we will not make progress here without a third opinion.


> 2. Compared to the previous implementation.

I do not know what the previous implementation was. I'm also unsure what the 
point is you are trying to make. If it is different from point 1., could you 
please elaborate?

> 3. It allows, yes, but the compiler generates constant ident_t. This 
> structure used not only for the location information, but it used also for 
> other purposes. There are no problems with the code inlining and optimization 
> for ident_ts.

For now, maybe. I just gave you a very plausible example of how there could be 
performance implications in the near future due to the indirection compared to 
explicit boolean parameters.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59424: [OpenMP][NVPTX] Replace void** buffer by byte-wise buffer

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:73
+/// Note: Only the team master is allowed to call non-const functions!
+struct shared_bytes_buffer {
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > > What is this buffer used for? Transferring pointers to the shread 
> > > > > > > variables to the parallel regions? If so, it must be handled by 
> > > > > > > the compiler. There are several reasons to do this:
> > > > > > > 1) You're using malloc/free functions for large buffers. The fact 
> > > > > > > is that the size of this buffer is known at the compile time and 
> > > > > > > compiler can generate the fixed size buffer in the global memory 
> > > > > > > if required. We already have similar implementation for target 
> > > > > > > regions, globalized variables etc. You can take a look and adapt 
> > > > > > > it for your purpose.
> > > > > > > 2) Malloc/free are not very fast on the GPU, so it will get an 
> > > > > > > additional performance with the preallocated buffers.
> > > > > > > 3) Another one problem with malloc/free is that they are using 
> > > > > > > preallocated memory and the size of this memory is limited by 8Mb 
> > > > > > > (if I do recall correctly). This memory is required for the 
> > > > > > > correct support of the local variables globalization and we 
> > > > > > > alredy ran into the situation when malloc could not allocate 
> > > > > > > enough memory for it with some previous implementations.
> > > > > > > 4) You can reused the shared memory buffers already generated by 
> > > > > > > the compiler and save shared memory.
> > > > > > 
> > > > > > [Quote by ABataev copied from 
> > > > > > https://reviews.llvm.org/D59319?id=190767#inline-525900 after the 
> > > > > > patch was split.]
> > > > > > 
> > > > > > 
> > > > > > This buffer is supposed to be used to communicate variables in 
> > > > > > shared and firstprivate clauses between threads in a team. In this 
> > > > > > patch it is simply used to implement the old `void**` buffer. How, 
> > > > > > when, if we use it is part of the interface implementation. For 
> > > > > > now, this buffer simply serves the users of the 
> > > > > > `omptarget_nvptx_globalArgs` global.
> > > > > > 
> > > > > > If you want to provide compiler allocated memory to avoid the 
> > > > > > buffer use, no problem,
> > > > > > the `__kmpc_target_region_kernel_parallel` function allows to do 
> > > > > > so, see the `SharedMemPointers` flag. I wouldn't want to put the 
> > > > > > logic to generate these buffers in the front-end though.
> > > > > Why?
> > > > Why what?
> > > Why you don't want to put the buffers generation to the compiler?
> > > Why you don't want to put the buffers generation to the compiler?
> > 
> > I did never say that. I explicitly explained how the new interface allows 
> > you to do exactly that. Maybe you confuse it with me not wanting the 
> > generation to be part of the front-end (=Clang). That is because it is an 
> > implementation choice for performance, as such it should not be done by 
> > Clang if it might obstruct analyses later on, or could be done better with 
> > more analysis support.
> > 
> > If we conclude we actually need to share values, after we tried to 
> > eliminate the need for sharing altogether, we can decide if a global buffer 
> > is preferable. If so, we can check if
> > there is already one that is unused or if we would need to create a new one.
> > 
> > //Regarding this patch:// It is not supposed to change the behavior at all. 
> > This patch just introduces a more general buffer which is then used to 
> > implement the old buffer. How/when the buffer is used is not affected.
> Only Clang can do better analysis for the shared variables, runtime is not. 
> This part must be implemented in Clang, not in the runtime. I had no time to 
> fix the existing implementation of parameters passing in non-SPMD mode. But 
> if you working on this, you should definetely implement this in the compiler, 
> not in the library.
> It does not brea the analysis or anything else. It really produces better 
> code with the better performance and less memory use.
> If you're going to implement it, you need to implement it in the best 
> possible way. Who else is going to fix this later, when we ran into the 
> problems with the shared memory and/or `malloc`ed memory?
> Only Clang can do better analysis for the shared variables, runtime is not. 
> This part must be implemented in Clang, not in the runtime. I had no time to 
> fix the existing implementation of parameters passing in non-SPMD mode. But 
> if you working on this, you should definetely implement this in the compiler, 
> not in the library.

Clang is not the right place for analysis. //That is what the patch set is all 
about.

r356296 - [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via cfe-commits]

Author: modocache
Date: Fri Mar 15 13:25:49 2019
New Revision: 356296

URL: http://llvm.org/viewvc/llvm-project?rev=356296&view=rev
Log:
[coroutines][PR40978] Emit error for co_yield within catch block

Summary:
As reported in https://bugs.llvm.org/show_bug.cgi?id=40978, it's an
error to use the `co_yield` or `co_await` keywords outside of a valid
"suspension context" as defined by [expr.await]p2 of
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4775.pdf.

Whether or not the current scope was in a function-try-block's
(https://en.cppreference.com/w/cpp/language/function-try-block) handler
could be determined using scope flag `Scope::FnTryCatchScope`. No
such flag existed for a simple C++ catch statement, so this commit adds
one.

Reviewers: GorNishanov, tks2103, rsmith

Reviewed By: GorNishanov

Subscribers: EricWF, jdoerfert, cfe-commits, lewissbaker

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59076

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Scope.h
cfe/trunk/lib/Parse/ParseStmt.cpp
cfe/trunk/lib/Sema/Scope.cpp
cfe/trunk/lib/Sema/SemaCoroutine.cpp
cfe/trunk/test/SemaCXX/coroutines.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=356296&r1=356295&r2=356296&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Mar 15 13:25:49 
2019
@@ -9251,6 +9251,8 @@ def err_coroutine_objc_method : Error<
   "Objective-C methods as coroutines are not yet supported">;
 def err_coroutine_unevaluated_context : Error<
   "'%0' cannot be used in an unevaluated context">;
+def err_coroutine_within_handler : Error<
+  "'%0' cannot be used in the handler of a try block">;
 def err_coroutine_outside_function : Error<
   "'%0' cannot be used outside a function">;
 def err_coroutine_invalid_func_context : Error<

Modified: cfe/trunk/include/clang/Sema/Scope.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Scope.h?rev=356296&r1=356295&r2=356296&view=diff
==
--- cfe/trunk/include/clang/Sema/Scope.h (original)
+++ cfe/trunk/include/clang/Sema/Scope.h Fri Mar 15 13:25:49 2019
@@ -131,6 +131,9 @@ public:
 
 /// We are between inheritance colon and the real class/struct definition 
scope.
 ClassInheritanceScope = 0x80,
+
+/// This is the scope of a C++ catch statement.
+CatchScope = 0x100,
   };
 
 private:

Modified: cfe/trunk/lib/Parse/ParseStmt.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=356296&r1=356295&r2=356296&view=diff
==
--- cfe/trunk/lib/Parse/ParseStmt.cpp (original)
+++ cfe/trunk/lib/Parse/ParseStmt.cpp Fri Mar 15 13:25:49 2019
@@ -2260,8 +2260,10 @@ StmtResult Parser::ParseCXXCatchBlock(bo
   // C++ 3.3.2p3:
   // The name in a catch exception-declaration is local to the handler and
   // shall not be redeclared in the outermost block of the handler.
-  ParseScope CatchScope(this, Scope::DeclScope | Scope::ControlScope |
-  (FnCatch ? Scope::FnTryCatchScope : 0));
+  unsigned ScopeFlags = Scope::DeclScope | Scope::ControlScope |
+Scope::CatchScope |
+(FnCatch ? Scope::FnTryCatchScope : 0);
+  ParseScope CatchScope(this, ScopeFlags);
 
   // exception-declaration is equivalent to '...' or a parameter-declaration
   // without default arguments.
@@ -2290,7 +2292,7 @@ StmtResult Parser::ParseCXXCatchBlock(bo
 return StmtError(Diag(Tok, diag::err_expected) << tok::l_brace);
 
   // FIXME: Possible draft standard bug: attribute-specifier should be allowed?
-  StmtResult Block(ParseCompoundStatement());
+  StmtResult Block(ParseCompoundStatement(/*isStmtExpr=*/false, ScopeFlags));
   if (Block.isInvalid())
 return Block;
 

Modified: cfe/trunk/lib/Sema/Scope.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Scope.cpp?rev=356296&r1=356295&r2=356296&view=diff
==
--- cfe/trunk/lib/Sema/Scope.cpp (original)
+++ cfe/trunk/lib/Sema/Scope.cpp Fri Mar 15 13:25:49 2019
@@ -166,7 +166,9 @@ void Scope::dumpImpl(raw_ostream &OS) co
   {SEHExceptScope, "SEHExceptScope"},
   {SEHFilterScope, "SEHFilterScope"},
   {CompoundStmtScope, "CompoundStmtScope"},
-  {ClassInheritanceScope, "ClassInheritanceScope"}};
+  {ClassInheritanceScope, "ClassInheritanceScope"},
+  {CatchScope, "CatchScope"},
+  };
 
   for (auto Info : FlagInfo) {
 if (Flags & Info.first) {

Modified: cfe/trunk/lib/Sema/SemaCoroutine.cpp
URL: 
http://llvm.org/viewvc/llvm

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356296: [coroutines][PR40978] Emit error for co_yield within 
catch block (authored by modocache, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59076?vs=190740&id=190890#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59076

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Scope.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/Scope.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: lib/Sema/Scope.cpp
===
--- lib/Sema/Scope.cpp
+++ lib/Sema/Scope.cpp
@@ -166,7 +166,9 @@
   {SEHExceptScope, "SEHExceptScope"},
   {SEHFilterScope, "SEHFilterScope"},
   {CompoundStmtScope, "CompoundStmtScope"},
-  {ClassInheritanceScope, "ClassInheritanceScope"}};
+  {ClassInheritanceScope, "ClassInheritanceScope"},
+  {CatchScope, "CatchScope"},
+  };
 
   for (auto Info : FlagInfo) {
 if (Flags & Info.first) {
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -185,21 +185,8 @@
 
 static bool isValidCoroutineContext(Sema &S, SourceLocation Loc,
 StringRef Keyword) {
-  // 'co_await' and 'co_yield' are not permitted in unevaluated operands,
-  // such as subexpressions of \c sizeof.
-  //
-  // [expr.await]p2, emphasis added: "An await-expression shall appear only in
-  // a *potentially evaluated* expression within the compound-statement of a
-  // function-body outside of a handler [...] A context within a function where
-  // an await-expression can appear is called a suspension context of the
-  // function." And per [expr.yield]p1: "A yield-expression shall appear only
-  // within a suspension context of a function."
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }
-
-  // Per [expr.await]p2, any other usage must be within a function.
+  // [expr.await]p2 dictates that 'co_await' and 'co_yield' must be used within
+  // a function body.
   // FIXME: This also covers [expr.await]p2: "An await-expression shall not
   // appear in a default argument." But the diagnostic QoI here could be
   // improved to inform the user that default arguments specifically are not
@@ -668,12 +655,57 @@
   return true;
 }
 
+// Recursively walks up the scope hierarchy until either a 'catch' or a function
+// scope is found, whichever comes first.
+static bool isWithinCatchScope(Scope *S) {
+  // 'co_await' and 'co_yield' keywords are disallowed within catch blocks, but
+  // lambdas that use 'co_await' are allowed. The loop below ends when a
+  // function scope is found in order to ensure the following behavior:
+  //
+  // void foo() {  // <- function scope
+  //   try {   //
+  // co_await x;   // <- 'co_await' is OK within a function scope
+  //   } catch {   // <- catch scope
+  // co_await x;   // <- 'co_await' is not OK within a catch scope
+  // []() {// <- function scope
+  //   co_await x; // <- 'co_await' is OK within a function scope
+  // }();
+  //   }
+  // }
+  while (S && !(S->getFlags() & Scope::FnScope)) {
+if (S->getFlags() & Scope::CatchScope)
+  return true;
+S = S->getParent();
+  }
+  return false;
+}
+
+// [expr.await]p2, emphasis added: "An await-expression shall appear only in
+// a *potentially evaluated* expression within the compound-statement of a
+// function-body *outside of a handler* [...] A context within a function
+// where an await-expression can appear is called a suspension context of the
+// function."
+static void checkSuspensionContext(Sema &S, SourceLocation Loc,
+   StringRef Keyword) {
+  // First emphasis of [expr.await]p2: must be a potentially evaluated context.
+  // That is, 'co_await' and 'co_yield' cannot appear in subexpressions of
+  // \c sizeof.
+  if (S.isUnevaluatedContext())
+S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
+
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (isWithinCatchScope(S.getCurScope()))
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
+}
+
 ExprResult Sema::ActOnCoawaitExpr(Scope *S, SourceLocation Loc, Expr *E) {
   if (!ActOnCoroutineBodyStart(S, Loc, "co_await")) {
 CorrectDelayedTyposInExpr(E);
 return ExprError();
   }
 
+  checkSuspensionContext(*this, Loc, "co_await");
+
   if (E->getType()->isPlaceholderType()) {
 ExprResult R = CheckPlaceholderExpr(E);
 if (R.isInvalid()) return ExprError();
@@ -771,6 +803,8 @@
 return ExprError();
   }
 
+  checkSuspensionContext(*this, Loc, "co_yield");
+
   // Build yield_value call.
   ExprRes

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Great, thanks for the reviews, everyone!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59076



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D58418#1430630 , @thakis wrote:

> In D58418#1430490 , @jkorous wrote:
>
> > In D58418#1430160 , @thakis wrote:
> >
> > > Why is this needed for index-while-building? My mental model for 
> > > index-while-building is that that streams out build index metadata as 
> > > part of the regular compile. Why does that require watching directories?
> >
> >
> > You're right that this isn't necessary for the indexing phase. But we also 
> > provide API so clients can consume the index. This functionality is used 
> > for getting notifications about index data changes.
> >
> > You can see it for example here:
> >  
> > https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111
>
>
> Is that code going to live in clang? This seems more like a tool built on top 
> of the compiler rather than something core to the compiler itself (like the 
> actual index-while-building feature). Maybe this could be in 
> clang-tools-extra?


It actually is part of the feature as the serialized format of the index isn't 
meant as a stable interface, that's what the API is for. DirectoryWatcher isn't 
a tool, it's just part of implementation of the IndexStore API.


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

https://reviews.llvm.org/D58418



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

[PATCH] D54978: Move the SMT API to LLVM

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho updated this revision to Diff 190893.
mikhail.ramalho added a comment.

Update Z3 script to use cmake's `try_run` in order to retrieve the version from 
the lib.


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

https://reviews.llvm.org/D54978

Files:
  clang/CMakeLists.txt
  clang/cmake/modules/FindZ3.cmake
  clang/include/clang/Config/config.h.cmake
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTAPI.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/SMTConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
  clang/test/CMakeLists.txt
  clang/test/lit.site.cfg.py.in
  llvm/CMakeLists.txt
  llvm/cmake/modules/FindZ3.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/SMTAPI.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Z3Solver.cpp

Index: llvm/lib/Support/Z3Solver.cpp
===
--- llvm/lib/Support/Z3Solver.cpp
+++ llvm/lib/Support/Z3Solver.cpp
@@ -1,4 +1,4 @@
-//== Z3ConstraintManager.cpp *- C++ -*--==//
+//== Z3Solver.cpp ---*- C++ -*--==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,18 +6,14 @@
 //
 //===--===//
 
-#include "clang/Basic/TargetInfo.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Config/config.h"
+#include "llvm/Support/SMTAPI.h"
+#include 
 
-#include "clang/Config/config.h"
+using namespace llvm;
 
-using namespace clang;
-using namespace ento;
-
-#if CLANG_ANALYZER_WITH_Z3
+#if LLVM_WITH_Z3
 
 #include 
 
@@ -818,18 +814,13 @@
 
 #endif
 
-SMTSolverRef clang::ento::CreateZ3Solver() {
-#if CLANG_ANALYZER_WITH_Z3
+llvm::SMTSolverRef llvm::CreateZ3Solver() {
+#if LLVM_WITH_Z3
   return llvm::make_unique();
 #else
   llvm::report_fatal_error("Clang was not compiled with Z3 support, rebuild "
-   "with -DCLANG_ANALYZER_ENABLE_Z3_SOLVER=ON",
+   "with -DLLVM_ENABLE_Z3_SOLVER=ON",
false);
   return nullptr;
 #endif
 }
-
-std::unique_ptr
-ento::CreateZ3ConstraintManager(ProgramStateManager &StMgr, SubEngine *Eng) {
-  return llvm::make_unique(Eng, StMgr.getSValBuilder());
-}
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -44,6 +44,13 @@
   set (delayload_flags delayimp -delayload:shell32.dll -delayload:ole32.dll)
 endif()
 
+# Link Z3 if the user wants to build it.
+if(LLVM_WITH_Z3)
+  set(Z3_LINK_FILES ${Z3_LIBRARIES})
+else()
+  set(Z3_LINK_FILES "")
+endif()
+
 add_llvm_library(LLVMSupport
   AArch64TargetParser.cpp
   ARMTargetParser.cpp
@@ -152,6 +159,7 @@
   regfree.c
   regstrlcpy.c
   xxhash.cpp
+  Z3Solver.cpp
 
 # System
   Atomic.cpp
@@ -177,7 +185,14 @@
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/ADT
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Support
   ${Backtrace_INCLUDE_DIRS}
-  LINK_LIBS ${system_libs} ${delayload_flags}
+  LINK_LIBS ${system_libs} ${delayload_flags} ${Z3_LINK_FILES}
   )
 
 set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${system_libs}")
+
+if(LLVM_WITH_Z3)
+  target_include_directories(LLVMSupport SYSTEM
+PRIVATE
+${Z3_INCLUDE_DIR}
+)
+endif()
Index: llvm/include/llvm/Support/SMTAPI.h
===
--- llvm/include/llvm/Support/SMTAPI.h
+++ llvm/include/llvm/Support/SMTAPI.h
@@ -11,15 +11,16 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SMTSOLVER_H
-#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SMTSOLVER_H
+#ifndef LLVM_SUPPORT_SMTAPI_H
+#define LLVM_SUPPORT_SMTAPI_H
 
-#include "clang/Basic/TargetInfo.h"
+#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
 
-namespace clang {
-namespace ento {
+namespace llvm {
 
 /// Generic base class for SMT sorts
 class SMTSort {
@@ -399,7 +400,6 @@
 /// Convenience method to create and Z3Solver object
 SMTSolverRef CreateZ3Solver();
 
-} // namespace ento
-} // namespace clang
+} // namespace llvm
 
 #endif
Index: llvm/includ

[PATCH] D54978: Move the SMT API to LLVM

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho added a comment.

Hi all,

Sorry for the massive delay, but I just updated the `FindZ3` script to retrieve 
the version from the lib. I changed it to use `try_run` instead of 
`try_compile` so we can get the version number.

I tried to use @brzycki code to get the version from the header, however, it's 
not working for Z3 4.8.4. In Z3 4.8.3 the FULL_VERSION is a nice `"Z3 4.8.3.0"` 
but in version 4.8.4 it's `"Z3 4.8.4.10272 d6df51951f4c master z3-4.8.4"` and 
cmake fails with the following message:

  -- Could NOT find Z3: Found unsuitable version "#define Z3_FULL_VERSION
"Z3 4.8.4.10272 d6df51951f4c master z3-4.8.4"", but required is at least 
"4.7.1" (found /home/mgadelha/z3/bin/libz3.so)

I believe we have a few options here:

1. Update the regex to handle the new format for `Z3_FULL_VERSION`.
2. Instead of parsing `Z3_FULL_VERSION`, we can parse `Z3_MAJOR_VERSION`, 
`Z3_MINOR_VERSION` and `Z3_BUILD_NUMBER` which are also available in the same 
header.

But, we'll still face the same problem of them changing the string format and 
breaking the scripts.


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

https://reviews.llvm.org/D54978



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

[PATCH] D59196: [NFC][clang][PCH] ASTStmtReader::VisitStmt(): fixup faulty assert.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a reviewer: dexonsmith.
lebedev.ri added a comment.

In D59196#1429393 , @riccibruno wrote:

> I am not an expert in the serialization code (just did some modifications), 
> but this seems reasonable to me.


That's my thoughts too..
Unless of course it is also testing that `NumStmtFields` is zero.
Anyone with more knowledged opinion?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59196



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

[PATCH] D59316: [HIP-Clang] propagate -mllvm options to opt and llc

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D59316#1431302 , @arsenm wrote:

>




> ML workloads are extremely unlikely to use a call. We should have an 
> execution tests with noinline somewhere to stress this

I compiled and ran a test with noinline function and I saw function call in ISA 
and it passes. I agree that we should have such a test and will keep it in mind.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59316



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