[PATCH] D53482: Add clang-format stability check with FormatTests

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

As @krasimir said we wouldn't normally commit tests which just break as they'd 
fail on the buildbot and someone would revert your change within 24 hours.. 
however, what you have here raises an interesting conversation

This bug of instability is often raised, I've even seen it myself with my own 
code and I normally see new bugs coming in about this on a regular basis

https://bugs.llvm.org/show_bug.cgi?id=43845
https://bugs.llvm.org/show_bug.cgi?id=42509

Firstly those developers who have clang-formated integrated into say 
vim,emac,vs,vscode with format on save likely never see this because they often 
format multiple times during their development and its likely those 
instabilities iron themselves out pretty quickly, but this does show up for 
those people who perhaps only do a final git clang-format before committing or 
who are reformatting a largely unformatted code base, then the CI system likely 
complains on commit if you have a check.

It would be good to investigate the root causes, but my experience seems to be 
that it often feels related to the alignment rules and the positioning of 
trailing comments. (at least that my experience)

It would also be good to understand if this is always solved with a second run 
or does it sometimes require more than two runs, or has anyone seen a case 
where the format flip-flops between 2 styles.

Ultimately fixing the alignment routines to try and make them completely stable 
with regard to future downstream rules, I believe would simply make them 
individually more complex and unstable if new rules were added.

It might be worth simply rerunning the specific alignXXX routine again on the 
subsequent changes.

I'd also be interested to understand if any real thought was put into the order 
of these routines? Or if just conceptually if you are trying to align 
declarations, assignments and trailing comments something has to go first and 
you don't know where the best place is until someone takes a first stab.

  llvm::sort(Changes, Change::IsBeforeInFile(SourceMgr));
  calculateLineBreakInformation();
  alignConsecutiveMacros();
  alignConsecutiveDeclarations();
  alignConsecutiveAssignments();
  alignTrailingComments();
  alignEscapedNewlines();
  generateChanges();

I'd would like to think there was a more elegant approach than just running 
clang-format twice! one solution might be to simply rerun the whole of 
reformat() function more than once. Of course, we have to keep one eye on 
performance because for large files clang-format can already be a little slow.

But running reformat() wouldn't be 2x because we definitely don't need to rerun 
the sort includes twice and we don't need to reannotate the tokens, detemine 
the spaces between or  apply the replacements twice (+ no startup costs, dll 
loading, code rewriting), so I actually think a second pass of reformat might 
not be terrible. (maybe undesirable but not terrible)

What if we experiment with a `-stable` command line switch which simply reruns 
those parts of reformat that are causing the issues? and then measure the 
impact to performance?


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

https://reviews.llvm.org/D53482



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

[PATCH] D69577: [clang-format] [PR35518] C++17 deduction guides are wrongly formatted

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:4995
+  verifyFormat("c()->f();");
+  verifyFormat("x()->foo<1>;");
+  verifyFormat("x = p->foo<3>();");

MyDeveloperDay wrote:
> curdeius wrote:
> > What about:
> > 
> > ```
> > verifyFormat("x()->x<1>;");
> > ```
> > i.e. a function `x` returning a pointer to a class having a template member 
> > `x` (for instance a template variable).
> I'm unsure if we are the limit of being able to differentiate between the two?
Well, deduction guides always have a template parameter list before class name 
(unless I'm mistaken), so we should be able to distinguish the two.


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

https://reviews.llvm.org/D69577



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

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added inline comments.



Comment at: clang/test/Analysis/check_analyzer_fixit.py:1
+#!/usr/bin/env python
+#

This does work with python3?


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

https://reviews.llvm.org/D69746



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

[clang] 0d14656 - [mips] Set __OCTEON__ macros

[Simon Atanasyan via cfe-commits]

Author: Simon Atanasyan
Date: 2019-11-05T12:10:58+03:00
New Revision: 0d14656b9d8ca38b8ea321c7047eaeec43c5b2ef

URL: 
https://github.com/llvm/llvm-project/commit/0d14656b9d8ca38b8ea321c7047eaeec43c5b2ef
DIFF: 
https://github.com/llvm/llvm-project/commit/0d14656b9d8ca38b8ea321c7047eaeec43c5b2ef.diff

LOG: [mips] Set __OCTEON__ macros

Added: 


Modified: 
clang/lib/Basic/Targets/Mips.cpp
clang/test/Preprocessor/init.c

Removed: 




diff  --git a/clang/lib/Basic/Targets/Mips.cpp 
b/clang/lib/Basic/Targets/Mips.cpp
index 4ca7f08af823..9b35bbcb7638 100644
--- a/clang/lib/Basic/Targets/Mips.cpp
+++ b/clang/lib/Basic/Targets/Mips.cpp
@@ -189,6 +189,9 @@ void MipsTargetInfo::getTargetDefines(const LangOptions 
&Opts,
   Builder.defineMacro("_MIPS_ARCH", "\"" + CPU + "\"");
   Builder.defineMacro("_MIPS_ARCH_" + StringRef(CPU).upper());
 
+  if (StringRef(CPU).startswith("octeon"))
+Builder.defineMacro("__OCTEON__");
+
   // These shouldn't be defined for MIPS-I but there's no need to check
   // for that since MIPS-I isn't supported.
   Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");

diff  --git a/clang/test/Preprocessor/init.c b/clang/test/Preprocessor/init.c
index 4e79077687c7..f80e4de9e5eb 100644
--- a/clang/test/Preprocessor/init.c
+++ b/clang/test/Preprocessor/init.c
@@ -4839,6 +4839,7 @@
 // MIPS-ARCH-OCTEON:#define _MIPS_ARCH "octeon"
 // MIPS-ARCH-OCTEON:#define _MIPS_ARCH_OCTEON 1
 // MIPS-ARCH-OCTEON:#define _MIPS_ISA _MIPS_ISA_MIPS64
+// MIPS-ARCH-OCTEON:#define __OCTEON__ 1
 // MIPS-ARCH-OCTEON:#define __mips_isa_rev 2
 //
 // Check MIPS float ABI macros



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

[clang] e578d0f - [mips] Fix `__mips_isa_rev` macros value for Octeon CPU

[Simon Atanasyan via cfe-commits]

Author: Simon Atanasyan
Date: 2019-11-05T12:10:58+03:00
New Revision: e578d0fd295a67bce1e1fc922237f459deb49c7e

URL: 
https://github.com/llvm/llvm-project/commit/e578d0fd295a67bce1e1fc922237f459deb49c7e
DIFF: 
https://github.com/llvm/llvm-project/commit/e578d0fd295a67bce1e1fc922237f459deb49c7e.diff

LOG: [mips] Fix `__mips_isa_rev` macros value for Octeon CPU

Added: 


Modified: 
clang/lib/Basic/Targets/Mips.cpp
clang/test/Preprocessor/init.c

Removed: 




diff  --git a/clang/lib/Basic/Targets/Mips.cpp 
b/clang/lib/Basic/Targets/Mips.cpp
index 2cafbe87a996..4ca7f08af823 100644
--- a/clang/lib/Basic/Targets/Mips.cpp
+++ b/clang/lib/Basic/Targets/Mips.cpp
@@ -61,7 +61,7 @@ void MipsTargetInfo::fillValidCPUList(
 unsigned MipsTargetInfo::getISARev() const {
   return llvm::StringSwitch(getCPU())
  .Cases("mips32", "mips64", 1)
- .Cases("mips32r2", "mips64r2", 2)
+ .Cases("mips32r2", "mips64r2", "octeon", 2)
  .Cases("mips32r3", "mips64r3", 3)
  .Cases("mips32r5", "mips64r5", 5)
  .Cases("mips32r6", "mips64r6", 6)

diff  --git a/clang/test/Preprocessor/init.c b/clang/test/Preprocessor/init.c
index 18972de35348..4e79077687c7 100644
--- a/clang/test/Preprocessor/init.c
+++ b/clang/test/Preprocessor/init.c
@@ -4832,6 +4832,15 @@
 // MIPS-ARCH-64R6:#define _MIPS_ISA _MIPS_ISA_MIPS64
 // MIPS-ARCH-64R6:#define __mips_isa_rev 6
 //
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=mips64-none-none \
+// RUN:-target-cpu octeon < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS-ARCH-OCTEON %s
+//
+// MIPS-ARCH-OCTEON:#define _MIPS_ARCH "octeon"
+// MIPS-ARCH-OCTEON:#define _MIPS_ARCH_OCTEON 1
+// MIPS-ARCH-OCTEON:#define _MIPS_ISA _MIPS_ISA_MIPS64
+// MIPS-ARCH-OCTEON:#define __mips_isa_rev 2
+//
 // Check MIPS float ABI macros
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding \



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

[PATCH] D69791: [ARM,MVE] Add intrinsics for gather/scatter load/stores.

[Simon Tatham via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/arm_mve_defs.td:175
 
+// CopyKind expects t and u to be scalars. It returns a scalar
+// whose kind (signed, unsigned or float) matches that of k, and whose

dmgreen wrote:
> Should the t and u be s and k?
Note to self: never change your mind about the variable names half way through 
writing a comment...



Comment at: clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple thumbv8.1m.main-arm-none-eabi -target-feature 
+mve.fp -mfloat-abi hard -O0 -disable-O0-optnone -S -emit-llvm -o - %s | opt -S 
-mem2reg | FileCheck %s
+

dmgreen wrote:
> -DPOLYMORPHIC line
D'oh, that's what I get for copy-pasting the header from the one test file that 
//doesn't// have that extra line.



Comment at: llvm/include/llvm/IR/IntrinsicsARM.td:815
 
-def int_arm_mve_vcvt_narrow: Intrinsic<[llvm_v8f16_ty],
-   [llvm_v8f16_ty, llvm_v4f32_ty, llvm_i32_ty], [IntrNoMem]>;
-def int_arm_mve_vcvt_narrow_predicated: Intrinsic<[llvm_v8f16_ty],
-   [llvm_v8f16_ty, llvm_v4f32_ty, llvm_i32_ty, llvm_v4i1_ty], [IntrNoMem]>;
+multiclass MVEPredicated rets, list params,
+ LLVMType pred, list props = []> {

dmgreen wrote:
> This looks useful.
Eventually we'll probably need another one alongside it, for the many 
intrinsics that add an `inactive` parameter as well as a predicate mask. But 
for the moment I'm just adding what I need to use right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69791



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

We hit this in V8 which is annotating some pointers as being 4GB-aligned. Does 
anyone know what it would take to raise Clang's limit here?


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

https://reviews.llvm.org/D68824



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

[PATCH] D69662: [Checkers] Avoid using evalCall in StreamChecker.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

So we can have only one checker to model `fread` with `eval::Call`. Why is 
`fread` now modeled by StreamChecker and StdLibraryFunctionsChecker (both use 
`eval::Call`)? These checkers look at the function from different aspects, I do 
not like to have one checker with both checks.

Another question: I want to extend the StreamChecker with function `freopen` 
that returns the passed file descriptor. We should indicate that the return 
value is the same as argument 2 (except if arg 2 is NULL if I understand 
correctly the man page). And this function can fail and return NULL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69662



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

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

[Erich Keane via Phabricator via cfe-commits]

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

In D68824#1733621 , @hans wrote:

> We hit this in V8 which is annotating some pointers as being 4GB-aligned. 
> Does anyone know what it would take to raise Clang's limit here?


The issue is LLVM's limit here. I'm unaware of the reasoning for this limit in 
LLVM, however the clang limit is based on LLVMs.  I'd suggest starting a 
conversation on llvm-dev mailing list to have the discussion of why this limit 
exists, and whether it is modifiable.


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

https://reviews.llvm.org/D68824



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

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59845 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69673



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

[PATCH] D64319: [OpenCL] Add function attributes handling for builtin functions

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a8d477a0e00: [OpenCL] Add builtin function attribute 
handling (authored by svenvh).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64319

Files:
  clang/lib/Sema/OpenCLBuiltins.td
  clang/lib/Sema/SemaLookup.cpp
  clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -271,6 +271,12 @@
   // the SignatureTable represent the complete signature.  The first type at
   // index SigTableIndex is the return type.
   const unsigned NumTypes;
+  // Function attribute __attribute__((pure))
+  const bool IsPure;
+  // Function attribute __attribute__((const))
+  const bool IsConst;
+  // Function attribute __attribute__((convergent))
+  const bool IsConv;
   // First OpenCL version in which this overload was introduced (e.g. CL20).
   const unsigned short MinVersion;
   // First OpenCL version in which this overload was removed (e.g. CL20).
@@ -409,6 +415,9 @@
 for (const auto &Overload : FOM.second) {
   OS << "  { " << Overload.second << ", "
  << Overload.first->getValueAsListOfDefs("Signature").size() << ", "
+ << (Overload.first->getValueAsBit("IsPure")) << ", "
+ << (Overload.first->getValueAsBit("IsConst")) << ", "
+ << (Overload.first->getValueAsBit("IsConv")) << ", "
  << Overload.first->getValueAsDef("MinVersion")->getValueAsInt("ID")
  << ", "
  << Overload.first->getValueAsDef("MaxVersion")->getValueAsInt("ID")
Index: clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -emit-llvm -o - -O0 -triple spir-unknown-unknown -fdeclare-opencl-builtins -finclude-default-header %s | FileCheck %s
+
+// Test that Attr.Const from OpenCLBuiltins.td is lowered to a readnone attribute.
+// CHECK-LABEL: @test_const_attr
+// CHECK: call i32 @_Z3maxii({{.*}}) [[ATTR_CONST:#[0-9]]]
+// CHECK: ret
+int test_const_attr(int a) {
+  return max(a, 2);
+}
+
+// Test that Attr.Pure from OpenCLBuiltins.td is lowered to a readonly attribute.
+// CHECK-LABEL: @test_pure_attr
+// CHECK: call <4 x float> @_Z11read_imagef{{.*}} [[ATTR_PURE:#[0-9]]]
+// CHECK: ret
+kernel void test_pure_attr(read_only image1d_t img) {
+  float4 resf = read_imagef(img, 42);
+}
+
+// CHECK: attributes [[ATTR_CONST]] =
+// CHECK-SAME: readnone
+// CHECK: attributes [[ATTR_PURE]] =
+// CHECK-SAME: readonly
Index: clang/lib/Sema/SemaLookup.cpp
===
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -815,9 +815,17 @@
 }
 NewOpenCLBuiltin->setParams(ParmList);
   }
-  if (!S.getLangOpts().OpenCLCPlusPlus) {
+
+  // Add function attributes.
+  if (OpenCLBuiltin.IsPure)
+NewOpenCLBuiltin->addAttr(PureAttr::CreateImplicit(Context));
+  if (OpenCLBuiltin.IsConst)
+NewOpenCLBuiltin->addAttr(ConstAttr::CreateImplicit(Context));
+  if (OpenCLBuiltin.IsConv)
+NewOpenCLBuiltin->addAttr(ConvergentAttr::CreateImplicit(Context));
+  if ((GenTypeMaxCnt > 1 || Len > 1) && !S.getLangOpts().OpenCLCPlusPlus)
 NewOpenCLBuiltin->addAttr(OverloadableAttr::CreateImplicit(Context));
-  }
+
   LR.addDecl(NewOpenCLBuiltin);
 }
   }
Index: clang/lib/Sema/OpenCLBuiltins.td
===
--- clang/lib/Sema/OpenCLBuiltins.td
+++ clang/lib/Sema/OpenCLBuiltins.td
@@ -180,10 +180,18 @@
   let VecWidth = 0;
 }
 
+// Builtin function attributes.
+def Attr {
+  list None = [0, 0, 0];
+  list Pure = [1, 0, 0];
+  list Const = [0, 1, 0];
+  list Convergent = [0, 0, 1];
+}
+
 //===--===//
 //  OpenCL C class for builtin functions
 //===--===//
-class Builtin _Signature> {
+class Builtin _Signature, list _Attributes = Attr.None> {
   // Name of the builtin function
   string Name = _Name;
   // List of types used by the function. The first one is the return type and
@@ -192,6 +200,12 @@
   list Signature = _Signature;
   // OpenCL Extension to which the function belongs (cl_khr_subgroups, ...)
   string Extension = "";
+  // Function attribute __attribute__((pure))
+  bit IsPure = _Attributes[0];
+  // Function attribute __attribute__((const))
+  bit IsConst = _Attributes[1];
+  // Function attribute __attribute__((convergent))

[PATCH] D63557: [OpenCL] Group builtin functions by prototype

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e56b0f94bfc: [OpenCL] Group builtin functions by prototype 
(authored by svenvh).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63557?vs=222446&id=227833#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63557

Files:
  clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Index: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
===
--- clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -69,6 +69,13 @@
 using namespace llvm;
 
 namespace {
+
+// A list of signatures that are shared by one or more builtin functions.
+struct BuiltinTableEntries {
+  SmallVector Names;
+  std::vector> Signatures;
+};
+
 class BuiltinNameEmitter {
 public:
   BuiltinNameEmitter(RecordKeeper &Records, raw_ostream &OS)
@@ -79,6 +86,9 @@
   void Emit();
 
 private:
+  // A list of indices into the builtin function table.
+  using BuiltinIndexListTy = SmallVector;
+
   // Contains OpenCL builtin functions and related information, stored as
   // Record instances. They are coming from the associated TableGen file.
   RecordKeeper &Records;
@@ -106,6 +116,23 @@
   // FctOverloadMap and TypeMap.
   void GetOverloads();
 
+  // Compare two lists of signatures and check that e.g. the OpenCL version,
+  // function attributes, and extension are equal for each signature.
+  // \param Candidate (in) Entry in the SignatureListMap to check.
+  // \param SignatureList (in) List of signatures of the considered function.
+  // \returns true if the two lists of signatures are identical.
+  bool CanReuseSignature(
+  BuiltinIndexListTy *Candidate,
+  std::vector> &SignatureList);
+
+  // Group functions with the same list of signatures by populating the
+  // SignatureListMap.
+  // Some builtin functions have the same list of signatures, for example the
+  // "sin" and "cos" functions. To save space in the BuiltinTable, the
+  // "isOpenCLBuiltin" function will have the same output for these two
+  // function names.
+  void GroupBySignature();
+
   // Emit the TypeTable containing all types used by OpenCL builtins.
   void EmitTypeTable();
 
@@ -170,6 +197,24 @@
 
   // Same as TypeList, but for generic types only.
   std::vector GenTypeList;
+
+  // Map an ordered vector of signatures to their original Record instances,
+  // and to a list of function names that share these signatures.
+  //
+  // For example, suppose the "cos" and "sin" functions have only three
+  // signatures, and these signatures are at index Ix in the SignatureTable:
+  //  cos | sin |  Signature| Index
+  //  float   cos(float)  | float   sin(float)  |  Signature1   | I1
+  //  double  cos(double) | double  sin(double) |  Signature2   | I2
+  //  halfcos(half)   | halfsin(half)   |  Signature3   | I3
+  //
+  // Then we will create a mapping of the vector of signatures:
+  // SignatureListMap[] = <
+  //  <"cos", "sin">,
+  //  >
+  // The function "tan", having the same signatures, would be mapped to the
+  // same entry ().
+  MapVector SignatureListMap;
 };
 } // namespace
 
@@ -183,6 +228,7 @@
   EmitDeclarations();
 
   GetOverloads();
+  GroupBySignature();
 
   // Emit tables.
   EmitTypeTable();
@@ -408,11 +454,15 @@
   unsigned Index = 0;
 
   OS << "static const OpenCLBuiltinStruct BuiltinTable[] = {\n";
-  for (const auto &FOM : FctOverloadMap) {
+  for (const auto &SLM : SignatureListMap) {
 
-OS << "  // " << (Index + 1) << ": " << FOM.first << "\n";
+OS << "  // " << (Index + 1) << ": ";
+for (const auto &Name : SLM.second.Names) {
+  OS << Name << ", ";
+}
+OS << "\n";
 
-for (const auto &Overload : FOM.second) {
+for (const auto &Overload : SLM.second.Signatures) {
   OS << "  { " << Overload.second << ", "
  << Overload.first->getValueAsListOfDefs("Signature").size() << ", "
  << (Overload.first->getValueAsBit("IsPure")) << ", "
@@ -428,19 +478,92 @@
   OS << "};\n\n";
 }
 
+bool BuiltinNameEmitter::CanReuseSignature(
+BuiltinIndexListTy *Candidate,
+std::vector> &SignatureList) {
+  assert(Candidate->size() == SignatureList.size() &&
+ "signature lists should have the same size");
+
+  auto &CandidateSigs =
+  SignatureListMap.find(Candidate)->second.Signatures;
+  for (unsigned Index = 0; Index < Candidate->size(); Index++) {
+const Record *Rec = SignatureList[Index].first;
+const Record *Rec2 = CandidateSigs[Index].first;
+if (Rec->getValueAsBit("IsPure") == Rec2->getValueAsBit("IsPure") &&
+Rec->getValueAsBit("IsConst") == Rec2->getValueAsBit("IsConst") &&
+Rec->getValueAsBit("IsConv") == Rec2->getValueAsBit("IsConv") &&
+   

[clang] 9a8d477 - [OpenCL] Add builtin function attribute handling

[Sven van Haastregt via cfe-commits]

Author: Sven van Haastregt
Date: 2019-11-05T10:26:47Z
New Revision: 9a8d477a0e00c15d6d33a52486fa931483b7f2ea

URL: 
https://github.com/llvm/llvm-project/commit/9a8d477a0e00c15d6d33a52486fa931483b7f2ea
DIFF: 
https://github.com/llvm/llvm-project/commit/9a8d477a0e00c15d6d33a52486fa931483b7f2ea.diff

LOG: [OpenCL] Add builtin function attribute handling

Add handling for the "pure", "const" and "convergent" function
attributes for OpenCL builtin functions.

Patch by Pierre Gondois and Sven van Haastregt.

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

Added: 
clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl

Modified: 
clang/lib/Sema/OpenCLBuiltins.td
clang/lib/Sema/SemaLookup.cpp
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Removed: 




diff  --git a/clang/lib/Sema/OpenCLBuiltins.td 
b/clang/lib/Sema/OpenCLBuiltins.td
index 298614059467..4f458652ff73 100644
--- a/clang/lib/Sema/OpenCLBuiltins.td
+++ b/clang/lib/Sema/OpenCLBuiltins.td
@@ -180,10 +180,18 @@ class GenericType :
   let VecWidth = 0;
 }
 
+// Builtin function attributes.
+def Attr {
+  list None = [0, 0, 0];
+  list Pure = [1, 0, 0];
+  list Const = [0, 1, 0];
+  list Convergent = [0, 0, 1];
+}
+
 
//===--===//
 //  OpenCL C class for builtin functions
 
//===--===//
-class Builtin _Signature> {
+class Builtin _Signature, list _Attributes = 
Attr.None> {
   // Name of the builtin function
   string Name = _Name;
   // List of types used by the function. The first one is the return type and
@@ -192,6 +200,12 @@ class Builtin _Signature> {
   list Signature = _Signature;
   // OpenCL Extension to which the function belongs (cl_khr_subgroups, ...)
   string Extension = "";
+  // Function attribute __attribute__((pure))
+  bit IsPure = _Attributes[0];
+  // Function attribute __attribute__((const))
+  bit IsConst = _Attributes[1];
+  // Function attribute __attribute__((convergent))
+  bit IsConv = _Attributes[2];
   // Version of OpenCL from which the function is available (e.g.: CL10).
   // MinVersion is inclusive.
   Version MinVersion = CL10;
@@ -307,11 +321,12 @@ foreach RType = [Float, Double, Half, Char, UChar, Short,
UShort, Int, UInt, Long, ULong] in {
 foreach sat = ["", "_sat"] in {
   foreach rnd = ["", "_rte", "_rtn", "_rtp", "_rtz"] in {
-def : Builtin<"convert_" # RType.Name # sat # rnd, [RType, IType]>;
+def : Builtin<"convert_" # RType.Name # sat # rnd, [RType, IType],
+  Attr.Const>;
 foreach v = [2, 3, 4, 8, 16] in {
   def : Builtin<"convert_" # RType.Name # v # sat # rnd,
-[VectorType,
- VectorType]>;
+[VectorType, VectorType],
+Attr.Const>;
 }
   }
 }
@@ -321,11 +336,11 @@ foreach RType = [Float, Double, Half, Char, UChar, Short,
 //
 // OpenCL v1.1 s6.11.1, v1.2 s6.12.1, v2.0 s6.13.1 - Work-item Functions
 // --- Table 7 ---
-def : Builtin<"get_work_dim", [UInt]>;
+def : Builtin<"get_work_dim", [UInt], Attr.Const>;
 foreach name = ["get_global_size", "get_global_id", "get_local_size",
 "get_local_id", "get_num_groups", "get_group_id",
 "get_global_offset"] in {
-  def : Builtin;
+  def : Builtin;
 }
 
 let MinVersion = CL20 in {
@@ -491,24 +506,24 @@ foreach Type = [Int, UInt] in {
 foreach name = ["acos", "acosh", "acospi",
 "asin", "asinh", "asinpi",
 "atan", "atanh", "atanpi"] in {
-  def : Builtin;
+  def : Builtin;
 }
 
 foreach name = ["atan2", "atan2pi"] in {
-  def : Builtin;
+  def : Builtin;
 }
 
 foreach name = ["fmax", "fmin"] in {
   def : Builtin;
-  def : Builtin;
-  def : Builtin;
-  def : Builtin;
+  def : Builtin;
+  def : Builtin;
+  def : Builtin;
 }
 
 // OpenCL v1.1 s6.11.3, v1.2 s6.12.3, v2.0 s6.13.3 - Integer Functions
 foreach name = ["max", "min"] in {
-  def : Builtin;
-  def : Builtin;
+  def : Builtin;
+  def : Builtin;
 }
 
 //
@@ -517,49 +532,49 @@ foreach name = ["max", "min"] in {
 // --- Table 22: Image Read Functions with Samplers ---
 foreach imgTy = [Image1d] in {
   foreach coordTy = [Int, Float] in {
-def : Builtin<"read_imagef", [VectorType, ImageType, Sampler, coordTy]>;
-def : Builtin<"read_imagei", [VectorType, ImageType, 
Sampler, coordTy]>;
-def : Builtin<"read_imageui", [VectorType, ImageType, Sampler, coordTy]>;
+def : Builtin<"read_imagef", [VectorType, ImageType, Sampler, coordTy], Attr.Pure>;
+def : Builtin<"read_imagei", [VectorType, ImageType, 
Sampler, coordTy], Attr.Pure>;
+def : Builtin<"read

[clang] 0e56b0f - [OpenCL] Group builtin functions by prototype

[Sven van Haastregt via cfe-commits]

Author: Sven van Haastregt
Date: 2019-11-05T10:26:47Z
New Revision: 0e56b0f94bfc683c5a95e96784cfc9229a730bc8

URL: 
https://github.com/llvm/llvm-project/commit/0e56b0f94bfc683c5a95e96784cfc9229a730bc8
DIFF: 
https://github.com/llvm/llvm-project/commit/0e56b0f94bfc683c5a95e96784cfc9229a730bc8.diff

LOG: [OpenCL] Group builtin functions by prototype

The TableGen-generated file containing the function definitions can be
reorganized to save some memory in the Clang binary.  Functions having
the same prototype(s) will point to a shared list of prototype(s).

Patch by Pierre Gondois and Sven van Haastregt.

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

Added: 


Modified: 
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp

Removed: 




diff  --git a/clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp 
b/clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
index 983e267ce9ac..4042c17c5296 100644
--- a/clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
+++ b/clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
@@ -69,6 +69,13 @@
 using namespace llvm;
 
 namespace {
+
+// A list of signatures that are shared by one or more builtin functions.
+struct BuiltinTableEntries {
+  SmallVector Names;
+  std::vector> Signatures;
+};
+
 class BuiltinNameEmitter {
 public:
   BuiltinNameEmitter(RecordKeeper &Records, raw_ostream &OS)
@@ -79,6 +86,9 @@ class BuiltinNameEmitter {
   void Emit();
 
 private:
+  // A list of indices into the builtin function table.
+  using BuiltinIndexListTy = SmallVector;
+
   // Contains OpenCL builtin functions and related information, stored as
   // Record instances. They are coming from the associated TableGen file.
   RecordKeeper &Records;
@@ -106,6 +116,23 @@ class BuiltinNameEmitter {
   // FctOverloadMap and TypeMap.
   void GetOverloads();
 
+  // Compare two lists of signatures and check that e.g. the OpenCL version,
+  // function attributes, and extension are equal for each signature.
+  // \param Candidate (in) Entry in the SignatureListMap to check.
+  // \param SignatureList (in) List of signatures of the considered function.
+  // \returns true if the two lists of signatures are identical.
+  bool CanReuseSignature(
+  BuiltinIndexListTy *Candidate,
+  std::vector> &SignatureList);
+
+  // Group functions with the same list of signatures by populating the
+  // SignatureListMap.
+  // Some builtin functions have the same list of signatures, for example the
+  // "sin" and "cos" functions. To save space in the BuiltinTable, the
+  // "isOpenCLBuiltin" function will have the same output for these two
+  // function names.
+  void GroupBySignature();
+
   // Emit the TypeTable containing all types used by OpenCL builtins.
   void EmitTypeTable();
 
@@ -170,6 +197,24 @@ class BuiltinNameEmitter {
 
   // Same as TypeList, but for generic types only.
   std::vector GenTypeList;
+
+  // Map an ordered vector of signatures to their original Record instances,
+  // and to a list of function names that share these signatures.
+  //
+  // For example, suppose the "cos" and "sin" functions have only three
+  // signatures, and these signatures are at index Ix in the SignatureTable:
+  //  cos | sin |  Signature| Index
+  //  float   cos(float)  | float   sin(float)  |  Signature1   | I1
+  //  double  cos(double) | double  sin(double) |  Signature2   | I2
+  //  halfcos(half)   | halfsin(half)   |  Signature3   | I3
+  //
+  // Then we will create a mapping of the vector of signatures:
+  // SignatureListMap[] = <
+  //  <"cos", "sin">,
+  //  >
+  // The function "tan", having the same signatures, would be mapped to the
+  // same entry ().
+  MapVector SignatureListMap;
 };
 } // namespace
 
@@ -183,6 +228,7 @@ void BuiltinNameEmitter::Emit() {
   EmitDeclarations();
 
   GetOverloads();
+  GroupBySignature();
 
   // Emit tables.
   EmitTypeTable();
@@ -408,11 +454,15 @@ void BuiltinNameEmitter::EmitBuiltinTable() {
   unsigned Index = 0;
 
   OS << "static const OpenCLBuiltinStruct BuiltinTable[] = {\n";
-  for (const auto &FOM : FctOverloadMap) {
+  for (const auto &SLM : SignatureListMap) {
 
-OS << "  // " << (Index + 1) << ": " << FOM.first << "\n";
+OS << "  // " << (Index + 1) << ": ";
+for (const auto &Name : SLM.second.Names) {
+  OS << Name << ", ";
+}
+OS << "\n";
 
-for (const auto &Overload : FOM.second) {
+for (const auto &Overload : SLM.second.Signatures) {
   OS << "  { " << Overload.second << ", "
  << Overload.first->getValueAsListOfDefs("Signature").size() << ", "
  << (Overload.first->getValueAsBit("IsPure")) << ", "
@@ -428,19 +478,92 @@ void BuiltinNameEmitter::EmitBuiltinTable() {
   OS << "};\n\n";
 }
 
+bool BuiltinNameEmitter::CanReuseSignature(
+BuiltinIndexListTy *Candidate,
+std::vector> &SignatureList) {
+  assert

[PATCH] D67763: [Clang FE] Recognize -mnop-mcount CL option

[Jonas Paulsson via Phabricator via cfe-commits]

jonpa closed this revision.
jonpa added a comment.

committed as 9376714 



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

https://reviews.llvm.org/D67763



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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 227841.
Anastasia marked an inline comment as done.
Anastasia added a comment.

- Factored OpenCL diagnostics out in a separate helper function
- Removed special case for ParenType


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

https://reviews.llvm.org/D65744

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaOpenCL/event_t.cl
  clang/test/SemaOpenCL/invalid-block.cl
  clang/test/SemaOpenCL/invalid-pipes-cl2.0.cl
  clang/test/SemaOpenCL/sampler_t.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/addrspace-auto.cl
  clang/test/SemaOpenCLCXX/restricted.cl

Index: clang/test/SemaOpenCLCXX/restricted.cl
===
--- clang/test/SemaOpenCLCXX/restricted.cl
+++ clang/test/SemaOpenCLCXX/restricted.cl
@@ -32,12 +32,14 @@
 __constant _Thread_local int a = 1;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '_Thread_local' storage class specifier}}
 // expected-warning@-2 {{'_Thread_local' is a C11 extension}}
-
+// expected-error@-3 {{thread-local storage is not supported for the current target}}
 __constant __thread int b = 2;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '__thread' storage class specifier}}
+// expected-error@-2 {{thread-local storage is not supported for the current target}}
 kernel void test_storage_classes() {
   register int x;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'register' storage class specifier}}
   thread_local int y;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'thread_local' storage class specifier}}
+  // expected-error@-2 {{thread-local storage is not supported for the current target}}
 }
Index: clang/test/SemaOpenCLCXX/addrspace-auto.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/addrspace-auto.cl
@@ -0,0 +1,35 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+__constant int i = 1;
+//CHECK: |-VarDecl {{.*}} ai '__global int':'__global int'
+auto ai = i;
+
+kernel void test() {
+  int i;
+  //CHECK: VarDecl {{.*}} ai 'int':'int'
+  auto ai = i;
+
+  constexpr int c = 1;
+  //CHECK: VarDecl {{.*}} used cai '__constant int':'__constant int'
+  __constant auto cai = c;
+  //CHECK: VarDecl {{.*}} aii 'int':'int'
+  auto aii = cai;
+
+  //CHECK: VarDecl {{.*}} ref 'int &'
+  auto &ref = i;
+  //CHECK: VarDecl {{.*}} ptr 'int *'
+  auto *ptr = &i;
+  //CHECK: VarDecl {{.*}} ref_c '__constant int &'
+  auto &ref_c = cai;
+
+  //CHECK: VarDecl {{.*}} ptrptr 'int *__generic *'
+  auto **ptrptr = &ptr;
+  //CHECK: VarDecl {{.*}} refptr 'int *__generic &'
+  auto *&refptr = ptr;
+
+  //CHECK: VarDecl {{.*}} invalid gref '__global auto &'
+  __global auto &gref = i; //expected-error{{variable 'gref' with type '__global auto &' has incompatible initializer of type 'int'}}
+  __local int *ptr_l;
+  //CHECK: VarDecl {{.*}} invalid gptr '__global auto *'
+  __global auto *gptr = ptr_l; //expected-error{{variable 'gptr' with type '__global auto *' has incompatible initializer of type '__local int *'}}
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -65,30 +65,42 @@
 x3::x3(const x3 &t) {}
 
 template 
-T xxx(T *in) {
+T xxx(T *in1, T in2) {
   // This pointer can't be deduced to generic because addr space
   // will be taken from the template argument.
   //CHECK: `-VarDecl {{.*}} i 'T *' cinit
-  T *i = in;
+  T *i = in1;
   T ii;
+  __private T *ptr = ⅈ
+  ptr = &in2;
   return *i;
 }
 
 __kernel void test() {
   int foo[10];
-  xxx(&foo[0]);
+  xxx<__private int>(&foo[0], foo[0]);
+  // FIXME: Template param deduction fails here because
+  // temporaries are not in the __private address space.
+  // It is probably reasonable to put them in __private
+  // considering that stack and function params are
+  // implicitly in __private.
+  // However, if temporaries are left in default addr
+  // space we should at least pretty print the __private
+  // addr space. Otherwise diagnostic apprears to be
+  // confusing.
+  //xxx(&foo[0], foo[0]);
 }
 
 // Addr space for pointer/reference to an array
-//CHECK: FunctionDecl {{.*}} t1 'void (const __generic float (&)[2])'
+//CHECK: FunctionDecl {{.*}} t1 'void (const float (__generic &)[2])'
 void t1(const float (&fYZ)[2]);
-//CHECK: FunctionDecl {{.*}} t2 'void (const __generic float (*)[2])'
+//CHECK: FunctionDecl {{.*}} t2 'void (const float (__gener

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:6721
 
+  if (getLangOpts().OpenCL) {
+

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Since you're moving this code anyway, can this be split into its own 
> > > function?  I'm not sure if it's actually important that some of these 
> > > failures return immediately and some of them fall through to later checks.
> > >  I'm not sure if it's actually important that some of these failures 
> > > return immediately and some of them fall through to later checks.
> > 
> > Yes, it looks a bit random. Do we have any guideline whether we should 
> > return or keep diagnosing as long as possible?
> > 
> > 
> If the user is likely to have made multiple independent errors, it's good to 
> diagnose as many of them as possible.  But if it's just as likely that the 
> user messed up in a single way that should've meant we didn't take this code 
> path, then it's better to bail out early.
> 
> In this case, most of these diagnostics are looking for different special 
> OpenCL types, and those are probably all independent  of each other.
> 
> ...it does occur to me to wonder if more of these checks should be looking 
> through array types the way that the check for `half` does.  Presumably you 
> shouldn't be able to declare global arrays of images or pipes if you can't 
> declare non-array variables of them.
> ...it does occur to me to wonder if more of these checks should be looking 
> through array types the way that the check for half does. Presumably you 
> shouldn't be able to declare global arrays of images or pipes if you can't 
> declare non-array variables of them.

We actually provide dedicated diagnostic for all other types in 
`Sema::BuildArrayType`. No idea why half is handled here I will try to refactor 
that in a separate patch. 


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

https://reviews.llvm.org/D65744



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

[clang] 9376714 - [Clang FE] Recognize -mnop-mcount CL option (SystemZ only).

[Jonas Paulsson via cfe-commits]

Author: Jonas Paulsson
Date: 2019-11-05T12:12:36+01:00
New Revision: 93767143147b7d765c6ce8123a4226d449228649

URL: 
https://github.com/llvm/llvm-project/commit/93767143147b7d765c6ce8123a4226d449228649
DIFF: 
https://github.com/llvm/llvm-project/commit/93767143147b7d765c6ce8123a4226d449228649.diff

LOG: [Clang FE]  Recognize -mnop-mcount CL option (SystemZ only).

Recognize -mnop-mcount from the command line and add a function attribute
"mnop-mcount"="true" when passed.

When this option is used, a nop is added instead of a call to fentry. This
is used when building the Linux Kernel.

If this option is passed for any other target than SystemZ, an error is
generated.

Review: Ulrich Weigand
https://reviews.llvm.org/D67763

Added: 
clang/test/CodeGen/mnop-mcount.c

Modified: 
clang/include/clang/Basic/CodeGenOptions.def
clang/include/clang/Basic/DiagnosticCommonKinds.td
clang/include/clang/Driver/Options.td
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInvocation.cpp

Removed: 




diff  --git a/clang/include/clang/Basic/CodeGenOptions.def 
b/clang/include/clang/Basic/CodeGenOptions.def
index 99559b5abe7b..f0d101534e57 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -112,6 +112,7 @@ VALUE_CODEGENOPT(XRayInstructionThreshold , 32, 200)
 
 CODEGENOPT(InstrumentForProfiling , 1, 0) ///< Set when -pg is enabled.
 CODEGENOPT(CallFEntry , 1, 0) ///< Set when -mfentry is enabled.
+CODEGENOPT(MNopMCount , 1, 0) ///< Set when -mnop-mcount is enabled.
 CODEGENOPT(LessPreciseFPMAD  , 1, 0) ///< Enable less precise MAD instructions 
to
  ///< be generated.
 CODEGENOPT(PrepareForLTO , 1, 0) ///< Set when -flto is enabled on the

diff  --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td 
b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 7a416c282e3d..d6281f157eea 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -271,6 +271,8 @@ def err_target_unsupported_mcmse : Error<
   "-mcmse is not supported for %0">;
 def err_opt_not_valid_with_opt : Error<
   "option '%0' cannot be specified with '%1'">;
+def err_opt_not_valid_without_opt : Error<
+  "option '%0' cannot be specified without '%1'">;
 def err_opt_not_valid_on_target : Error<
   "option '%0' cannot be specified on this target">;
 

diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index d75a0f601d7e..fa609281a6cc 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2448,6 +2448,8 @@ def mpie_copy_relocations : Flag<["-"], 
"mpie-copy-relocations">, Group
 def mno_pie_copy_relocations : Flag<["-"], "mno-pie-copy-relocations">, 
Group;
 def mfentry : Flag<["-"], "mfentry">, HelpText<"Insert calls to fentry at 
function entry (x86/SystemZ only)">,
   Flags<[CC1Option]>, Group;
+def mnop_mcount : Flag<["-"], "mnop-mcount">, HelpText<"Generate 
mcount/__fentry__ calls as nops. To activate they need to be patched in.">,
+  Flags<[CC1Option]>, Group;
 def mips16 : Flag<["-"], "mips16">, Group;
 def mno_mips16 : Flag<["-"], "mno-mips16">, Group;
 def mmicromips : Flag<["-"], "mmicromips">, Group;

diff  --git a/clang/lib/CodeGen/CodeGenFunction.cpp 
b/clang/lib/CodeGen/CodeGenFunction.cpp
index 1b1d391a63df..89dd06948baa 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -893,6 +893,16 @@ void CodeGenFunction::StartFunction(GlobalDecl GD,
 Fn->addFnAttr("instrument-function-entry-inlined",
   getTarget().getMCountName());
   }
+  if (CGM.getCodeGenOpts().MNopMCount) {
+if (getContext().getTargetInfo().getTriple().getArch() !=
+llvm::Triple::systemz)
+  CGM.getDiags().Report(diag::err_opt_not_valid_on_target)
+<< "-mnop-mcount";
+if (!CGM.getCodeGenOpts().CallFEntry)
+  CGM.getDiags().Report(diag::err_opt_not_valid_without_opt)
+<< "-mnop-mcount" << "-mfentry";
+Fn->addFnAttr("mnop-mcount", "true");
+  }
 }
   }
 

diff  --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 16bf68254d19..6f3f7bfe61f4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4616,6 +4616,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction 
&JA,
   if (TC.SupportsProfiling())
 Args.AddLastArg(CmdArgs, options::OPT_mfentry);
 
+  if (TC.SupportsProfiling())
+Args.AddLastArg(CmdArgs, options::OPT_mnop_mcount);
+
   if (Args.getLastArg(options::OPT_fapple_kext) ||
   (Args.hasArg(options::OPT_mkernel) && types::isCXX(InputType)))
 CmdArgs.push_back("-fapple-kext");

diff  --git a/clang

[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki created this revision.
miyuki added reviewers: aprantl, rsmith, faisalv.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
miyuki edited reviewers, added: JDevlieghere; removed: jdoerfert.
Herald added a reviewer: jdoerfert.

This change creates a DenseMapInfo trait specialization for the
SourceLocation class. The empty key, the tombstone key and the hash
function are identical to DenseMapInfo, because we already
have hash maps that use raw the representation of SourceLocation as
a key.

The patch also converts the existing llvm::DenseMap,
llvm::DenseSet and std::map objects that store
source location as 'unsigned' to using SourceLocation directly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69840

Files:
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Edit/EditedSource.h
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/ARCMigrate/TransGCAttrs.cpp
  clang/lib/ARCMigrate/TransProperties.cpp
  clang/lib/ARCMigrate/Transforms.h
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/Edit/EditedSource.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -297,7 +297,7 @@
  Collector->PP.getSourceManager().isBeforeInTranslationUnit(
  Range.getBegin(), LastExpansionEnd)))
   return;
-Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd();
+Collector->Expansions[Range.getBegin()] = Range.getEnd();
 LastExpansionEnd = Range.getEnd();
   }
   // FIXME: handle directives like #pragma, #include, etc.
@@ -524,7 +524,7 @@
   auto L = File.SpelledTokens[NextSpelled].location();
   if (Offset <= SM.getFileOffset(L))
 return llvm::None; // reached the offset we are looking for.
-  auto Mapping = CollectedExpansions.find(L.getRawEncoding());
+  auto Mapping = CollectedExpansions.find(L);
   if (Mapping != CollectedExpansions.end())
 return Mapping->second; // found a mapping before the offset.
 }
Index: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
===
--- clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
+++ clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
@@ -44,13 +44,13 @@
   bool ShowLineMarkers; ///< Show #line markers.
   bool UseLineDirectives; ///< Use of line directives or line markers.
   /// Tracks where inclusions that change the file are found.
-  std::map FileIncludes;
+  std::map FileIncludes;
   /// Tracks where inclusions that import modules are found.
-  std::map ModuleIncludes;
+  std::map ModuleIncludes;
   /// Tracks where inclusions that enter modules (in a module build) are found.
-  std::map ModuleEntryIncludes;
+  std::map ModuleEntryIncludes;
   /// Tracks where #if and #elif directives get evaluated and whether to true.
-  std::map IfConditions;
+  std::map IfConditions;
   /// Used transitively for building up the FileIncludes mapping over the
   /// various \c PPCallbacks callbacks.
   SourceLocation LastInclusionLocation;
@@ -65,7 +65,7 @@
   void detectMainFileEOL();
   void handleModuleBegin(Token &Tok) {
 assert(Tok.getKind() == tok::annot_module_begin);
-ModuleEntryIncludes.insert({Tok.getLocation().getRawEncoding(),
+ModuleEntryIncludes.insert({Tok.getLocation(),
 (Module *)Tok.getAnnotationValue()});
   }
 private:
@@ -164,7 +164,7 @@
 return;
   FileID Id = FullSourceLoc(Loc, SM).getFileID();
   auto P = FileIncludes.insert(
-  std::make_pair(LastInclusionLocation.getRawEncoding(),
+  std::make_pair(LastInclusionLocation,
  IncludedFile(Id, NewFileType, PP.GetCurDirLookup(;
   (void)P;
   assert(P.second && "Unexpected revisitation of the same include directive");
@@ -199,8 +199,7 @@
const Module *Imported,
SrcMgr::CharacteristicKind FileType){
   if (Imported) {
-auto P = ModuleIncludes.insert(
-std::make_pair(HashLoc.getRawEncoding(), Imported));
+auto P = ModuleIncludes.insert(std::make_pair(HashLoc, Imported));
 (void)P;
 assert(P.second && "Unexpected revisitation of the same include directive");
   } else
@@ -209,8 +208,7 @@
 
 void InclusionRewriter::If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind ConditionValue) {
-  auto P = IfConditions.insert(
-  std::make_pair(Loc.getRawEncoding(), ConditionValue == CVK_True));
+  auto P = IfConditions.insert(std::make_pair(Loc, ConditionValue == CVK_True));
   (void)P;
   assert(P.second && "Unexpected revisitation of the same if directive");
 }
@@ -218,8 +216,7 @

[PATCH] D69841: Target Ivy bridge on macOS Mojave and later

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki created this revision.
davezarzycki added a reviewer: bob.wilson.
davezarzycki added a project: clang.

Ivy Bridge is required by macOS 10.14 (Mojave) and later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69841

Files:
  clang/lib/Driver/ToolChains/Arch/X86.cpp


Index: clang/lib/Driver/ToolChains/Arch/X86.cpp
===
--- clang/lib/Driver/ToolChains/Arch/X86.cpp
+++ clang/lib/Driver/ToolChains/Arch/X86.cpp
@@ -73,12 +73,15 @@
   if (Triple.isOSDarwin()) {
 if (Triple.getArchName() == "x86_64h")
   return "core-avx2";
-// macosx10.12 drops support for all pre-Penryn Macs.
-// Simulators can still run on 10.11 though, like Xcode.
-if (Triple.isMacOSX() && !Triple.isOSVersionLT(10, 12))
-  return "penryn";
 // The oldest x86_64 Macs have core2/Merom; the oldest x86 Macs have Yonah.
-return Is64Bit ? "core2" : "yonah";
+// The simulators (i.e. Darwin but not macOS) can still run on older Macs.
+// macosx10.12 requires Penryn.
+if (!Triple.isMacOSX() || Triple.isOSVersionLT(10, 12))
+  return Is64Bit ? "core2" : "yonah";
+// macosx10.14 requires Ivy Bridge.
+if (Triple.isOSVersionLT(10, 14))
+  return "penryn";
+return "ivybridge";
   }
 
   // Set up default CPU name for PS4 compilers.


Index: clang/lib/Driver/ToolChains/Arch/X86.cpp
===
--- clang/lib/Driver/ToolChains/Arch/X86.cpp
+++ clang/lib/Driver/ToolChains/Arch/X86.cpp
@@ -73,12 +73,15 @@
   if (Triple.isOSDarwin()) {
 if (Triple.getArchName() == "x86_64h")
   return "core-avx2";
-// macosx10.12 drops support for all pre-Penryn Macs.
-// Simulators can still run on 10.11 though, like Xcode.
-if (Triple.isMacOSX() && !Triple.isOSVersionLT(10, 12))
-  return "penryn";
 // The oldest x86_64 Macs have core2/Merom; the oldest x86 Macs have Yonah.
-return Is64Bit ? "core2" : "yonah";
+// The simulators (i.e. Darwin but not macOS) can still run on older Macs.
+// macosx10.12 requires Penryn.
+if (!Triple.isMacOSX() || Triple.isOSVersionLT(10, 12))
+  return Is64Bit ? "core2" : "yonah";
+// macosx10.14 requires Ivy Bridge.
+if (Triple.isOSVersionLT(10, 14))
+  return "penryn";
+return "ivybridge";
   }
 
   // Set up default CPU name for PS4 compilers.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69841: Target Ivy bridge on macOS Mojave and later

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added a comment.

Hi @bob.wilson – Am I missing something here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69841



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

[PATCH] D69033: [clangd] Improve symbol qualification in DefineInline code action

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196
 //~ -> we need to qualify Bar not x.
-if (!ND->getDeclContext()->isNamespace())
+if (!ND->getLexicalDeclContext()->isNamespace())
   return;

Why do we need to change this?
My understanding is that we want semantic decl context, not the lexical one 
here.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:200
+std::string Qualifier;
+// FIXME: Also take using directives and namespace aliases inside function
+// body into account.

NIT: it looks like this FIXME belongs to the `getQualification` function itself.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1721
+  namespace ns1 {
+  void foo();
+  namespace qq { void test(); }

NIT: could you indent functions inside the namespaces?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69033



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

[PATCH] D69608: [clangd] Helper for getting nested namespace qualification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:281
+
+auto *NSD = llvm::dyn_cast(Context);
+assert(NSD && "Non-namespace decl context found.");

NIT: `dyn_cast` + `assert` are equivalent to a single `llvm::cast`



Comment at: clang-tools-extra/clangd/AST.cpp:282
+auto *NSD = llvm::dyn_cast(Context);
+assert(NSD && "Non-namespace decl context found.");
+// Stop if this namespace is already visible at DestContext.

This can actually fail with non-namespace decl contexts, right?
Now that we expose this as a public helper, that's actually an important 
failure mode that we have to care about.

Maybe change the interface to accept `NamespaceDecl` or add assertions at the 
start of the function?



Comment at: clang-tools-extra/clangd/AST.cpp:287
+// Again, ananoymous namespaces are not spelled while qualifying a name.
+if (NSD->isAnonymousNamespace())
+  continue;

NIT: maybe check this alongside the `Context->isInlineNamespace()`?

```
auto *NSD = ...;
if (NSD->isAnonymousNamespace() || NSD->isInlineNamespace())
  continue;
```




Comment at: clang-tools-extra/clangd/AST.h:118
+/// such that it is visible in \p DestContext, considering all the using
+/// directives before \p InsertionPoint.
+/// Returns null if no qualification is necessary. For example, if you want to

NIT: using **namespace** directives

otherwise it's too easy to confuse those with using declarations, i.e. `using 
ns::foo;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69608



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 227860.
hokein marked 13 inline comments as done.
hokein added a comment.

address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/refactor/Rename.h
  clang-tools-extra/clangd/refactor/Tweak.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -38,8 +38,8 @@
 llvm::Expected>
 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
-llvm::Expected>
-runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName);
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, StringRef NewName);
 
 std::string runDumpAST(ClangdServer &Server, PathRef File);
 
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -96,11 +96,11 @@
   return std::move(*Result);
 }
 
-llvm::Expected> runRename(ClangdServer &Server,
-PathRef File, Position Pos,
-llvm::StringRef NewName) {
-  llvm::Optional>> Result;
-  Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result));
+llvm::Expected runRename(ClangdServer &Server, PathRef File,
+Position Pos, llvm::StringRef NewName) {
+  llvm::Optional> Result;
+  Server.rename(File, Pos, NewName, /*WantFormat=*/true,
+/*DirtyBuffer=*/nullptr, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -9,8 +9,10 @@
 #include "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "index/Ref.h"
 #include "refactor/Rename.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,9 +20,54 @@
 namespace clangd {
 namespace {
 
+using testing::Eq;
+using testing::Matches;
+using testing::Pair;
+using testing::UnorderedElementsAre;
+
 MATCHER_P2(RenameRange, Code, Range, "") {
   return replacementToEdit(Code, arg).range == Range;
 }
+MATCHER_P2(EqualFileEdit, Code, Ranges, "") {
+  using ReplacementMatcher = testing::Matcher;
+  std::vector Expected;
+  for (const auto &R : Ranges)
+Expected.push_back(RenameRange(Code, R));
+  auto Matcher =
+  testing::internal::UnorderedElementsAreArrayMatcher(
+  Expected.begin(), Expected.end());
+  return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements);
+}
+
+std::unique_ptr buildRefSlab(const Annotations &Code,
+  llvm::StringRef SymbolName,
+  llvm::StringRef Path) {
+  RefSlab::Builder Builder;
+  TestTU TU;
+  TU.HeaderCode = Code.code();
+  auto Symbols = TU.headerSymbols();
+  const auto &SymbolID = findSymbol(Symbols, SymbolName).ID;
+  for (const auto &Range : Code.ranges()) {
+Ref R;
+R.Kind = RefKind::Reference;
+R.Location.Start.setLine(Range.start.line);
+R.Location.Start.setColumn(Range.start.character);
+R.Location.End.setLine(Range.end.line);
+R.Location.End.setColumn(Range.end.character);
+auto U = URI::create(Path).toString();
+R.Location.FileURI = U.c_str();
+Builder.insert(SymbolID, R);
+  }
+
+  return std::make_unique(std::move(Builder).build());
+}
+
+std::vector> toVector(FileEdits FE) {
+  std::vector> Results;
+  for (auto &It : FE)
+Results.emplace_back(It.first().str(), std::move(It.getValue()));
+  return Results;
+}
 
 TEST(RenameTest, SingleFile) {
   struct Test {
@@ -80,10 +127,12 @@
 auto TU = TestTU::withCode(Code.code());
 auto AST = TU.build();
 auto RenameResult =
-renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
+rename({Code.point(), "abcde", AST, testPath(TU.Filename)});
 ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-auto ApplyResult =
-tooling::applyAllReplacements(Code.code(), *RenameResult);
+   

[PATCH] D69732: [WIP][LTO] Apply SamplePGO pipeline tunes for ThinLTO pre-link to full LTO

[Teresa Johnson via Phabricator via cfe-commits]

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

In D69732#1733447 , @wristow wrote:

> > This probably needs to be taken over by someone who cares about full LTO 
> > performance
>
> We at PlayStation are definitely interested in full LTO performance, so we're 
> looking into this.  We certainly agree with the rationale that if suppressing 
> some optimizations is useful to allow better SamplePGO matching, then we'd 
> expect that would apply equally to both ThinLTO and full LTO.
>
> I guess much of this comes down to a balancing act between:
>
> 1. The amount of the runtime benefit with Sample PGO if these loop 
> optimizations are deferred to the full LTO back-end (like they are for 
> ThinLTO).
> 2. The cost in compile-time resources in the full LTO back-end to do these 
> loop optimizations at that later stage.
>
>   From the discussion here, the Sample PGO runtime win (point 1) seems more 
> or less to be a given.  If we find the compile-time cost in the full LTO 
> back-end (point 2) is not significant, then the decision should be easy.  So 
> after seeing this patch, we're doing some experiments to at least try to get 
> a handle on this.  (I'm a bit concerned we won't be able to draw any hard 
> conclusions from the results of our experiments, but at least we'll be able 
> to make a better informed assessment.)
>
>   FTR, for PlayStation, we're using the old PM.  But we'll do some 
> experiments for both the old and new PM, to get a sense of the answers to the 
> (old PM) `LoopUnrollAndJam` point, and the (new PM) FIXME comment.


This is a good summary. I look forward to your results.




Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:614
   // during ThinLTO and perform the rest of the optimizations afterward.
   if (PrepareForThinLTO) {
 // Ensure we perform any last passes, but do so before renaming anonymous

wristow wrote:
> wenlei wrote:
> > this also need to be `PrepareForThinLTO || PrepareForLTO` for oldPM?
> I agree this is another instance where a balancing act question applies.  In 
> this case, assuming the comment about the concern of code bloat is accurate, 
> it's not so much about compile-time resources in the full LTO back-end, but 
> rather about minimizing the ThinLTO bitcode write/read time.  So if as this 
> WIP evolves, it ultimately is a win for SamplePGO to suppress some loop 
> optimizations (unrolling/vectorization) here, then that will probably also be 
> a //small // win in full LTO compile time.  That said, in addition to these 
> loop-related optimizations, there are other transformations here that are 
> done in the full LTO pipeline (but not in the ThinLTO pipeline).  So I 
> suspect if some change to check for `PrepareForThinLTO || PrepareForLTO` 
> (rather than only `PrepareForThinLTO`) makes sense here from a Sample PGO 
> perspective, then the change will be more complicated than simply adding the 
> small set of passes here followed by the early return (that is, I think there 
> are probably things after the `return` on line 621 that still ought to be 
> enabled for full LTO -- essentially continuing to do them in the pre-link 
> stage for full LTO, to try to avoid needing to do too much work in the full 
> LTO backend stage, since it's more of a problem for the full backend to 
> absorb that compile time cost).
This early return was not for Sample PGO btw. It was added much earlier with 
the thought that a) these types of optimizations might affect function 
importing heuristics because they could bloat the code; b) we can push more 
optimizations to the post-link in ThinLTO because it is parallel; and c) there 
isn't otherwise a benefit to doing these optimizations in the pre vs post link, 
i.e. they aren't cleanup/simplification passes.  The equation is of course 
different for full LTO which has a monolithic serial post link backend. But I 
believe this early return is the one @ormris is looking to remove on the 
ThinLTO pass to "merge" the two pipelines, which needs a good amount of 
evaluation on the ThinLTO performance side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69732



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+  for (auto &E : *Edits) {
+if (auto Err = reformatEdit(E.getValue(), Style))
+  elog("Failed to format replacements: {0}", std::move(Err));

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Maybe use `llvm::joinErrors` to combine multiple failures into a single 
> > > error?
> > > Should simplify the code.
> > I don't see using `llvm::joinErrors` can simplify the code here, 
> > `joinErrors` accepts two `Error` objects, and there is no way to create an 
> > empty Error object, we may end up with more code like:
> > 
> > ```
> > llvm::Optional Err;
> > for (auto &E : *Edits) {
> >if (auto E = reformatEdit(E.getValue(), Style)) {
> >   if (!Err) Err = std::move(E);
> >   else Err = joinErrors(std::move(*Err), std::move(Err));
> >}
> > }
> > ```
> Works nicely if you start with `Error::success`
> ```
> auto Err = Error::success();
> for (...) {
>   Err = llvm::joinErrors(reformatEdit(E.getValue(), Style));
> }
> if (Err)
>   return CB(std::move(Err));
> ```
thanks.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+if (!Index)
+  return NoIndexProvided;
+

ilya-biryukov wrote:
> Why isn't this a scope enum in the first place?
this is tricky, the reason why I put it here and another copy below is to avoid 
regression of local rename (keep existing tests passed), if we move it in the 
first place, the error message of local rename may be changed, e.g. when 
renaming an external symbol (we know that from AST) without Index support, we'd 
like to return "used outside of the main file" rather than "no index provided".

thinking more about this, the no-index case is making our code complicated 
(within-file rename with no-index, cross-file rename with no-index), I believe 
the no-index case is not important, and is rare in practice, we could change 
this behavior, or assume the index is always provided, this would help to 
simplify the code, WDYT?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa(RenameDecl))

ilya-biryukov wrote:
> Why not a blacklist? I'd expect us to blacklist:
> - things with custom names (operators, constructors, etc)
> - virtual methods
> 
> Probably some things are missing from the list, but fundamentally most that 
> have names are pretty similar, right?
I believe our plan is to support major kinds of symbols (class, functions, 
enums, typedef, alias) , I feel scary to allow renaming nearly all `NamedDecl` 
(in theory, it should work but we're uncertain, for our experience of local 
rename, it sometimes leads to very surprising results). Having a whitelist can 
lead us to a better state, these symbols are **officially supported**.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in
+// subclasses, which our index doesn't support yet.

ilya-biryukov wrote:
> Isn't the same true for local rename?
no, actually the local rename does handle this, it finds all overriden methods 
from the AST, and updates all of them when doing rename.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:221
+  if (!Index)
+return {};
+  RefsRequest RQuest;

ilya-biryukov wrote:
> This behavior is very surprising... Obviously, returning empty results is 
> incorrect and the callers have to handle the no-index case in a custom manner.
> 
> Maybe accept only non-null index and handle in the caller?
moved the no-index handling to the caller, the caller code is still a bit fuzzy.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}

ilya-biryukov wrote:
> Again, it looks like the function returns incorrect results and the callers 
> should actually handle the case where `SymbolID` cannot be obtained in a 
> custom manner.
> 
> Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the 
> callers handle this case gracefully?
hmm, is this critical? I think it is safe to assume getSymbolID should always 
succeed,  I believe we make this assumption in other code parts in clangd as 
well.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+// !positionToOffset is O(N), it is okay at the moment since we only
+// process at most 100 references.
+auto RangeOffset = toRangeOffset(Occurrence, InitialCode);

ilya-biryukov wrote:
> This is not true anymore, right?
> We should probably try getting to `O(N)` to avoid slowing things down
> 
> Could be a FIXME for now, but have to fix it soon.
yes, I think it is not too bad to leave it n

[PATCH] D69760: [Clang][Driver] Don't pun -fuse-ld=lld as -fuse-ld=lld-link with msvc

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

I'm not familiar with where/how the CrossWindows toolchain is used, but you say 
(and the code seems to agree) that it's intended to be able to use ld.bfd for 
linking code built for the MSVC ABI with MSVC/WinSDK libs? I'm surprised that 
even works at all (or does it, or is it just set up and tested for minimal 
cases and then unused?), as ld.bfd lacks support for a lot of COFF specific 
features that MSVC ABI uses (which probably can be worked around with 
`--allow-multiple-definitions` for some bits at least, but I'm not confident 
that it generally would work even with that).

I presume all of this is about how to call the linker, as compiling with `clang 
-target -windows-msvc`, using the MSVC toolchain, should work just fine?

I think a better distinguisher would be whether clang was invoked as 
`clang[++]` or `clang-cl` (i.e. driver mode). Normally when linking MSVC style, 
the build system would call `link` (or the link-like tool) with the linker 
flags directly. Occasionally one could be linking via the cl frontend, as `cl 
obj1.obj obj2.obj -Feout.exe somelib.lib -link -linkpath:some/path 
-other_link_style_option`.

Now when doing linking via the clang GCC-style driver, `clang obj1.obj obj2.obj 
-o out.exe -lsomelib -Lsome/path -Wl,--other-unix-linker-option`, one expects 
to use unix linking style flags. Hypothesis: There's no predecent for actually 
using an MSVC link-style linker via this syntax. Or is there?

If there isn't, we could make that the distinguishing factor instead.

The same issue has come up for build systems (meson tried to tackle this 
recently, don't remember how/if they did yet), when just invoking "clang" on a 
windows platform (instead of intentionally e.g. "clang-cl"), not knowing 
whether the implicit default triple is an msvc or mingw triple, which means 
they don't know in what form to pass linker options. For cross cases where 
there's an explicit `-target` option, this are slightly clearer (at least from 
some perspectives), but with the default target, you're really working blindly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69760



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

[PATCH] D69769: [Driver] Don't pun -fuse-ld=lld as -fuse-ld=lld-link with msvc

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Same thing here, no need to send a separate patch for backporting.

(And in general, for cases where backporting would be relevant, you'd normally 
just make a patch for master, then let it be merged there, and request a 
backport of it from the release branch maintainer, who would do the 
cherrypicking - and only in conflicting cases, he'd request help merging. But 
all of that is done after the discussion of the initial patch for master has 
settled and the patch is merged in master.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D69769



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

[PATCH] D68720: Support -fstack-clash-protection for x86

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@sylvestre.ledru did the testing and benchmarking on firefox (see 
https://bugzilla.mozilla.org/show_bug.cgi?id=1588710#c12), everything seems ok, 
let's move forward?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720



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

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via Phabricator via cfe-commits]

hliao marked 2 inline comments as done.
hliao added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:7689
+  // Coerce HIP pointer arguments from generic pointers to global ones.
+  llvm::Type *coerce(llvm::Type *Ty, unsigned DefaultAS,
+ unsigned GlobalAS) const {

tra wrote:
> hliao wrote:
> > tra wrote:
> > > Now it could use a more descriptive name, too. :-)
> > > 
> > > You can now also make DefaultAS/GlobalAS into local variables as you have 
> > > access to `getContext()` here.
> > name is changed but I want to leave `DefaultAS` and `GlobalAS` as 
> > parameters as they may vary from HIP to OpenCL and different targets. Even 
> > though it may be rare case, I want to avoid careless errors.
> You may not need it, ever and it would be easy to add, but I'll leave it up 
> to you.
> 
> If you do want to keep them as parameters you may want to consider renaming 
> them to FromAS/ToAS.
> There's nothing in the code that has anything to do with whether they are for 
> generic/specific address space and the function name does not indicate the 
> direction of coercion between them. It's very easy to pass them in the wrong 
> order and not notice it. Making them local variables would avoid it. Giving 
> names some sort of 'directionality' would at least give user a hint what goes 
> where, even if it would not prevent making the error.
> 
From the target device side, we have generic and global addresses. But, at the 
language level, we have `opencl_global` and `cuda_device`. Even though they map 
into the same address space, it would be very confusing if they are misused to 
initialize that address space numbers. That's why the original helper makes 
more sense to me and makes the code more readable. Anyway, I change the 
parameter names to give a clear direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826



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

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via Phabricator via cfe-commits]

hliao updated this revision to Diff 227864.
hliao marked an inline comment as done.
hliao added a comment.

revise parameter names


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu

Index: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
===
--- /dev/null
+++ clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -fcuda-is-device \
+// RUN: -emit-llvm -x hip %s -o - | FileCheck %s
+#include "Inputs/cuda.h"
+
+// Coerced struct from `struct S` without all generic pointers lowered into
+// global ones.
+// CHECK: %struct.S.coerce = type { i32 addrspace(1)*, float addrspace(1)* }
+// CHECK: %struct.T.coerce = type { [2 x float addrspace(1)*] }
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel1Pi(i32 addrspace(1)* %x.coerce)
+__global__ void kernel1(int *x) {
+  x[0]++;
+}
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel2Ri(i32 addrspace(1)* dereferenceable(4) %x.coerce)
+__global__ void kernel2(int &x) {
+  x++;
+}
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel3PU3AS2iPU3AS1i(i32 addrspace(2)* %x, i32 addrspace(1)* %y)
+__global__ void kernel3(__attribute__((address_space(2))) int *x,
+__attribute__((address_space(1))) int *y) {
+  y[0] = x[0];
+}
+
+// CHECK: define void @_Z4funcPi(i32* %x)
+__device__ void func(int *x) {
+  x[0]++;
+}
+
+struct S {
+  int *x;
+  float *y;
+};
+// `by-val` struct will be coerced into a similar struct with all generic
+// pointers lowerd into global ones.
+// CHECK: define amdgpu_kernel void @_Z7kernel41S(%struct.S.coerce %s.coerce)
+__global__ void kernel4(struct S s) {
+  s.x[0]++;
+  s.y[0] += 1.f;
+}
+
+// If a pointer to struct is passed, only the pointer itself is coerced into the global one.
+// CHECK: define amdgpu_kernel void @_Z7kernel5P1S(%struct.S addrspace(1)* %s.coerce)
+__global__ void kernel5(struct S *s) {
+  s->x[0]++;
+  s->y[0] += 1.f;
+}
+
+struct T {
+  float *x[2];
+};
+// `by-val` array is also coerced.
+// CHECK: define amdgpu_kernel void @_Z7kernel61T(%struct.T.coerce %t.coerce)
+__global__ void kernel6(struct T t) {
+  t.x[0][0] += 1.f;
+  t.x[1][0] += 2.f;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7685,6 +7685,42 @@
   bool isHomogeneousAggregateSmallEnough(const Type *Base,
  uint64_t Members) const override;
 
+  // Coerce HIP pointer arguments from generic pointers to global ones.
+  llvm::Type *coerceKernelArgumentType(llvm::Type *Ty, unsigned FromAS,
+   unsigned ToAS) const {
+// Structure types.
+if (auto STy = dyn_cast(Ty)) {
+  SmallVector EltTys;
+  bool Changed = false;
+  for (auto T : STy->elements()) {
+auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+EltTys.push_back(NT);
+Changed |= (NT != T);
+  }
+  // Skip if there is no change in element types.
+  if (!Changed)
+return STy;
+  if (STy->hasName())
+return llvm::StructType::create(
+EltTys, (STy->getName() + ".coerce").str(), STy->isPacked());
+  return llvm::StructType::get(getVMContext(), EltTys, STy->isPacked());
+}
+// Arrary types.
+if (auto ATy = dyn_cast(Ty)) {
+  auto T = ATy->getElementType();
+  auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+  // Skip if there is no change in that element type.
+  if (NT == T)
+return ATy;
+  return llvm::ArrayType::get(NT, ATy->getNumElements());
+}
+// Single value types.
+if (Ty->isPointerTy() && Ty->getPointerAddressSpace() == FromAS)
+  return llvm::PointerType::get(
+  cast(Ty)->getElementType(), ToAS);
+return Ty;
+  }
+
 public:
   explicit AMDGPUABIInfo(CodeGen::CodeGenTypes &CGT) :
 DefaultABIInfo(CGT) {}
@@ -7812,14 +7848,22 @@
 
   // TODO: Can we omit empty structs?
 
-  // Coerce single element structs to its element.
+  llvm::Type *LTy = nullptr;
   if (const Type *SeltTy = isSingleElementStruct(Ty, getContext()))
-return ABIArgInfo::getDirect(CGT.ConvertType(QualType(SeltTy, 0)));
+LTy = CGT.ConvertType(QualType(SeltTy, 0));
+
+  if (getContext().getLangOpts().HIP) {
+if (!LTy)
+  LTy = CGT.ConvertType(Ty);
+LTy = coerceKernelArgumentType(
+LTy, /*FromAS=*/getContext().getTargetAddressSpace(LangAS::Default),
+/*ToAS=*/getContext().getTargetAddressSpace(LangAS::cuda_device));
+  }
 
   // If we set CanBeFlattened to true, CodeGen will expand the struct to its
   // individual elements, which confuses the Clove

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7246-7248
+// Set implicit behavior except for "default" for defaultmap
+if ((Bits & OMP_MAP_IMPLICIT) &&
+(ImplicitBehavior != OMPC_DEFAULTMAP_MODIFIER_default)) {

cchen wrote:
> ABataev wrote:
> > Hmm, this is strange, Do we really need this kind of processing here? The 
> > variables must be mapped implicitly in Sema and, thus, all this processing 
> > of the default mapping rules should not be required.
> I'm now having design question about setting the correct implicit map type in 
> Sema for the below situation:
> ```
> int *ptr_1, *ptr_2, arr[50];
> #pragma omp target defaultmap(alloc:pointer) defaultmap(from:aggregate)
> {
>   ptr_1++, ptr_2++;
>   arr[0]++;
> }
> ```
> In this case we need to store two maptypes - alloc and from for an 
> `ActOnOpenMPMapClause` but `ActOnOpenMPMapClause` only pass one maptype so 
> I'm wondering should I modify the interface of OMPMapClause which pass an 
> array of maptypes rather than one maptype variable?
Just create 3 arrays instead of single array for mapped items and call 
`ActOnOpenMPMapClause` for each of them



Comment at: clang/lib/Sema/SemaOpenMP.cpp:149
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+DefaultmapInfo DefaultmapMap[3];
+

cchen wrote:
> ABataev wrote:
> > ABataev wrote:
> > > Maybe, it would be better to make `DMVC_unspecified` the last one in the 
> > > `DefaultMapVariableCategory` and use it as an array dimension here rather 
> > > than rely on the magical number?
> > Not done.
> Not sure about this one. I've already put DMVC_unspecified to the last one in 
> the DefaultMapVariableCategory enum so that I now don't need magic number. Or 
> you are pointing something else? Thanks
Use `DefaultmapInfo DefaultmapMap[DMVC_unspecified]` instead of `DefaultmapInfo 
DefaultmapMap[3]`, this is what I meant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69204



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

[PATCH] D69813: [analyzer][WIP] CERTStrChecker: Model gets()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:124
+  if (const SymbolicRegion *SR = DestMR->getSymbolicBase())
+if (const Expr *SizeExpr = getDynamicSizeExpr(C.getState(), SR))
+  return exprToStr(SizeExpr, C);

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > Charusso wrote:
> > > > > NoQ wrote:
> > > > > > Again, you will have to highlight the allocation site with a note. 
> > > > > > Therefore you will have to write a bug visitor that traverses the 
> > > > > > size expression at some point (or, equivalently, a note tag when 
> > > > > > the size expression is evaluated). Therefore you don't need to 
> > > > > > store the expression in the program state.
> > > > > Yes, you have pointed out the necessary visitor, but it needs more 
> > > > > thinking.
> > > > > 
> > > > > I have a memory region which could be any kind of "memory block 
> > > > > region" therefore I have no idea where is the size expression. We are 
> > > > > supporting ~20 different allocations, which is nothing compared to 
> > > > > the wild with the not so uncommon 5+ parameter allocators. Therefore 
> > > > > I still do not want to reverse engineer a small MallocChecker + 
> > > > > ExprEngine + BuiltinFunctionChecker inside my checker. They provide 
> > > > > the necessary `DynamicSizeInfo` easily, which could be used in at 
> > > > > least 4 checkers at the moment (which I have pointed out earlier in 
> > > > > D68725).
> > > > > 
> > > > > If I have the size expression in the dynamic size map, and I can 
> > > > > clearly point out the destination buffer, it is a lot more simplified 
> > > > > to traverse the graph where the buffer and its size comes from.
> > > > Well, you really do not want to store `SizeExpr` of `malloc(SizeExpr)` 
> > > > and you are right I will have to traverse from it to see whether the 
> > > > `SizeExpr` is ambiguous or not, where it comes from.
> > > > 
> > > > I want to rely on the `trackExpressionValue()` as the `SizeExpr` is 
> > > > available by `getDynamicSizeExpr()`, so it is one or two lines of code.
> > > > 
> > > > Would you create your own switch-case to see where is the size 
> > > > expression goes in the allocation and use `trackExpressionValue()` on 
> > > > it? So that you do not store information in the global state which 
> > > > results in better run-time / less memory.
> > > > 
> > > > At first I really wanted to model `malloc()` and `realloc()` and stuff, 
> > > > then I realized the `MallocChecker` provides every information I need. 
> > > > Would it be a better idea to create my own tiny `MallocChecker` inside 
> > > > my checker which does nothing but marks the size expression being 
> > > > interesting with `NoteTags`?
> > > > 
> > > > Also I am thinking of a switch-case on the `DefinedOrUnknownSVal Size` 
> > > > which somewhere has an expression inside it which I could 
> > > > `trackExpressionValue()` on.
> > > > 
> > > > Basically we are missing the rules what to use and I have picked the 
> > > > easiest solution. Could you share please which would be the right 
> > > > direction for such a simple task?
> > > > I want to rely on the `trackExpressionValue()` as the `SizeExpr` is 
> > > > available by `getDynamicSizeExpr()`, so it is one or two lines of code.
> > > 
> > > This won't work. `trackExpressionValue()` can only track an active 
> > > expression (that has, or at least should have, a value in the bug-node's 
> > > environment). You'll have to make a visitor or a note tag.
> > > 
> > > You can either make your own visitor (which will detect the node in which 
> > > the extent information becomes present), or convert `MallocChecker` to 
> > > use note tags and then inter-operate with those tags (though the 
> > > interestingness map - "i mark the symbol as interesting so i'm interested 
> > > in highlighting the allocation site" - or a similar mechanism). The 
> > > second approach is more work because no such interoperation has ever been 
> > > implemented yet, but it should be pretty rewarding for the future.
> > > This won't work. trackExpressionValue() can only track an active 
> > > expression (that has, or at least should have, a value in the bug-node's 
> > > environment). You'll have to make a visitor or a note tag.
> > So because most likely after the `malloc()` the `size` symbol dies, the 
> > `trackExpressionValue()` cannot track dead symbols? Because we could make 
> > the `size` dying base on the `buffer`, we have some dependency logic for 
> > that. It also represents the truth, the size is part of that memory block's 
> > region. After that we could track the expression of the `size`?
> > So because most likely after the malloc() the size symbol dies...?
> 
> After the `malloc()` is consumed, the size //expression// dies and gets 
> cleaned up from the //Environment//. The symbol will only die if the value 
> wasn't put into the //Store//

[PATCH] D69844: [Basic] Integrate SourceLocation with FoldingSet, NFCI

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki created this revision.
miyuki added reviewers: aprantl, faisalv, rsmith, JDevlieghere.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch removes the necessity to access the SourceLocation internal
representation in several places which use FoldingSet objects.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69844

Files:
  clang/include/clang/Basic/SourceLocation.h
  clang/lib/Analysis/PathDiagnostic.cpp
  clang/lib/Basic/SourceLocation.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp


Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2144,8 +2144,8 @@
   for (SourceRange range : Ranges) {
 if (!range.isValid())
   continue;
-hash.AddInteger(range.getBegin().getRawEncoding());
-hash.AddInteger(range.getEnd().getRawEncoding());
+hash.Add(range.getBegin());
+hash.Add(range.getEnd());
   }
 }
 
@@ -2167,8 +2167,8 @@
   for (SourceRange range : Ranges) {
 if (!range.isValid())
   continue;
-hash.AddInteger(range.getBegin().getRawEncoding());
-hash.AddInteger(range.getEnd().getRawEncoding());
+hash.Add(range.getBegin());
+hash.Add(range.getEnd());
   }
 }
 
Index: clang/lib/Basic/SourceLocation.cpp
===
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -74,6 +75,10 @@
   return OS.str();
 }
 
+void SourceLocation::Profile(llvm::FoldingSetNodeID &Node) const {
+  Node.AddInteger(ID);
+}
+
 LLVM_DUMP_METHOD void SourceLocation::dump(const SourceManager &SM) const {
   print(llvm::errs(), SM);
   llvm::errs() << '\n';
Index: clang/lib/Analysis/PathDiagnostic.cpp
===
--- clang/lib/Analysis/PathDiagnostic.cpp
+++ clang/lib/Analysis/PathDiagnostic.cpp
@@ -1067,9 +1067,9 @@
 
//===--===//
 
 void PathDiagnosticLocation::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger(Range.getBegin().getRawEncoding());
-  ID.AddInteger(Range.getEnd().getRawEncoding());
-  ID.AddInteger(Loc.getRawEncoding());
+  ID.Add(Range.getBegin());
+  ID.Add(Range.getEnd());
+  ID.Add(Loc);
 }
 
 void PathDiagnosticPiece::Profile(llvm::FoldingSetNodeID &ID) const {
@@ -1079,8 +1079,8 @@
   ID.AddInteger((unsigned) getDisplayHint());
   ArrayRef Ranges = getRanges();
   for (const auto &I : Ranges) {
-ID.AddInteger(I.getBegin().getRawEncoding());
-ID.AddInteger(I.getEnd().getRawEncoding());
+ID.Add(I.getBegin());
+ID.Add(I.getEnd());
   }
 }
 
Index: clang/include/clang/Basic/SourceLocation.h
===
--- clang/include/clang/Basic/SourceLocation.h
+++ clang/include/clang/Basic/SourceLocation.h
@@ -26,6 +26,8 @@
 
 template  struct DenseMapInfo;
 
+class FoldingSetNodeID;
+
 } // namespace llvm
 
 namespace clang {
@@ -179,6 +181,9 @@
 return ID * 37U;
   }
 
+  /// Write this source location to a FoldingSetNodeID
+  void Profile(llvm::FoldingSetNodeID &Node) const;
+
   void print(raw_ostream &OS, const SourceManager &SM) const;
   std::string printToString(const SourceManager &SM) const;
   void dump(const SourceManager &SM) const;


Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2144,8 +2144,8 @@
   for (SourceRange range : Ranges) {
 if (!range.isValid())
   continue;
-hash.AddInteger(range.getBegin().getRawEncoding());
-hash.AddInteger(range.getEnd().getRawEncoding());
+hash.Add(range.getBegin());
+hash.Add(range.getEnd());
   }
 }
 
@@ -2167,8 +2167,8 @@
   for (SourceRange range : Ranges) {
 if (!range.isValid())
   continue;
-hash.AddInteger(range.getBegin().getRawEncoding());
-hash.AddInteger(range.getEnd().getRawEncoding());
+hash.Add(range.getBegin());
+hash.Add(range.getEnd());
   }
 }
 
Index: clang/lib/Basic/SourceLocation.cpp
===
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -74,6 +75

[PATCH] D69813: [analyzer][WIP] CERTStrChecker: Model gets()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 227867.
Charusso marked 6 inline comments as done.
Charusso added a comment.

- Use existing visitors.
- Make the `MallocBugVisitor` available for every checker.
- Fix duplication of fix-its on the warning path piece when we emit a note as 
well.


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

https://reviews.llvm.org/D69813

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cert/str31-alloc.cpp
  clang/test/Analysis/cert/str31-notes.cpp
  clang/test/Analysis/cert/str31-safe.cpp
  clang/test/Analysis/cert/str31-unsafe.cpp

Index: clang/test/Analysis/cert/str31-unsafe.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-unsafe.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+// The following is not defined therefore the safe functions are unavailable.
+// #define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // CHECK-FIXES: if (fgets(buf, sizeof(buf), stdin)) {}
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {}
+}
+} // namespace test_gets_good
Index: clang/test/Analysis/cert/str31-safe.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-safe.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // CHECK-FIXES: if (gets_s(buf, sizeof(buf))) {}
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (gets_s(buff, sizeof(buff))) {}
+}
+} // namespace test_gets_good
Index: clang/test/Analysis/cert/str31-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-notes.cpp
@@ -0,0 +1,31 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -analyzer-output=text -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+// The following is not defined therefore the safe functions are unavailable.
+// #define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+void *malloc(size_t size);
+void free(void *memblock);
+
+void func(void) {
+  unsigned size = 13;
+  // expected-note@-1 {{'size' initialized to 13}}
+
+  char *buf = (char *)malloc(size);
+  // expected-note@-1 {{Memory is allocated}}
+
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // expected-note@-2 {{'gets' could write outside of 'buf'}}
+
+  // CHECK-FIXES: if (fgets(buf, size, stdin)) {}
+  free(buf);
+}
Index: clang/test/Analysis/cert/str31-alloc.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-alloc.cpp
@

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1151
+  || Value > 5
+  || Value < 1)
+TC.getDriver().Diag(diag::err_drv_invalid_int_value)

dblaikie wrote:
> probinson wrote:
> > Clang doesn't support DWARF v1.  I expect nobody does at this point (DWARF 
> > 2 came out in 1993).
> You'd recommend/request moving this bound up to 2 (to be consistent with the 
> fact that clang supports -gdwarf-2 technically (even though we probably don't 
> produce fully conformant DWARFv2 these days, I would guess))?
It wants to permit the exact same range as the -gdwarf-N options.



Comment at: clang/test/CodeGen/debug-default-version.c:24
+// disables codeview, of you can explicitly ask for both if you don't know how
+// the app will be debugged.
+// RUN: %clang -target i686-pc-windows-msvc -fdebug-default-version=2 -gdwarf 
-S -emit-llvm -o - %s \

dblaikie wrote:
> probinson wrote:
> > Does that actually work?  Last I checked, DWARF and COFF didn't play 
> > nicely, but I admit that was quite a while ago.
> Existing test cases cover scenarios like this (see 
> clang/test/CodeGen/dwarf-version.c), so this seems consistent to preserve 
> that sort of functionality, however well it currently works.
Ok.


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

https://reviews.llvm.org/D69822



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

[PATCH] D65511: Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Sorry, seems I forgot to submit this comment..




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11545
+for (CXXMethodDecl *M : WorkList) {
+  DefineImplicitSpecialMember(*this, M, M->getLocation());
+  ActOnFinishInlineFunctionDef(M);

aganea wrote:
> rnk wrote:
> > hans wrote:
> > > rnk wrote:
> > > > Do we need to do this until fixpoint? Suppose a dllexported implicit 
> > > > special member triggers a template instantiation, and the template has 
> > > > a dllexported defaulted special member, something like:
> > > > ```
> > > > struct Bar { Bar(); };
> > > > template  struct Foo { __declspec(dllexport) Foo() = 
> > > > default; Bar obj; };
> > > > struct Baz {
> > > >... not sure how to trigger instantiation
> > > > };
> > > > ```
> > > I think that should work, and that's why the function is written to be 
> > > re-entrant by having a local worklist. If it triggers a template 
> > > instantiation, ActOnFinishCXXNonNestedClass should get called for the 
> > > newly instantiated class. But I'm also not sure how to write a test case 
> > > that would trigger it.
> > Right, I see. Nevermind then.
> This works on latest MSVC:
> ```
> $ cat test.cc
> struct Bar { Bar(); };
> template  struct Foo { __declspec(dllexport) Foo() = default; Bar 
> b; };
> template class Foo;
> 
> $ cl /c /Z7 test.cc   <-- crashes if using clang-cl
> 
> $ lib test.obj /def
> 
> $ cat test2.cc
> struct Bar { Bar() { } };
> template  struct Foo { __declspec(dllimport) Foo() = default; Bar 
> b; };
> int main() {
>   Foo f;
>   return 0;
> }
> 
> $ cl /c /Z7 test2.cc
> 
> $ link test.lib test2.cc /DEBUG
> ```
Thanks! I believe that's a separate issue, though, and not related to this 
patch. I've filed https://bugs.llvm.org/show_bug.cgi?id=42857 for it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65511



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

Re: [PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

[Nico Weber via cfe-commits]

Thanks!

On Mon, Nov 4, 2019 at 4:31 PM James Y Knight via Phabricator via
cfe-commits  wrote:

> jyknight added a comment.
>
> Updated release notes in d11a9018b773c0359934a7989d886b02468112e4 <
> https://reviews.llvm.org/rGd11a9018b773c0359934a7989d886b02468112e4>.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D67983/new/
>
> https://reviews.llvm.org/D67983
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69615: [clangd] Implement a function to lex the file to find candidate occurrences.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 227877.
hokein marked 4 inline comments as done.
hokein added a comment.

address comments, and simplify the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69615

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -680,6 +680,27 @@
 EXPECT_EQ(Res.EnclosingNamespace, Case.EnclosingNamespace) << Test.code();
   }
 }
+
+TEST(SourceCodeTests, IdentifierRanges) {
+  Annotations Code(R"cpp(
+   class [[Foo]] {};
+   // Foo
+   /* Foo */
+   void f([[Foo]]* foo1) {
+ [[Foo]] foo2;
+ auto S = [[Foo]]();
+// cross-line identifier is not supported.
+F\
+o\
+o foo2;
+   }
+  )cpp");
+  LangOptions LangOpts;
+  LangOpts.CPlusPlus = true;
+  EXPECT_EQ(Code.ranges(),
+collectIdentifierRanges("Foo", Code.code(), LangOpts));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/SourceCode.h
===
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -232,6 +232,11 @@
 llvm::StringMap collectIdentifiers(llvm::StringRef Content,
  const format::FormatStyle &Style);
 
+/// Collects all ranges of the given identifier in the source code.
+std::vector collectIdentifierRanges(llvm::StringRef Identifier,
+   llvm::StringRef Content,
+   const LangOptions &LangOpts);
+
 /// Collects words from the source code.
 /// Unlike collectIdentifiers:
 /// - also finds text in comments:
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -719,41 +719,49 @@
   return formatReplacements(Code, std::move(*CleanReplaces), Style);
 }
 
-void lex(llvm::StringRef Code, const format::FormatStyle &Style,
- llvm::function_ref Action) {
+void lex(llvm::StringRef Code, const LangOptions &LangOpts,
+ llvm::function_ref
+ Action) {
   // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
   std::string NullTerminatedCode = Code.str();
   SourceManagerForFile FileSM("dummy.cpp", NullTerminatedCode);
   auto &SM = FileSM.get();
   auto FID = SM.getMainFileID();
-  Lexer Lex(FID, SM.getBuffer(FID), SM, format::getFormattingLangOpts(Style));
+  // Create a raw lexer (with no associated preprocessor object).
+  Lexer Lex(FID, SM.getBuffer(FID), SM, LangOpts);
   Token Tok;
 
   while (!Lex.LexFromRawLexer(Tok))
-Action(Tok, sourceLocToPosition(SM, Tok.getLocation()));
+Action(Tok, SM);
   // LexFromRawLexer returns true after it lexes last token, so we still have
   // one more token to report.
-  Action(Tok, sourceLocToPosition(SM, Tok.getLocation()));
+  Action(Tok, SM);
 }
 
 llvm::StringMap collectIdentifiers(llvm::StringRef Content,
  const format::FormatStyle &Style) {
   llvm::StringMap Identifiers;
-  lex(Content, Style, [&](const clang::Token &Tok, Position) {
-switch (Tok.getKind()) {
-case tok::identifier:
-  ++Identifiers[Tok.getIdentifierInfo()->getName()];
-  break;
-case tok::raw_identifier:
+  auto LangOpt = format::getFormattingLangOpts(Style);
+  lex(Content, LangOpt, [&](const clang::Token &Tok, const SourceManager &) {
+if (Tok.getKind() == tok::raw_identifier)
   ++Identifiers[Tok.getRawIdentifier()];
-  break;
-default:
-  break;
-}
   });
   return Identifiers;
 }
 
+std::vector collectIdentifierRanges(llvm::StringRef Identifier,
+   llvm::StringRef Content,
+   const LangOptions &LangOpts) {
+  std::vector Ranges;
+  lex(Content, LangOpts, [&](const clang::Token &Tok, const SourceManager &SM) {
+if (Tok.getKind() == tok::raw_identifier)
+  if (Tok.getRawIdentifier() == Identifier)
+if (auto Range = getTokenRange(SM, LangOpts, Tok.getLocation()))
+  Ranges.push_back(*Range);
+  });
+  return Ranges;
+}
+
 namespace {
 struct NamespaceEvent {
   enum {
@@ -786,8 +794,9 @@
   std::string NSName;
 
   NamespaceEvent Event;
-  lex(Code, Style, [&](const clang::Token &Tok, Position P) {
-Event.Pos = std::move(P);
+  lex(Code, format::getFormattingLangOpts(Style),
+  [&](const clang::Token &Tok,const SourceManager &SM) {
+Event.Pos = sourceLocToPosition(SM, Tok.getLocation());
 switc

[PATCH] D68969: [clang-format] Remove the dependency on frontend

[Nico Weber via Phabricator via cfe-commits]

thakis added a subscriber: vlad.tsyrklevich.
thakis added a comment.

The revert message said:

  Vlad Tsyrklevich via cfe-commits 
  Tue, Oct 29, 1:51 PM (7 days ago)
  to via, mydeveloperday
  
  I've reverted this commit as it was causing UBSan failures on the ubsan bot. 
These failures looked like:
  llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: pointer index 
expression with base 0x overflowed to 0xfffa
  
  Looking at a backtrace, this line was reached from the `Diags.print(nullptr, 
llvm::errs(), (ShowColors && !NoShowColors));` call introduced in this change.

Is that enough to see what's wrong? If not, @vlad.tsyrklevich , can you give 
repro steps for that ubsan failure that made you revert ec66603 in efed314 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68969



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

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:220
+  bool VisitOverloadExpr(OverloadExpr *E) {
+if (!E->decls().empty())
+  return true; // handled by findExplicitReferences.

ilya-biryukov wrote:
> hokein wrote:
> > IIUC, if the decls don't have the same kind, we will not highlight it 
> > (previously, we fall back to `DependentName`).
> You're right. I don't think highlighting as dependent name is any better than 
> not highlighting at all in that case, though.
> Therefore, I would argue it's not worth adding more code to handle this 
> corner case (I'm not even sure it's possible in practice, it'll take quite 
> some time to come up with examples like these)
I thought it is better to highlight it rather than not highlighting, but I 
agree with you, it may be not worth handling this edge case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69673



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

[PATCH] D69615: [clangd] Implement a function to lex the file to find candidate occurrences.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:769
+llvm::StringRef TokenName;
+if (Tok.getKind() == tok::identifier)
+  TokenName = Tok.getIdentifierInfo()->getName();

ilya-biryukov wrote:
> Why do we have both `raw_identifier` and `identifier`?
> We should run lexer in the raw mode, in which case we should always get 
> `raw_identifier`.
I just borrowed from `collectIdentifiers`. You are right, we only run a raw 
lexer, updated the code.



Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:690
+   void f() {
+ [[Foo]] foo;
+   }

ilya-biryukov wrote:
> Could you add a few more occurrences?
> Maybe also with the weird things like multi-line tokens:
> ```
> F\
> o\
> o
> ```
Done, note that multi-line identifier is not supported -- `getRawIdentifier` 
returns a literal name `F\o\o` which makes the check `Tok.getRawIdentifier() == 
Identifier` fail. I think it is fine to not support it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69615



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

[clang] 642916a - [OPENMP][DOCS]Fix coloring of the implemented features status, NFC.

[Alexey Bataev via cfe-commits]

Author: Alexey Bataev
Date: 2019-11-05T10:13:58-05:00
New Revision: 642916adc97e54810aa597512ca7012b3c8697c5

URL: 
https://github.com/llvm/llvm-project/commit/642916adc97e54810aa597512ca7012b3c8697c5
DIFF: 
https://github.com/llvm/llvm-project/commit/642916adc97e54810aa597512ca7012b3c8697c5.diff

LOG: [OPENMP][DOCS]Fix coloring of the implemented features status, NFC.

Added: 


Modified: 
clang/docs/OpenMPSupport.rst

Removed: 




diff  --git a/clang/docs/OpenMPSupport.rst b/clang/docs/OpenMPSupport.rst
index f3922fa20837..b47a2b65d4f4 100644
--- a/clang/docs/OpenMPSupport.rst
+++ b/clang/docs/OpenMPSupport.rst
@@ -126,11 +126,11 @@ implementation.
 
+--+--+--+---+
 | loop extension   | #pragma omp loop (directive)  
   | :none:`unclaimed`| 
  |
 
+--+--+--+---+
-| loop extension   | collapse imperfectly nested loop  
   | :none:`done` | 
  |
+| loop extension   | collapse imperfectly nested loop  
   | :good:`done` | 
  |
 
+--+--+--+---+
 | loop extension   | collapse non-rectangular nested loop  
   | :good:`done` | 
  |
 
+--+--+--+---+
-| loop extension   | C++ range-base for loop   
   | :none:`done` | 
  |
+| loop extension   | C++ range-base for loop   
   | :good:`done` | 
  |
 
+--+--+--+---+
 | loop extension   | clause: nosimd for SIMD directives
   | :none:`unclaimed`| 
  |
 
+--+--+--+---+
@@ -160,11 +160,11 @@ implementation.
 
+--+--+--+---+
 | task extension   | master taskloop   
   | :good:`done` | 
  |
 
+--+--+--+---+
-| task extension   | parallel master taskloop  
   | :none:`done` | 
  |
+| task extension   | parallel master taskloop  
   | :good:`done` | 
  |
 
+--+--+--+---+
-| task extension   | master taskloop simd  
   | :none:`done` | 
  |
+| task extension   | master taskloop simd  
   | :good:`done` | 
  |
 
+--+-

[PATCH] D69825: [Clang][Driver] Bypass cc1 process and re-enter driver main

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Thanks for sending this out!

Instead of the dynamic lookup of that symbol, what do you think about passing 
in the function via a normal api? That way, the type checker and linker help us 
keep things working; dynamic lookup is always a bit subtle and hard to grep 
for. (llvm-cs wouldn't know about the current use, for example.) See 
https://reviews.llvm.org/D69848 for a rough sketch.

For robust crash handling, I figured on non-win we'd have a signal handler that 
on signal self-execs with a "actually, do use a cc1 process" and then use the 
existing crash machinery, but maybe that's not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D69791: [ARM,MVE] Add intrinsics for gather/scatter load/stores.

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

Looks good as far as I can see.




Comment at: clang/include/clang/Basic/arm_mve.td:78
+def _gather_base: Intrinsic<
+  Vector, (args VecOf>:$addr, imm_mem7bit:$offset),
+  (IRInt<"vldr_gather_base", [Vector, VecOf>]> $addr, 
$offset)>;

Is it worth giving a name to VecOf>?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69791



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

[PATCH] D69825: [Clang][Driver] Bypass cc1 process and re-enter driver main

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for the feedback @Meinersbur!

This patch is mainly geared towards Windows users. I'm not expecting anything 
on Linux, you already have all the bells & whistles there :-) Although I 
definitely see improvements on my Linux box. Would the distro make a 
difference? Mine is:

  $ uname -a
  Linux (name edited) 5.0.0-29-generic #31~18.04.1-Ubuntu SMP Thu Sep 12 
18:29:21 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux



In D69825#1733254 , @Meinersbur wrote:

> This patch reduced the build time from 12 to 7 minutes? Sounds too good to be 
> true.


Starting up and cooling down a large process like clang on Windows is very 
expensive. A fair amount of cpu time is spent in the clang driver, essentially 
in the kernel, in the range of 30 - 100 ms per process:
//(timings below are before this patch)//
F10630335: procmon-clang-cl.PNG 

We don't have `fork()` on Windows meaning that the kernel needs to re-start a 
the cc1 process from scratch, allocate pages, remap the EXE, allocate heap, 
allocate TLS, start the CRT, etc. Also, `InitLLVM::InitLLVM` is expensive 
because it calls into `dbghelp.dll` and loads symbols - just that can sometimes 
take up to 10 ms per process (depending on the system load).
In addition, recent Windows builds have all sorts of kernel regressions related 
to process creation and virtual pages allocation. Bruce Dawson 
 has 
several blog entries about all this.
We're simply mitigating these issues. Upgrading the 36-core server to Windows 
10 build 1903 will probably decrease the gap (12 min -> 7 min). However I would 
still expect users of Clang on pre-1903 builds for a few years from now.

I will fix the other issues you've mentioned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D68969: [clang-format] Remove the dependency on frontend

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D68969#1733946 , @thakis wrote:

> The revert message said:
>
>   Vlad Tsyrklevich via cfe-commits 
>   Tue, Oct 29, 1:51 PM (7 days ago)
>   to via, mydeveloperday
>  
>   I've reverted this commit as it was causing UBSan failures on the ubsan 
> bot. These failures looked like:
>   llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: pointer index 
> expression with base 0x overflowed to 0xfffa
>  
>   Looking at a backtrace, this line was reached from the 
> `Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));` call 
> introduced in this change.
>
>
> Is that enough to see what's wrong? If not, @vlad.tsyrklevich , can you give 
> repro steps for that ubsan failure that made you revert ec66603 in efed314 
> ?


Note to self: Took me a bit to set up a docker container to build with clang-10 
on linux (I'm normally a windows guy), but hopefully, I can debug from here...

  test1.cpp:1:7: warning: code should be clang-formatted 
[-Wclang-format-violations]
  /llvm-project/llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: applying 
non-zero offset 1844674407
  3709551610 to null pointer
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
/buildareas/llvm2/llvm-project/llvm/lib/Support/SourceMgr.cpp:44
  0:48 in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68969



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 59795 tests passed, 21 failed and 762 were skipped.

  failed: lld.ELF/linkerscript/filename-spec.s
  failed: Clang.Index/index-module-with-vfs.m
  failed: Clang.Modules/double-quotes.m
  failed: Clang.Modules/framework-public-includes-private.m
  failed: Clang.VFS/external-names.c
  failed: Clang.VFS/framework-import.m
  failed: Clang.VFS/implicit-include.c
  failed: Clang.VFS/include-mixed-real-and-virtual.c
  failed: Clang.VFS/include-real-from-virtual.c
  failed: Clang.VFS/include-virtual-from-real.c
  failed: Clang.VFS/include.c
  failed: Clang.VFS/incomplete-umbrella.m
  failed: Clang.VFS/module-import.m
  failed: Clang.VFS/module_missing_vfs.m
  failed: Clang.VFS/real-path-found-first.m
  failed: Clang.VFS/relative-path.c
  failed: Clang.VFS/test_nonmodular.c
  failed: Clang.VFS/umbrella-framework-import-skipnonexist.m
  failed: Clang.VFS/vfsroot-include.c
  failed: Clang.VFS/vfsroot-module.m
  failed: Clang.VFS/vfsroot-with-overlay.c

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263



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

[PATCH] D69615: [clangd] Implement a function to lex the file to find candidate occurrences.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 59741 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69615



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

[clang] 7b710a4 - [OPENMP]Improve diagnostics for unsupported unified addressing.

[Alexey Bataev via cfe-commits]

Author: Alexey Bataev
Date: 2019-11-05T10:31:59-05:00
New Revision: 7b710a4294c1baed0157d86d3e2dabac78c306ce

URL: 
https://github.com/llvm/llvm-project/commit/7b710a4294c1baed0157d86d3e2dabac78c306ce
DIFF: 
https://github.com/llvm/llvm-project/commit/7b710a4294c1baed0157d86d3e2dabac78c306ce.diff

LOG: [OPENMP]Improve diagnostics for unsupported unified addressing.

Improved diagnostics for better user experience.

Added: 


Modified: 
clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
clang/test/OpenMP/requires_codegen.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp 
b/clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
index eab2d7be1aeb..96716c0edd9f 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -4954,7 +4954,8 @@ void CGOpenMPRuntimeNVPTX::checkArchForUnifiedAddressing(
 const OMPRequiresDecl *D) {
   for (const OMPClause *Clause : D->clauselists()) {
 if (Clause->getClauseKind() == OMPC_unified_shared_memory) {
-  switch (getCudaArch(CGM)) {
+  CudaArch Arch = getCudaArch(CGM);
+  switch (Arch) {
   case CudaArch::SM_20:
   case CudaArch::SM_21:
   case CudaArch::SM_30:
@@ -4966,10 +4967,14 @@ void 
CGOpenMPRuntimeNVPTX::checkArchForUnifiedAddressing(
   case CudaArch::SM_53:
   case CudaArch::SM_60:
   case CudaArch::SM_61:
-  case CudaArch::SM_62:
-CGM.Error(Clause->getBeginLoc(),
-  "Target architecture does not support unified addressing");
+  case CudaArch::SM_62: {
+SmallString<256> Buffer;
+llvm::raw_svector_ostream Out(Buffer);
+Out << "Target architecture " << CudaArchToString(Arch)
+<< " does not support unified addressing";
+CGM.Error(Clause->getBeginLoc(), Out.str());
 return;
+  }
   case CudaArch::SM_70:
   case CudaArch::SM_72:
   case CudaArch::SM_75:

diff  --git a/clang/test/OpenMP/requires_codegen.cpp 
b/clang/test/OpenMP/requires_codegen.cpp
index e94fd28b419e..84821e89ed24 100644
--- a/clang/test/OpenMP/requires_codegen.cpp
+++ b/clang/test/OpenMP/requires_codegen.cpp
@@ -21,5 +21,5 @@
 #endif
 
 #ifdef REGION_DEVICE
-#pragma omp requires unified_shared_memory // expected-error {{Target 
architecture does not support unified addressing}} 
+#pragma omp requires unified_shared_memory // expected-error-re {{Target 
architecture sm_{{20|21|30|32|35|37|50|52|53|60|61|62}} does not support 
unified addressing}}
 #endif



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

[PATCH] D69825: [Clang][Driver] Bypass cc1 process and re-enter driver main

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D69825#1733958 , @thakis wrote:

> Thanks for sending this out!
>
> Instead of the dynamic lookup of that symbol, what do you think about passing 
> in the function via a normal api? That way, the type checker and linker help 
> us keep things working; dynamic lookup is always a bit subtle and hard to 
> grep for. (llvm-cs wouldn't know about the current use, for example.) See 
> https://reviews.llvm.org/D69848 for a rough sketch.


Absolutely, it make more sense, I can't remember why I used the dynamic lookup 
:-( I have this patch lying around since last year.

> For robust crash handling, I figured on non-win we'd have a signal handler 
> that on signal self-execs with a "actually, do use a cc1 process" and then 
> use the existing crash machinery, but maybe that's not needed.

Right now on Windows I'm doing about that, not in the exception handler, but a 
bit later on in `clang/lib/Driver/Driver.cpp, L1338`. The worse that can happen 
would be a memory corruption, which would crash 
`Driver::generateCompilationDiagnostics()` and prevent displaying the 
preprocessed file and the cmd-line. But you would still see the callstack 
(which is rendered now in the SEH, see 
`llvm/lib/Support/CrashRecoveryContext.cpp, L194` - I still need to hook that 
up for the posix implementation). Ideally we could render some of these things 
in advance (or pre-allocate memory at least) to minimize the risk of a crash 
when displaying crash diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 227888.
Charusso marked 2 inline comments as done.
Charusso retitled this revision from "[analyzer][WIP] CERTStrChecker: Model 
gets()" to "[analyzer] CERTStrChecker: Model gets()".
Charusso added a comment.

- Do not try to fix-it an array with offsets.


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

https://reviews.llvm.org/D69813

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cert/str31-alloc.cpp
  clang/test/Analysis/cert/str31-notes.cpp
  clang/test/Analysis/cert/str31-safe.cpp
  clang/test/Analysis/cert/str31-unsafe.cpp

Index: clang/test/Analysis/cert/str31-unsafe.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-unsafe.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+// The following is not defined therefore the safe functions are unavailable.
+// #define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // CHECK-FIXES: if (fgets(buf, sizeof(buf), stdin)) {}
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {}
+}
+} // namespace test_gets_good
Index: clang/test/Analysis/cert/str31-safe.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-safe.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // CHECK-FIXES: if (gets_s(buf, sizeof(buf))) {}
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (gets_s(buff, sizeof(buff))) {}
+}
+} // namespace test_gets_good
Index: clang/test/Analysis/cert/str31-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-notes.cpp
@@ -0,0 +1,31 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -analyzer-output=text -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+// The following is not defined therefore the safe functions are unavailable.
+// #define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+void *malloc(size_t size);
+void free(void *memblock);
+
+void func(void) {
+  unsigned size = 13;
+  // expected-note@-1 {{'size' initialized to 13}}
+
+  char *buf = (char *)malloc(size);
+  // expected-note@-1 {{Memory is allocated}}
+
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // expected-note@-2 {{'gets' could write outside of 'buf'}}
+
+  // CHECK-FIXES: if (fgets(buf, size, stdin)) {}
+  free(buf);
+}
Index: clang/test/Analysis/cert/str31-alloc.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-all

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, 
gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim.
Herald added a subscriber: jholewinski.
Herald added projects: clang, LLVM.

The new OpenMPConstants.h is a location for all OpenMP related constants
(and helpers) to live.

This patch moves the directives there (the enum OpenMPDirectiveKind) and
rewires Clang to use the new location.

Initially part of D69785 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// 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
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/IR/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/IR/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+const char *llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/include/llvm/IR/OpenMPKinds.def
===
--- /dev/null
+++ llvm/include/llvm/IR/OpenMPKinds.def
@@ -0,0 +1,101 @@
+//===--- OpenMPKinds.def - OpenMP directives, clauses, rt-calls -*- C++ -*-===//
+//
+// 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
+//
+//===--===//
+/// \file
+///
+/// This file defines the list of supported OpenMP directives, clauses, runtime
+/// calls, and other things that need to be listed in enums.
+///
+//===--===//
+
+/// OpenMP Directives and combined directives
+///
+///{
+
+#ifndef OMP_DIRECTIVE
+#define OMP_DIRECTIVE(Enum, Str)
+#endif
+
+#define __OMP_DIRECTIVE_EXT(Name, Str) OMP_DIRECTIVE(OMPD_##Name, Str)
+#define __OMP_DIRECTIVE(Name) __OMP_DIRECTIVE_EXT(Name, #Name)
+
+__OMP_DIRECTIVE(threadprivate)
+__OMP_DIRECTIVE(parallel)
+__OMP_DIRECTIVE(task)
+__OMP_DIRECTIVE(simd)
+__OMP_DIRECTIVE(for)
+__OMP_DIRECTIVE(sections)
+__OMP_DIRECTIVE(section)
+__OMP_DIRECTIVE(single)
+__OMP_DIRECTIVE(master)
+__OMP_DIRECTIVE(critical)
+__OMP_DIRECTIVE(taskyield)
+__OMP_DIRECTIVE(barrier)
+__OMP_DIRECTIVE(taskwait)
+__OMP_DIRECTIVE(taskgroup)
+__OMP_DIRECTIVE(flush)
+__OMP_DIRECTIVE(ordered)
+__OMP_DIRECTIVE(atomic)
+__OMP_DIRECTIVE(target)
+__OMP_DIRECTIVE(teams)
+__OMP_DIRECTIVE(cancel)
+__OMP_DIRECTIVE(requires)
+__OMP_DIRECTIVE_EXT(target_data, "target data")
+__OMP_DIRECTIVE_EXT(target_enter_data, "target enter data")
+__OMP_DIRECTIVE_EXT(target_exit_data, "target exit data")
+__OMP_DIRECTIVE_EXT(target_parallel, "target parallel")
+__OMP_DIRECTIVE_EXT(target_parallel_for, "target parallel for")
+__OMP_DIRECTIVE_EXT(target_update, "target update")
+__OMP_DIRECTIVE_EXT(parallel_for, "parallel for")
+__OMP_DIRECTIVE_EXT(parallel_for_simd, "parallel for simd")
+__OMP_DIRECTIVE_EXT(parallel_sections, "parallel sections")
+__OMP_DIRECTIVE_EXT(for_simd, "for simd")
+__OMP_DIRECTIVE_EXT(cancellation_point, "cancellation point")
+__OMP_DIRE

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227891.
jdoerfert marked 5 inline comments as done.
jdoerfert added a comment.

Split D69853  out and changed according to 
(most) comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,14 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 
+#define DEBUG_TYPE "openmp-ir-builder"
+
 using namespace llvm;
 using namespace omp;
 
@@ -32,3 +38,204 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RTLFnKind FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext &Ctx = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext &Ctx = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ unsigned LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable *&DefaultIdent = IdentMap[{SrcLocStr, LocFlags}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null, ConstantInt::get(Int32, LocFlags),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable &GV : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = &GV;
+
+DefaultIdent = new GlobalVariable(M, IdentPtr->getPoi

[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 227892.
Charusso added a comment.

- Support `alloca()`.


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

https://reviews.llvm.org/D69813

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cert/str31-alloc.cpp
  clang/test/Analysis/cert/str31-notes.cpp
  clang/test/Analysis/cert/str31-safe.cpp
  clang/test/Analysis/cert/str31-unsafe.cpp

Index: clang/test/Analysis/cert/str31-unsafe.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-unsafe.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+// The following is not defined therefore the safe functions are unavailable.
+// #define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // CHECK-FIXES: if (fgets(buf, sizeof(buf), stdin)) {}
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {}
+}
+} // namespace test_gets_good
Index: clang/test/Analysis/cert/str31-safe.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-safe.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // CHECK-FIXES: if (gets_s(buf, sizeof(buf))) {}
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (gets_s(buff, sizeof(buff))) {}
+}
+} // namespace test_gets_good
Index: clang/test/Analysis/cert/str31-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-notes.cpp
@@ -0,0 +1,31 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+// RUN:  -analyzer-output=text -I %S
+
+// See the examples on the page of STR31:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+// The following is not defined therefore the safe functions are unavailable.
+// #define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+void *malloc(size_t size);
+void free(void *memblock);
+
+void func(void) {
+  unsigned size = 13;
+  // expected-note@-1 {{'size' initialized to 13}}
+
+  char *buf = (char *)malloc(size);
+  // expected-note@-1 {{Memory is allocated}}
+
+  if (gets(buf)) {}
+  // expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  // expected-note@-2 {{'gets' could write outside of 'buf'}}
+
+  // CHECK-FIXES: if (fgets(buf, size, stdin)) {}
+  free(buf);
+}
Index: clang/test/Analysis/cert/str31-alloc.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-alloc.cpp
@@ -0,0 +1,89 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:  -analyzer-checker=core,unix,security.cert.Str \
+// RUN:  -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \
+/

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

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

Be consistent wrt. enum classes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,13 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
 
 using namespace llvm;
 using namespace omp;
@@ -32,3 +39,205 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext &Ctx = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext &Ctx = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ IdentFlag LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable *&DefaultIdent = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null,
+ ConstantInt::get(Int32, uint64_t(LocFlags)),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable &GV : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = &GV;
+
+DefaultIdent = new GlobalVa

[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki updated this revision to Diff 227894.
miyuki added a comment.

Changed getRawEncoding -> getHashValue in Sema.h


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

https://reviews.llvm.org/D69840

Files:
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Edit/EditedSource.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Tooling/Syntax/Tokens.h
  clang/lib/ARCMigrate/TransGCAttrs.cpp
  clang/lib/ARCMigrate/TransProperties.cpp
  clang/lib/ARCMigrate/Transforms.h
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/Edit/EditedSource.cpp
  clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
  clang/lib/Tooling/Syntax/Tokens.cpp

Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -297,7 +297,7 @@
  Collector->PP.getSourceManager().isBeforeInTranslationUnit(
  Range.getBegin(), LastExpansionEnd)))
   return;
-Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd();
+Collector->Expansions[Range.getBegin()] = Range.getEnd();
 LastExpansionEnd = Range.getEnd();
   }
   // FIXME: handle directives like #pragma, #include, etc.
@@ -524,7 +524,7 @@
   auto L = File.SpelledTokens[NextSpelled].location();
   if (Offset <= SM.getFileOffset(L))
 return llvm::None; // reached the offset we are looking for.
-  auto Mapping = CollectedExpansions.find(L.getRawEncoding());
+  auto Mapping = CollectedExpansions.find(L);
   if (Mapping != CollectedExpansions.end())
 return Mapping->second; // found a mapping before the offset.
 }
Index: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
===
--- clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
+++ clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
@@ -44,13 +44,13 @@
   bool ShowLineMarkers; ///< Show #line markers.
   bool UseLineDirectives; ///< Use of line directives or line markers.
   /// Tracks where inclusions that change the file are found.
-  std::map FileIncludes;
+  std::map FileIncludes;
   /// Tracks where inclusions that import modules are found.
-  std::map ModuleIncludes;
+  std::map ModuleIncludes;
   /// Tracks where inclusions that enter modules (in a module build) are found.
-  std::map ModuleEntryIncludes;
+  std::map ModuleEntryIncludes;
   /// Tracks where #if and #elif directives get evaluated and whether to true.
-  std::map IfConditions;
+  std::map IfConditions;
   /// Used transitively for building up the FileIncludes mapping over the
   /// various \c PPCallbacks callbacks.
   SourceLocation LastInclusionLocation;
@@ -65,7 +65,7 @@
   void detectMainFileEOL();
   void handleModuleBegin(Token &Tok) {
 assert(Tok.getKind() == tok::annot_module_begin);
-ModuleEntryIncludes.insert({Tok.getLocation().getRawEncoding(),
+ModuleEntryIncludes.insert({Tok.getLocation(),
 (Module *)Tok.getAnnotationValue()});
   }
 private:
@@ -164,7 +164,7 @@
 return;
   FileID Id = FullSourceLoc(Loc, SM).getFileID();
   auto P = FileIncludes.insert(
-  std::make_pair(LastInclusionLocation.getRawEncoding(),
+  std::make_pair(LastInclusionLocation,
  IncludedFile(Id, NewFileType, PP.GetCurDirLookup(;
   (void)P;
   assert(P.second && "Unexpected revisitation of the same include directive");
@@ -199,8 +199,7 @@
const Module *Imported,
SrcMgr::CharacteristicKind FileType){
   if (Imported) {
-auto P = ModuleIncludes.insert(
-std::make_pair(HashLoc.getRawEncoding(), Imported));
+auto P = ModuleIncludes.insert(std::make_pair(HashLoc, Imported));
 (void)P;
 assert(P.second && "Unexpected revisitation of the same include directive");
   } else
@@ -209,8 +208,7 @@
 
 void InclusionRewriter::If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind ConditionValue) {
-  auto P = IfConditions.insert(
-  std::make_pair(Loc.getRawEncoding(), ConditionValue == CVK_True));
+  auto P = IfConditions.insert(std::make_pair(Loc, ConditionValue == CVK_True));
   (void)P;
   assert(P.second && "Unexpected revisitation of the same if directive");
 }
@@ -218,8 +216,7 @@
 void InclusionRewriter::Elif(SourceLocation Loc, SourceRange ConditionRange,
  ConditionValueKind ConditionValue,
  SourceLocation IfLoc) {
-  auto P = IfConditions.insert(
-  std::make_pair(Loc.getRawEncoding(), ConditionValue == CVK_True));
+  auto P = IfConditions.insert(std::make_pair(Loc, ConditionValue == CVK_True));
   (void)P;
   assert(P.second && "Unexpected revisitation of the same elif directive");
 }
@@ -228,7 +225,7 @@
 /// an inclusion directive) in the map o

[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

What's the motivation behind this?


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

https://reviews.llvm.org/D69840



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

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:408
 OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
   assert(CKind <= OMPC_unknown);

Why assert is removed?



Comment at: clang/lib/Parse/ParseOpenMP.cpp:51-60
+// Helper to unify the enum class OpenMPDirectiveKind with its extension
+// OpenMPDirectiveKindEx.
+struct OpenMPDirectiveKindExWrapper {
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKind DK) : Value(unsigned(DK)) {}
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKindEx DKE)
+  : Value(unsigned(DKE)) {}
+  operator unsigned() const { return Value; }

Why do we need this?



Comment at: llvm/include/llvm/IR/OpenMPConstants.h:40
+/// Return a textual representation of the directive \p D.
+const char *getOpenMPDirectiveName(Directive D);
+

1. Better to return StringRef, I think.
2. Do we really need these 2 convert functions here? Are we going to use them 
in LLVM or just in clang?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

Maybe just `-fopenmp-experimental`?



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:25
+/// Each OpenMP directive has a corresponding public generator method.
+struct OpenMPIRBuilder {
+

class?



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
+

Do we need a new `Builder` here or we can reuse the one from clang 
CodeGenFunction?



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:52
+  ///should be checked and acted upon.
+  void createOMPBarrier(const LocationDescription &Loc, omp::Directive DK,
+bool CheckCancelFlag = true);

Do you really need to spell it as `createOMPBarrier`? Maybe, just 
`createBarrier` since it is already a member of OMPBuilder.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:62
+  /// Return the (LLVM-IR) string describing the source location \p LocStr.
+  Constant *getOrCreateSrcLocStr(std::string LocStr);
+

StringRef?



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:178
+
+auto FnDecl = getOrCreateRuntimeFunction(OMPRTL___kmpc_global_thread_num);
+Instruction *Call =

`Function *` instead of `auto`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki added a comment.

The motivation is to be able to make source locations' underlying type 
configurable. Richard Smith suggested that this might be feasible: 
http://lists.llvm.org/pipermail/cfe-dev/2019-October/063515.html
So, the first step is to get rid of getRawEncoding where possible. I think this 
would make the code cleaner anyway.


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

https://reviews.llvm.org/D69840



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

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: clang/lib/Basic/OpenMPKinds.cpp:408
 OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
   assert(CKind <= OMPC_unknown);

ABataev wrote:
> Why assert is removed?
I can add it back, I mean it would look like this now:
  `assert(unsigned(DKind) <= unsigned(OMPD_unknown));`

(I thought that the idea of enum classes is not to need it but we can keep it).



Comment at: clang/lib/Parse/ParseOpenMP.cpp:51-60
+// Helper to unify the enum class OpenMPDirectiveKind with its extension
+// OpenMPDirectiveKindEx.
+struct OpenMPDirectiveKindExWrapper {
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKind DK) : Value(unsigned(DK)) {}
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKindEx DKE)
+  : Value(unsigned(DKE)) {}
+  operator unsigned() const { return Value; }

ABataev wrote:
> Why do we need this?
First, I'm open for suggestions on how to do this in a nicer way :)

Why I did it:
The code below uses two enums as if they were unsigned numbers and one of them 
is now an enum class 
which requires you to do explicit casts. Without this trickery here we would 
need to sprinkle `unsigned()` in the code below, especially the array `F` 
which I did not want to do.




Comment at: llvm/include/llvm/IR/OpenMPConstants.h:40
+/// Return a textual representation of the directive \p D.
+const char *getOpenMPDirectiveName(Directive D);
+

ABataev wrote:
> 1. Better to return StringRef, I think.
> 2. Do we really need these 2 convert functions here? Are we going to use them 
> in LLVM or just in clang?
1. Can do, I just moved them in this patch.
2) We will reuse them in Flang so they should be moved (and it seems natural to 
keep these where the enums are defined.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



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

[PATCH] D68969: [clang-format] Remove the dependency on frontend

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

see D69854: [clang-format] [RELAND] Remove the dependency on frontend 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68969



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

[PATCH] D69854: [clang-format] [RELAND] Remove the dependency on frontend

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: thakis, vlad.tsyrklevich, klimek, 
mitchell-stellar.
MyDeveloperDay added projects: clang-format, clang.
Herald added a subscriber: mgorny.

relanding D68969: [clang-format] Remove the dependency on frontend 
 after it failed UBSAN build caused by the 
passing of an invalid SMLoc() (nullptr)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69854

Files:
  clang/tools/clang-format/CMakeLists.txt
  clang/tools/clang-format/ClangFormat.cpp


Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -18,7 +18,6 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/Format/Format.h"
-#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
@@ -300,12 +299,9 @@
   IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
   DiagOpts->ShowColors = (ShowColors && !NoShowColors);
 
-  TextDiagnosticPrinter *DiagsBuffer =
-  new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts, false);
-
   IntrusiveRefCntPtr DiagID(new DiagnosticIDs());
   IntrusiveRefCntPtr Diags(
-  new DiagnosticsEngine(DiagID, &*DiagOpts, DiagsBuffer));
+  new DiagnosticsEngine(DiagID, &*DiagOpts));
 
   IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
@@ -314,24 +310,40 @@
   FileID FileID = createInMemoryFile(AssumedFileName, Code.get(), Sources,
  Files, InMemoryFileSystem.get());
 
-  const unsigned ID = Diags->getCustomDiagID(
-  WarningsAsErrors ? clang::DiagnosticsEngine::Error
-   : clang::DiagnosticsEngine::Warning,
-  "code should be clang-formatted [-Wclang-format-violations]");
+  FileManager &FileMgr = Sources.getFileManager();
+  llvm::ErrorOr FileEntryPtr =
+  FileMgr.getFile(AssumedFileName);
 
   unsigned Errors = 0;
-  DiagsBuffer->BeginSourceFile(LangOptions(), nullptr);
   if (WarnFormat && !NoWarnFormat) {
 for (const auto &R : Replaces) {
-  Diags->Report(
-  Sources.getLocForStartOfFile(FileID).getLocWithOffset(R.getOffset()),
-  ID);
+  PresumedLoc PLoc = Sources.getPresumedLoc(
+  
Sources.getLocForStartOfFile(FileID).getLocWithOffset(R.getOffset()));
+
+  SourceLocation LineBegin =
+  Sources.translateFileLineCol(FileEntryPtr.get(), PLoc.getLine(), 1);
+  SourceLocation NextLineBegin = Sources.translateFileLineCol(
+  FileEntryPtr.get(), PLoc.getLine() + 1, 1);
+
+  const char *StartBuf = Sources.getCharacterData(LineBegin);
+  const char *EndBuf = Sources.getCharacterData(NextLineBegin);
+
+  StringRef Line(StartBuf, (EndBuf - StartBuf) - 1);
+
+  SMDiagnostic Diags(
+  llvm::SourceMgr(), SMLoc::getFromPointer(StartBuf), AssumedFileName,
+  PLoc.getLine(), PLoc.getColumn(),
+  WarningsAsErrors ? SourceMgr::DiagKind::DK_Error
+   : SourceMgr::DiagKind::DK_Warning,
+  "code should be clang-formatted [-Wclang-format-violations]", Line,
+  ArrayRef>());
+
+  Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));
   Errors++;
   if (ErrorLimit && Errors >= ErrorLimit)
 break;
 }
   }
-  DiagsBuffer->EndSourceFile();
   return WarningsAsErrors;
 }
 
Index: clang/tools/clang-format/CMakeLists.txt
===
--- clang/tools/clang-format/CMakeLists.txt
+++ clang/tools/clang-format/CMakeLists.txt
@@ -7,7 +7,6 @@
 set(CLANG_FORMAT_LIB_DEPS
   clangBasic
   clangFormat
-  clangFrontend
   clangRewrite
   clangToolingCore
   )


Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -18,7 +18,6 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/Format/Format.h"
-#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
@@ -300,12 +299,9 @@
   IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
   DiagOpts->ShowColors = (ShowColors && !NoShowColors);
 
-  TextDiagnosticPrinter *DiagsBuffer =
-  new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts, false);
-
   IntrusiveRefCntPtr DiagID(new DiagnosticIDs());
   IntrusiveRefCntPtr Diags(
-  new DiagnosticsEngine(DiagID, &*DiagOpts, DiagsBuffer));
+  new DiagnosticsEngine(DiagID, &*DiagOpts));
 
   IntrusiveRefCntPtr InMemoryFileSystem(
   new llvm::vfs::InMemoryFileSystem);
@@ -314,24 +310,40 @@
   FileID FileID = c

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227900.
jdoerfert marked 6 inline comments as done.
jdoerfert added a comment.

Minor changes according to comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,13 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
 
 using namespace llvm;
 using namespace omp;
@@ -32,3 +39,206 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext &Ctx = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext &Ctx = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ IdentFlag LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable *&DefaultIdent = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null,
+ ConstantInt::get(Int32, uint64_t(LocFlags)),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable &GV : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = &GV;
+
+DefaultIdent = new Glob

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

ABataev wrote:
> Maybe just `-fopenmp-experimental`?
I would prefer the option to be self explanatory but I'm not married to the 
current name.



Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
+

ABataev wrote:
> Do we need a new `Builder` here or we can reuse the one from clang 
> CodeGenFunction?
If you have a "simple" way to do it, we can think about it but I am still 
unsure if that is actually useful. The clang (=frontend) builder is used for 
callbacks so user code is build with it either way. We could set up ours here 
differently if we wish to and I'm a little afraid we would generate some 
unwanted interactions.

That being said, I tried to reuse the one in clang but struggled *a long time* 
to make it work. The problem is that it is a templated class with Clang 
specific template parameters. We would need to make this a template class as 
well (I think) and that comes with a long tail of problems.




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:52
+  ///should be checked and acted upon.
+  void createOMPBarrier(const LocationDescription &Loc, omp::Directive DK,
+bool CheckCancelFlag = true);

ABataev wrote:
> Do you really need to spell it as `createOMPBarrier`? Maybe, just 
> `createBarrier` since it is already a member of OMPBuilder.
> Do you really need to spell it as createOMPBarrier? Maybe, just createBarrier 
> since it is already a member of OMPBuilder.

fair point. I will rename if no one objects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

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

make it a class (NFC)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPIRBuilder.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- llvm/lib/IR/OpenMPIRBuilder.cpp
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -5,13 +5,20 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
+/// \file
+///
+/// This file implements the OpenMPIRBuilder class, which is used as a
+/// convenient way to create LLVM instructions for OpenMP directives.
+///
 //===--===//
 
-#include "llvm/IR/OpenMPConstants.h"
+#include "llvm/IR/OpenMPIRBuilder.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/IRBuilder.h"
+
+#define DEBUG_TYPE "openmp-ir-builder"
 
 using namespace llvm;
 using namespace omp;
@@ -32,3 +39,206 @@
   }
   llvm_unreachable("Invalid OpenMP directive kind");
 }
+
+Function *OpenMPIRBuilder::getOrCreateRuntimeFunction(RuntimeFunction FnID) {
+  Function *Fn = nullptr;
+
+  // Try to find the declation in the module first.
+  switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = M.getFunction(Str);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+
+  if (!Fn) {
+
+// Create a new declaration if we need one.
+switch (FnID) {
+#define OMP_RTL(Enum, Str, IsVarArg, ReturnType, ...)  \
+  case Enum:   \
+Fn = Function::Create(FunctionType::get(ReturnType,\
+ArrayRef{__VA_ARGS__}, \
+IsVarArg), \
+  GlobalValue::ExternalLinkage, Str, M);   \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+assert(Fn && "Failed to create OpenMP runtime function");
+
+LLVMContext &Ctx = Fn->getContext();
+// Add attributes to the new declaration.
+switch (FnID) {
+#define OMP_RTL_ATTRS(Enum, FnAttrSet, RetAttrSet, ArgAttrSets)\
+  case Enum:   \
+Fn->setAttributes( \
+AttributeList::get(Ctx, FnAttrSet, RetAttrSet, ArgAttrSets));  \
+break;
+#include "llvm/IR/OpenMPKinds.def"
+default:
+  // Attributes are optional.
+  break;
+}
+  }
+
+  return Fn;
+}
+
+void OpenMPIRBuilder::initialize() {
+  LLVMContext &Ctx = M.getContext();
+
+  // Create all simple and struct types exposed by the runtime and remember the
+  // llvm::PointerTypes of them for easy access later.
+  Type *T;
+#define OMP_TYPE(VarName, InitValue) this->VarName = InitValue;
+#define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
+  T = M.getTypeByName(StructName); \
+  if (!T)  \
+T = StructType::create(Ctx, {__VA_ARGS__}, StructName);\
+  this->VarName = PointerType::getUnqual(T);
+#include "llvm/IR/OpenMPKinds.def"
+}
+
+Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr,
+ IdentFlag LocFlags) {
+  // Enable "C-mode".
+  LocFlags |= OMP_IDENT_FLAG_KMPC;
+
+  GlobalVariable *&DefaultIdent = IdentMap[{SrcLocStr, uint64_t(LocFlags)}];
+  if (!DefaultIdent) {
+Constant *I32Null = ConstantInt::getNullValue(Int32);
+Constant *IdentData[] = {I32Null,
+ ConstantInt::get(Int32, uint64_t(LocFlags)),
+ I32Null, I32Null, SrcLocStr};
+Constant *Initializer = ConstantStruct::get(
+cast(IdentPtr->getPointerElementType()), IdentData);
+
+// Look for existing encoding of the location + flags, not needed but
+// minimizes the difference to the existing solution while we transition.
+for (GlobalVariable &GV : M.getGlobalList())
+  if (GV.getType() == IdentPtr && GV.hasInitializer())
+if (GV.getInitializer() == Initializer)
+  return DefaultIdent = &GV;
+
+DefaultIdent = new GlobalVariable(M, IdentPtr->getPointerElementType(),
+

[PATCH] D69855: Fix llvm-namespace-comment for macro expansions

[Marcin Twardak via Phabricator via cfe-commits]

twardakm created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
twardakm edited the summary of this revision.
twardakm added a project: clang-tools-extra.

If a namespace is a macro name, it should be allowed to close the
namespace with the same name.

This commit adds also unit tests for llvm-namespace-comment.

https://bugs.llvm.org/show_bug.cgi?id=26274


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69855

Files:
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
  clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s llvm-namespace-comment %t
+
+namespace n1 {
+namespace n2 {
+  void f();
+
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace n1
+
+#define MACRO macro_expansion
+namespace MACRO {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: namespace 'macro_expansion' not terminated with a closing comment [llvm-namespace-comment]
+}
+// CHECK-FIXES: } // namespace macro_expansion
+
+namespace MACRO {
+  void g();
+} // namespace MACRO
+
+namespace MACRO {
+  void h();
+} // namespace macro_expansion
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -26,6 +26,10 @@
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+
+  void addMacro(const std::string &Name, const std::string &Value) noexcept;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
@@ -34,6 +38,10 @@
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
   llvm::SmallVector Ends;
+
+  // Store macros to verify that warning is not thrown when namespace name is a
+  // preprocessed define
+  std::vector> Macros;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -19,6 +19,44 @@
 namespace tidy {
 namespace readability {
 
+namespace {
+class NamespaceCommentPPCallbacks : public PPCallbacks {
+public:
+  NamespaceCommentPPCallbacks(Preprocessor *PP, NamespaceCommentCheck *Check)
+  : PP(PP), Check(Check) {}
+
+  void MacroDefined(const Token &MacroNameTok, const MacroDirective *MD) {
+// Record all defined macros. We store the whole token to compare names
+// later
+
+const MacroInfo *const MI = MD->getMacroInfo();
+
+if (MI->isFunctionLike())
+  return;
+
+std::string ValueBuffer;
+llvm::raw_string_ostream Value(ValueBuffer);
+
+SmallString<128> SpellingBuffer;
+bool First = true;
+for (const auto &T : MI->tokens()) {
+  if (!First && T.hasLeadingSpace())
+Value << ' ';
+
+  Value << PP->getSpelling(T, SpellingBuffer);
+  First = false;
+}
+
+Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(),
+Value.str());
+  }
+
+private:
+  Preprocessor *PP;
+  NamespaceCommentCheck *Check;
+};
+} // namespace
+
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
  ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
@@ -40,6 +78,11 @@
 Finder->addMatcher(namespaceDecl().bind("namespace"), this);
 }
 
+void NamespaceCommentCheck::registerPPCallbacks(
+const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique(PP, this));
+}
+
 static bool locationsInSameFile(const SourceManager &Sources,
 SourceLocation Loc1, SourceLocation Loc2) {
   return Loc1.isFileID() && Loc2.isFileID() &&
@@ -65,6 +108,11 @@
   return Fix;
 }
 
+void NamespaceCommentCheck::addMacro(const std::string &Name,
+ const std::strin

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Artem Belevich via Phabricator via cfe-commits]

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

Thank you!




Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:1-2
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -fcuda-is-device \
+// RUN: -emit-llvm -x hip %s -o - | FileCheck %s
+#include "Inputs/cuda.h"

Perhaps we should add host-side test, too to make sure the pointers there do 
remain generic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826



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

[PATCH] D69791: [ARM,MVE] Add intrinsics for gather/scatter load/stores.

[Dave Green via Phabricator via cfe-commits]

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

Nice. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69791



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

[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

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

Szelethus added a comment.

Hmm, so this checker is rather a collection of CERT rule checkers, right? 
Shouldn't the checker name contain the actual rule name (STR31-C)? User 
interfacewise, I would much prefer smaller, leaner checkers than a big one with 
a lot of options, which are barely supported for end-users. I would expect a 
`cert` package to contain subpackages like `cert.str`, and checker names 
`cert.str.31StringSize`, or similar. Also, shouldn't we move related checkers 
from `security.insecureAPI` to `cert`? Or just mention the rule name in the 
description, and continue not having a  `cert` package?




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:70
 def Security : Package <"security">;
+def CERT : Package<"cert">, ParentPackage;
 def InsecureAPI : Package<"insecureAPI">, ParentPackage;

Hmm. We have a variety of checkers that check for a CERT rule. Maybe we should 
put the rest here as well, if would better follow the clang-tidy interface.

I'll ask around in the office.



Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:29
+/// \returns The MallocBugVisitor.
+std::unique_ptr getMallocBRVisitor(SymbolRef Sym);
+

I would prefer if this header file didn't exist, or was thought out better, 
because its messy that we hide `MallocChecker`, but expose its guts like this.

The change is fine, this is just a critique of the checker.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:184
+  if (IsFix) {
+if (Optional SizeStr = getSizeExprAsString(Call, CallC, C)) {
+  renameFunctionFix(UseSafeFunctions ? "gets_s" : "fgets", Call, *Report);

Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > Also, which is probably more important, you will never be able to 
> > > > provide a fixit for the malloced memory case, because there may be 
> > > > multiple execution paths that reach the current point with different 
> > > > size expressions (in fact, not necessarily all of them are malloced).
> > > > 
> > > > Eg.:
> > > > ```lang=c
> > > > char *x = 0;
> > > > char y[10];
> > > > 
> > > > if (coin()) {
> > > >   x = malloc(20);
> > > > } else {
> > > >   x = y;
> > > > }
> > > > 
> > > > gets(x);
> > > > ```
> > > > 
> > > > If you suggest replacing `gets(x)` with `gets_s(x, 20)`, you'll still 
> > > > have a buffer overflow on the else-branch on which `x` points to an 
> > > > array of 10 bytes.
> > > This checker going to evolve a lot, and all of the checked function calls 
> > > have issues like that. I do not even think what else issues they have. I 
> > > would like to cover the false alarm suppression when we are about to 
> > > alarm. Is it would be okay? I really would like to see alarms first.
> > > 
> > > For example, I have seen stuff in the wild so that I can state out 
> > > 8-param allocators and we need to rely on the checkers provide 
> > > information about allocation.
> > *summons @Szelethus*
> > 
> > Apart from the obviously syntactic cases, you might actually be able to 
> > implement fixits for the situation when the reaching-definitions analysis 
> > displays exactly one definition for `x`, which additionally coincides with 
> > the allocation site. If that definition is a simple assignment, you'll be 
> > able to re-run the reaching definitions analysis for the RHS of that 
> > assignment. If that definition comes from a function call, you might be 
> > able to re-run the reaching definitions analysis on the return statement(s) 
> > of that function (note that this function must have been inlined during 
> > path-sensitive analysis, otherwise no definition in it would coincide with 
> > the allocation site). And so on.
> > 
> > This problem sheds some light on how much do we want to make the reaching 
> > definitions analysis inter-procedural. My current guess is that we probably 
> > don't need to; we'd rather have this guided by re-running the 
> > reaching-definitions analysis based on the path-sensitive report data, than 
> > have the reaching-definitions analysis be inter-procedural on our own.
> That is a cool idea! I hope @Szelethus has time for his project.
This sounds very cool! Once we're at the bug report construction phase, we can 
make reaching definitions analysis "interprocedural enough" for most cases, I 
believe.


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

https://reviews.llvm.org/D69813



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Also, I think it would better to split LLVM part and clang part into separate 
patches.




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
+

jdoerfert wrote:
> ABataev wrote:
> > Do we need a new `Builder` here or we can reuse the one from clang 
> > CodeGenFunction?
> If you have a "simple" way to do it, we can think about it but I am still 
> unsure if that is actually useful. The clang (=frontend) builder is used for 
> callbacks so user code is build with it either way. We could set up ours here 
> differently if we wish to and I'm a little afraid we would generate some 
> unwanted interactions.
> 
> That being said, I tried to reuse the one in clang but struggled *a long 
> time* to make it work. The problem is that it is a templated class with Clang 
> specific template parameters. We would need to make this a template class as 
> well (I think) and that comes with a long tail of problems.
> 
You can make this class a template and instantiate it with the type of the 
CodeGenFunction IRBuilder and pass it by reference in the constructor. But only 
if it really worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+if (!Index)
+  return NoIndexProvided;
+

hokein wrote:
> ilya-biryukov wrote:
> > Why isn't this a scope enum in the first place?
> this is tricky, the reason why I put it here and another copy below is to 
> avoid regression of local rename (keep existing tests passed), if we move it 
> in the first place, the error message of local rename may be changed, e.g. 
> when renaming an external symbol (we know that from AST) without Index 
> support, we'd like to return "used outside of the main file" rather than "no 
> index provided".
> 
> thinking more about this, the no-index case is making our code complicated 
> (within-file rename with no-index, cross-file rename with no-index), I 
> believe the no-index case is not important, and is rare in practice, we could 
> change this behavior, or assume the index is always provided, this would help 
> to simplify the code, WDYT?
Agree, we shouldn't bother about the no-index case too much.
My comment here was referring to the lack of `ReasonToReject::` qualifier of 
the name.

Maybe make `ReasonToReject` a scoped enum, i.e. `enum class ReasonToReject` 
rather than `enum ReasonToReject`?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa(RenameDecl))

hokein wrote:
> ilya-biryukov wrote:
> > Why not a blacklist? I'd expect us to blacklist:
> > - things with custom names (operators, constructors, etc)
> > - virtual methods
> > 
> > Probably some things are missing from the list, but fundamentally most that 
> > have names are pretty similar, right?
> I believe our plan is to support major kinds of symbols (class, functions, 
> enums, typedef, alias) , I feel scary to allow renaming nearly all 
> `NamedDecl` (in theory, it should work but we're uncertain, for our 
> experience of local rename, it sometimes leads to very surprising results). 
> Having a whitelist can lead us to a better state, these symbols are 
> **officially supported**.
I disagree here, I don't see how `NamedDecl` is special if its name is an 
identifier unless we something smart about it.
My recollection is that local rename issues come from the fact that there are 
certain kinds of references, which are not handled in the corresponding visitor.

In any case, I'd suggest blacklisting symbols we know are broken, e.g. IIRC we 
don't index references to namespaces, so they're definitely broken. And finding 
the issues with the rest of the symbols and fixing those.
In practice, this should improve both rename and cross-references - whenever we 
have broken cross-file rename, there's a good chance that the indexer is also 
broken.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast(&RenameDecl)) {
+// FIXME: renaming virtual methods requires to rename all overridens in
+// subclasses, which our index doesn't support yet.

hokein wrote:
> ilya-biryukov wrote:
> > Isn't the same true for local rename?
> no, actually the local rename does handle this, it finds all overriden 
> methods from the AST, and updates all of them when doing rename.
Do we properly check overriden methods are not used outside the main file, 
though?
We might be assuming rename is "local" because there are not usages of the 
original function, but there may still be usages of the overriden function.

Worth adding a comment that local rename handles that through the AST.
Also useful to keep in mind that this is a regression, compared to the local 
rename (i.e. if we switch to cross-file rename by default, we'll break this 
use-case)



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}

hokein wrote:
> ilya-biryukov wrote:
> > Again, it looks like the function returns incorrect results and the callers 
> > should actually handle the case where `SymbolID` cannot be obtained in a 
> > custom manner.
> > 
> > Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the 
> > callers handle this case gracefully?
> hmm, is this critical? I think it is safe to assume getSymbolID should always 
> succeed,  I believe we make this assumption in other code parts in clangd as 
> well.
Should we maybe assert it succeeds instead?
If we make this assumption in the code, why not make it explicit?
If not, we should probably add tests where it fails.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+// !positionToOffset is O(N), it is okay at the moment since we only
+// process at most 100 references.
+auto RangeOffset = toRangeOffset(Occurrence, InitialCode);

hokein wrote:
> ilya-biryukov wrote:
> > This is not true anymore, right?
> > We should probably try getting to `O(

[PATCH] D69854: [clang-format] [RELAND] Remove the dependency on frontend

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: fail - 33803 tests passed, 1 failed and 462 were skipped.

  failed: LLVM.Object/macho-invalid.test

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69854



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

[PATCH] D69760: [Clang][Driver] Don't pun -fuse-ld=lld as -fuse-ld=lld-link with msvc

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

> I think a better distinguisher would be whether clang was invoked as 
> clang[++] or clang-cl (i.e. driver mode).

I think Martin said most of what I wanted to say. Making this dependent on the 
driver mode seems reasonable and would be the easier way forward.

If you still want to make `clang-cl -fuse-ld=lld` call `ld.lld` with gnu-style 
flags, you will need to pre-fix the various in-tree test suites. If you look 
through the LLDB, compiler-rt, and debuginfo-tests test suites, you will see 
many instances of `-fuse-ld=lld` that rely on the current behavior. See cases 
like:
https://github.com/llvm/llvm-project/blob/dd0c00b5f8b303f78565ab407d37a643e99a9e75/compiler-rt/test/asan/lit.cfg.py#L96
Maybe that case in particular doesn't apply, but it needs to be looked into.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69760



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

[clang] 15140e4 - [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via cfe-commits]

Author: Michael Liao
Date: 2019-11-05T13:05:05-05:00
New Revision: 15140e4bacf94fbc509e5a139909aefcd1cc3363

URL: 
https://github.com/llvm/llvm-project/commit/15140e4bacf94fbc509e5a139909aefcd1cc3363
DIFF: 
https://github.com/llvm/llvm-project/commit/15140e4bacf94fbc509e5a139909aefcd1cc3363.diff

LOG: [hip] Enable pointer argument lowering through coercing type.

Reviewers: tra, rjmccall, yaxunl

Subscribers: jvesely, nhaehnle, cfe-commits

Tags: #clang

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

Added: 
clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu

Modified: 
clang/lib/CodeGen/CGCall.cpp
clang/lib/CodeGen/TargetInfo.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 62e8fa037013..e832e4c28334 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -1305,6 +1305,15 @@ static void CreateCoercedStore(llvm::Value *Src,
 DstTy = Dst.getType()->getElementType();
   }
 
+  llvm::PointerType *SrcPtrTy = llvm::dyn_cast(SrcTy);
+  llvm::PointerType *DstPtrTy = llvm::dyn_cast(DstTy);
+  if (SrcPtrTy && DstPtrTy &&
+  SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) {
+Src = CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(Src, DstTy);
+CGF.Builder.CreateStore(Src, Dst, DstIsVolatile);
+return;
+  }
+
   // If the source and destination are integer or pointer types, just do an
   // extension or truncation to the desired type.
   if ((isa(SrcTy) || isa(SrcTy)) &&

diff  --git a/clang/lib/CodeGen/TargetInfo.cpp 
b/clang/lib/CodeGen/TargetInfo.cpp
index e33d69c86b3c..26c527d7c983 100644
--- a/clang/lib/CodeGen/TargetInfo.cpp
+++ b/clang/lib/CodeGen/TargetInfo.cpp
@@ -7685,6 +7685,42 @@ class AMDGPUABIInfo final : public DefaultABIInfo {
   bool isHomogeneousAggregateSmallEnough(const Type *Base,
  uint64_t Members) const override;
 
+  // Coerce HIP pointer arguments from generic pointers to global ones.
+  llvm::Type *coerceKernelArgumentType(llvm::Type *Ty, unsigned FromAS,
+   unsigned ToAS) const {
+// Structure types.
+if (auto STy = dyn_cast(Ty)) {
+  SmallVector EltTys;
+  bool Changed = false;
+  for (auto T : STy->elements()) {
+auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+EltTys.push_back(NT);
+Changed |= (NT != T);
+  }
+  // Skip if there is no change in element types.
+  if (!Changed)
+return STy;
+  if (STy->hasName())
+return llvm::StructType::create(
+EltTys, (STy->getName() + ".coerce").str(), STy->isPacked());
+  return llvm::StructType::get(getVMContext(), EltTys, STy->isPacked());
+}
+// Arrary types.
+if (auto ATy = dyn_cast(Ty)) {
+  auto T = ATy->getElementType();
+  auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+  // Skip if there is no change in that element type.
+  if (NT == T)
+return ATy;
+  return llvm::ArrayType::get(NT, ATy->getNumElements());
+}
+// Single value types.
+if (Ty->isPointerTy() && Ty->getPointerAddressSpace() == FromAS)
+  return llvm::PointerType::get(
+  cast(Ty)->getElementType(), ToAS);
+return Ty;
+  }
+
 public:
   explicit AMDGPUABIInfo(CodeGen::CodeGenTypes &CGT) :
 DefaultABIInfo(CGT) {}
@@ -7812,14 +7848,22 @@ ABIArgInfo 
AMDGPUABIInfo::classifyKernelArgumentType(QualType Ty) const {
 
   // TODO: Can we omit empty structs?
 
-  // Coerce single element structs to its element.
+  llvm::Type *LTy = nullptr;
   if (const Type *SeltTy = isSingleElementStruct(Ty, getContext()))
-return ABIArgInfo::getDirect(CGT.ConvertType(QualType(SeltTy, 0)));
+LTy = CGT.ConvertType(QualType(SeltTy, 0));
+
+  if (getContext().getLangOpts().HIP) {
+if (!LTy)
+  LTy = CGT.ConvertType(Ty);
+LTy = coerceKernelArgumentType(
+LTy, /*FromAS=*/getContext().getTargetAddressSpace(LangAS::Default),
+/*ToAS=*/getContext().getTargetAddressSpace(LangAS::cuda_device));
+  }
 
   // If we set CanBeFlattened to true, CodeGen will expand the struct to its
   // individual elements, which confuses the Clover OpenCL backend; therefore 
we
   // have to set it to false here. Other args of getDirect() are just defaults.
-  return ABIArgInfo::getDirect(nullptr, 0, nullptr, false);
+  return ABIArgInfo::getDirect(LTy, 0, nullptr, false);
 }
 
 ABIArgInfo AMDGPUABIInfo::classifyArgumentType(QualType Ty,

diff  --git a/clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu 
b/clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
new file mode 100644
index ..cb8a75882d4d
--- /dev/null
+++ b/clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -fcuda-is-device -emit-llvm -x 
hip %s -o - | FileCheck %s
+// RUN: %clang_cc1

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via Phabricator via cfe-commits]

hliao updated this revision to Diff 227908.
hliao marked an inline comment as done.
hliao added a comment.

Add host-side checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu

Index: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
===
--- /dev/null
+++ clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -fcuda-is-device -emit-llvm -x hip %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -x hip %s -o - | FileCheck -check-prefix=HOST %s
+
+#include "Inputs/cuda.h"
+
+// Coerced struct from `struct S` without all generic pointers lowered into
+// global ones.
+// CHECK: %struct.S.coerce = type { i32 addrspace(1)*, float addrspace(1)* }
+// CHECK: %struct.T.coerce = type { [2 x float addrspace(1)*] }
+
+// On the host-side compilation, generic pointer won't be coerced.
+// HOST-NOT: %struct.S.coerce
+// HOST-NOT: %struct.T.coerce
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel1Pi(i32 addrspace(1)* %x.coerce)
+// HOST: define void @_Z7kernel1Pi.stub(i32* %x)
+__global__ void kernel1(int *x) {
+  x[0]++;
+}
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel2Ri(i32 addrspace(1)* dereferenceable(4) %x.coerce)
+// HOST: define void @_Z7kernel2Ri.stub(i32* dereferenceable(4) %x)
+__global__ void kernel2(int &x) {
+  x++;
+}
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel3PU3AS2iPU3AS1i(i32 addrspace(2)* %x, i32 addrspace(1)* %y)
+// HOST: define void @_Z7kernel3PU3AS2iPU3AS1i.stub(i32 addrspace(2)* %x, i32 addrspace(1)* %y)
+__global__ void kernel3(__attribute__((address_space(2))) int *x,
+__attribute__((address_space(1))) int *y) {
+  y[0] = x[0];
+}
+
+// CHECK: define void @_Z4funcPi(i32* %x)
+__device__ void func(int *x) {
+  x[0]++;
+}
+
+struct S {
+  int *x;
+  float *y;
+};
+// `by-val` struct will be coerced into a similar struct with all generic
+// pointers lowerd into global ones.
+// CHECK: define amdgpu_kernel void @_Z7kernel41S(%struct.S.coerce %s.coerce)
+// HOST: define void @_Z7kernel41S.stub(i32* %s.coerce0, float* %s.coerce1)
+__global__ void kernel4(struct S s) {
+  s.x[0]++;
+  s.y[0] += 1.f;
+}
+
+// If a pointer to struct is passed, only the pointer itself is coerced into the global one.
+// CHECK: define amdgpu_kernel void @_Z7kernel5P1S(%struct.S addrspace(1)* %s.coerce)
+// HOST: define void @_Z7kernel5P1S.stub(%struct.S* %s)
+__global__ void kernel5(struct S *s) {
+  s->x[0]++;
+  s->y[0] += 1.f;
+}
+
+struct T {
+  float *x[2];
+};
+// `by-val` array is also coerced.
+// CHECK: define amdgpu_kernel void @_Z7kernel61T(%struct.T.coerce %t.coerce)
+// HOST: define void @_Z7kernel61T.stub(float* %t.coerce0, float* %t.coerce1)
+__global__ void kernel6(struct T t) {
+  t.x[0][0] += 1.f;
+  t.x[1][0] += 2.f;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7685,6 +7685,42 @@
   bool isHomogeneousAggregateSmallEnough(const Type *Base,
  uint64_t Members) const override;
 
+  // Coerce HIP pointer arguments from generic pointers to global ones.
+  llvm::Type *coerceKernelArgumentType(llvm::Type *Ty, unsigned FromAS,
+   unsigned ToAS) const {
+// Structure types.
+if (auto STy = dyn_cast(Ty)) {
+  SmallVector EltTys;
+  bool Changed = false;
+  for (auto T : STy->elements()) {
+auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+EltTys.push_back(NT);
+Changed |= (NT != T);
+  }
+  // Skip if there is no change in element types.
+  if (!Changed)
+return STy;
+  if (STy->hasName())
+return llvm::StructType::create(
+EltTys, (STy->getName() + ".coerce").str(), STy->isPacked());
+  return llvm::StructType::get(getVMContext(), EltTys, STy->isPacked());
+}
+// Arrary types.
+if (auto ATy = dyn_cast(Ty)) {
+  auto T = ATy->getElementType();
+  auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+  // Skip if there is no change in that element type.
+  if (NT == T)
+return ATy;
+  return llvm::ArrayType::get(NT, ATy->getNumElements());
+}
+// Single value types.
+if (Ty->isPointerTy() && Ty->getPointerAddressSpace() == FromAS)
+  return llvm::PointerType::get(
+  cast(Ty)->getElementType(), ToAS);
+return Ty;
+  }
+
 public:
   explicit AMDGPUABIInfo(CodeGen::CodeGenTypes &CGT) :
 DefaultABIInfo(CGT) {}
@@ -7812,14 +7848,22 @@
 
   // TODO: Can we omit empty structs?
 
-  // Coerce single element stru

[PATCH] D69615: [clangd] Implement a function to lex the file to find candidate occurrences.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:757
+  lex(Content, LangOpts, [&](const clang::Token &Tok, const SourceManager &SM) 
{
+if (Tok.getKind() == tok::raw_identifier)
+  if (Tok.getRawIdentifier() == Identifier)

NIT: [[ 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
 | use early exits ]] to keep the code flat.
```
if (Tok.getKind() != ...)
  return;
auto Range = ...;
if (!Range)
  return;
...
```



Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:690
+   void f() {
+ [[Foo]] foo;
+   }

hokein wrote:
> ilya-biryukov wrote:
> > Could you add a few more occurrences?
> > Maybe also with the weird things like multi-line tokens:
> > ```
> > F\
> > o\
> > o
> > ```
> Done, note that multi-line identifier is not supported -- `getRawIdentifier` 
> returns a literal name `F\o\o` which makes the check `Tok.getRawIdentifier() 
> == Identifier` fail. I think it is fine to not support it.
Yeah, it's probably ok.
I'd still add this as a test-case to make sure we don't crash and fix the 
behavior.

Although agree it's not very important, this is super rare.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69615



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

[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 227909.
Tyker added a comment.

@rsmith

Changes:

- Rebased on recent master.
- Adapted this patch to constexpr destructors.
- Fixed issues with handling of temporaries.
- Improve Tests.


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

https://reviews.llvm.org/D63960

Files:
  clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only -Wno-unused-value %s -verify
 
 namespace basic_sema {
 
@@ -12,6 +12,7 @@
 }
 
 constexpr auto l_eval = [](int i) consteval {
+// expected-note@-1+ {{declared here}}
 
   return i;
 };
@@ -23,11 +24,12 @@
 
 struct A {
   consteval int f1(int i) const {
+// expected-note@-1 {{declared here}}
 return i;
   }
   consteval A(int i);
   consteval A() = default;
-  consteval ~A() = default;
+  consteval ~A() = default; // expected-error {{destructor cannot be declared consteval}}
 };
 
 consteval struct B {}; // expected-error {{struct cannot be marked consteval}}
@@ -51,14 +53,296 @@
 struct D {
   C c;
   consteval D() = default; // expected-error {{cannot be consteval}}
-  consteval ~D() = default; // expected-error {{cannot be consteval}}
+  consteval ~D() = default; // expected-error {{destructor cannot be declared consteval}}
 };
 
-struct E : C { // expected-note {{here}}
-  consteval ~E() {} // expected-error {{cannot be declared consteval because base class 'basic_sema::C' does not have a constexpr destructor}}
+struct E : C {
+  consteval ~E() {} // expected-error {{cannot be declared consteval}}
 };
 }
 
 consteval int main() { // expected-error {{'main' is not allowed to be declared consteval}}
   return 0;
 }
+
+consteval int f_eval(int i) {
+// expected-note@-1+ {{declared here}}
+  return i;
+}
+
+namespace taking_address {
+
+using func_type = int(int);
+
+func_type* p1 = (&f_eval);
+// expected-error@-1 {{take address}}
+func_type* p7 = __builtin_addressof(f_eval);
+// expected-error@-1 {{take address}}
+
+auto p = f_eval;
+// expected-error@-1 {{take address}}
+
+auto m1 = &basic_sema::A::f1;
+// expected-error@-1 {{take address}}
+auto l1 = &decltype(basic_sema::l_eval)::operator();
+// expected-error@-1 {{take address}}
+
+consteval int f(int i) {
+// expected-note@-1+ {{declared here}}
+  return i;
+}
+
+auto ptr = &f;
+// expected-error@-1 {{take address}}
+
+auto f1() {
+  return &f;
+// expected-error@-1 {{take address}}
+}
+
+}
+
+namespace invalid_function {
+using size_t = unsigned long;
+struct A {
+  consteval void *operator new(size_t count);
+  // expected-error@-1 {{'operator new' cannot be declared consteval}}
+  consteval void *operator new[](size_t count);
+  // expected-error@-1 {{'operator new[]' cannot be declared consteval}}
+  consteval void operator delete(void* ptr);
+  // expected-error@-1 {{'operator delete' cannot be declared consteval}}
+  consteval void operator delete[](void* ptr);
+  // expected-error@-1 {{'operator delete[]' cannot be declared consteval}}
+  consteval ~A() {}
+  // expected-error@-1 {{destructor cannot be declared consteval}}
+};
+
+}
+
+namespace nested {
+consteval int f() {
+  return 0;
+}
+
+consteval int f1(...) {
+  return 1;
+}
+
+enum E {};
+
+using T = int(&)();
+
+consteval auto operator+ (E, int(*a)()) {
+  return 0;
+}
+
+void d() {
+  auto i = f1(E() + &f);
+}
+
+auto l0 = [](auto) consteval {
+  return 0;
+};
+
+int i0 = l0(&f1);
+
+int i1 = f1(l0(4));
+
+int i2 = f1(&f1, &f1, &f1, &f1, &f1, &f1, &f1);
+
+int i3 = f1(f1(f1(&f1, &f1), f1(&f1, &f1), f1(f1(&f1, &f1), &f1)));
+
+}
+
+namespace user_defined_literal {
+
+consteval int operator"" _test(unsigned long long i) {
+// expected-note@-1+ {{declared here}}
+  return 0;
+}
+
+int i = 0_test;
+
+auto ptr = &operator"" _test;
+// expected-error@-1 {{take address}}
+
+}
+
+namespace return_address {
+
+consteval int f() {
+  return 0;
+}
+
+consteval int(*ret1(int i))() {
+  return &f;
+}
+
+auto ptr = ret1(0);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{pointer to a consteval}}
+
+struct A {
+  consteval int f(int) {
+return 0;
+  }
+};
+
+using mem_ptr_type = int (A::*)(int);

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

[Caroline Tice via Phabricator via cfe-commits]

cmtice updated this revision to Diff 227910.
cmtice marked 9 inline comments as done.
cmtice added a comment.

Made requested changes:

- renamed option to be dwarf-specific
- fixed spelling & blank line issues
- only set version if emit-dwarf is true
- move test to Driver directory

I *think* I got it all done...


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

https://reviews.llvm.org/D69822

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/test/CodeGen/debug-default-version.c

Index: clang/test/CodeGen/debug-default-version.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-default-version.c
@@ -0,0 +1,47 @@
+// RUN: %clang -target x86_64-linux-gnu -fdebug-default-version=4 -gdwarf-2 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER2
+// RUN: %clang -target x86_64-linux-gnu -gdwarf-3 -fdebug-default-version=4 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER3
+// RUN: %clang -target x86_64-linux-gnu -gdwarf-4 -fdebug-default-version=2 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER4
+// RUN: %clang -target x86_64-linux-gnu -gdwarf-5 -S -fdebug-default-version=2 -emit-llvm -o - %s | FileCheck %s --check-prefix=VER5
+// RUN: %clang -target x86_64-linux-gnu -fdebug-default-version=5 -g -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER5
+// RUN: %clang -target x86_64-linux-gnu -gdwarf -fdebug-default-version=2 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER2
+// RUN: %clang -target x86_64-linux-gnu -fdebug-default-version=4 -S -emit-llvm -o - %s | FileCheck %s --check-prefixes=NODEBUGINFO,VER4
+
+// The -isysroot is used as a hack to avoid LIT messing with the SDKROOT
+// environment variable which indirecty overrides the version in the target
+// triple used here.
+// RUN: %clang -target x86_64-apple-macosx10.11 -g -S -emit-llvm -o - %s -isysroot %t | FileCheck %s --check-prefix=VER4
+// RUN: %clang -target x86_64-apple-darwin14 -g -S -emit-llvm -o - %s -isysroot %t | FileCheck %s --check-prefix=VER2
+// RUN: %clang -target x86_64-apple-macosx10.11 -fdebug-default-version=5 -g -S -emit-llvm -o - %s -isysroot %t | FileCheck %s --check-prefix=VER5
+// RUN: %clang -target x86_64-apple-darwin14 -g -fdebug-default-version=4 -S -emit-llvm -o - %s -isysroot %t | FileCheck %s --check-prefix=VER4
+
+// RUN: %clang -target powerpc-unknown-openbsd -fdebug-default-version=4 -g -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER4
+// RUN: %clang -target powerpc-unknown-freebsd -g -fdebug-default-version=4 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER4
+// RUN: %clang -target i386-pc-solaris -fdebug-default-version=4 -g -S -emit-llvm -o - %s | FileCheck %s --check-prefix=VER4
+
+// Check which debug info formats we use on Windows. By default, in an MSVC
+// environment, we should use codeview. You can enable dwarf, which implicitly
+// disables codeview, of you can explicitly ask for both if you don't know how
+// the app will be debugged.
+// RUN: %clang -target i686-pc-windows-msvc -fdebug-default-version=2 -gdwarf -S -emit-llvm -o - %s \
+// RUN: | FileCheck %s --check-prefixes=VER2,NOCODEVIEW
+// Explicitly request both.
+// RUN: %clang -target i686-pc-windows-msvc -fdebug-default-version=4 -gdwarf -gcodeview -S -emit-llvm -o - %s \
+// RUN: | FileCheck %s --check-prefixes=VER4,CODEVIEW
+
+int main (void) {
+  return 0;
+}
+
+// NODEBUGINFO-NOT: !llvm.dbg.cu = !{!0}
+
+// NOCODEVIEW-NOT: !"CodeView"
+
+// VER2: !{i32 2, !"Dwarf Version", i32 2}
+// VER3: !{i32 2, !"Dwarf Version", i32 3}
+// VER4: !{i32 2, !"Dwarf Version", i32 4}
+// VER5: !{i32 2, !"Dwarf Version", i32 5}
+
+// NODWARF-NOT: !"Dwarf Version"
+// CODEVIEW: !{i32 2, !"CodeView", i32 1}
+// NOCODEVIEW-NOT: !"CodeView"
+// NODWARF-NOT: !"Dwarf Version"
Index: clang/lib/Driver/ToolChains/CommonArgs.h
===
--- clang/lib/Driver/ToolChains/CommonArgs.h
+++ clang/lib/Driver/ToolChains/CommonArgs.h
@@ -68,6 +68,9 @@
 unsigned ParseFunctionAlignment(const ToolChain &TC,
 const llvm::opt::ArgList &Args);
 
+unsigned ParseDebugDefaultVersion(const ToolChain &TC,
+  const llvm::opt::ArgList &Args);
+
 void AddAssemblerKPIC(const ToolChain &ToolChain,
   const llvm::opt::ArgList &Args,
   llvm::opt::ArgStringList &CmdArgs);
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1138,6 +1138,22 @@
   return Value ? llvm::Log2_32_Ceil(std::min(Value, 65536u)) : Value;
 }
 
+unsigned tools::ParseDebugDefaultVersion(const ToolChain &TC,
+  

[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D69813#1734193 , @Szelethus wrote:

> Hmm, so this checker is rather a collection of CERT rule checkers, right? 
> Shouldn't the checker name contain the actual rule name (STR31-C)? User 
> interfacewise, I would much prefer smaller, leaner checkers than a big one 
> with a lot of options, which are barely supported for end-users. I would 
> expect a `cert` package to contain subpackages like `cert.str`, and checker 
> names `cert.str.31StringSize`, or similar.


It is the STR rules of CERT, nothing else. Most of the rules are tied together, 
and that is why the checker needs to be designed as one checker at first. I am 
not sure which part of the STR I will cover, so may when the checker evolves 
and some functions does not need the same helper methods we need to create new 
checkers. STR31 and STR32 are my projects which is like one single project. 
Also I did not except the users to specify the rule number, but this checker 
could be something like `cert.str.Termination`. There is two floating-point 
CERT checkers inside the `insecureAPI` that is why I have introduced the `cert` 
package, which will have three members, one is that new checker. I think a new 
package is only necessary if it contains at least two checkers.

> Also, shouldn't we move related checkers from `security.insecureAPI` to 
> `cert`? Or just mention the rule name in the description, and continue not 
> having a  `cert` package?

We should not, they does not fit into CERT rules, but it has two CERT 
floating-point checkers. The `cert` package should be well described with CERT 
rules. I want to move the CERT checkers from it into that `cert` package, and 
leave the rest.


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

https://reviews.llvm.org/D69813



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

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG15140e4bacf9: [hip] Enable pointer argument lowering through 
coercing type. (authored by hliao).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu

Index: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
===
--- /dev/null
+++ clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -fcuda-is-device -emit-llvm -x hip %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -x hip %s -o - | FileCheck -check-prefix=HOST %s
+
+#include "Inputs/cuda.h"
+
+// Coerced struct from `struct S` without all generic pointers lowered into
+// global ones.
+// CHECK: %struct.S.coerce = type { i32 addrspace(1)*, float addrspace(1)* }
+// CHECK: %struct.T.coerce = type { [2 x float addrspace(1)*] }
+
+// On the host-side compilation, generic pointer won't be coerced.
+// HOST-NOT: %struct.S.coerce
+// HOST-NOT: %struct.T.coerce
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel1Pi(i32 addrspace(1)* %x.coerce)
+// HOST: define void @_Z7kernel1Pi.stub(i32* %x)
+__global__ void kernel1(int *x) {
+  x[0]++;
+}
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel2Ri(i32 addrspace(1)* dereferenceable(4) %x.coerce)
+// HOST: define void @_Z7kernel2Ri.stub(i32* dereferenceable(4) %x)
+__global__ void kernel2(int &x) {
+  x++;
+}
+
+// CHECK: define amdgpu_kernel void  @_Z7kernel3PU3AS2iPU3AS1i(i32 addrspace(2)* %x, i32 addrspace(1)* %y)
+// HOST: define void @_Z7kernel3PU3AS2iPU3AS1i.stub(i32 addrspace(2)* %x, i32 addrspace(1)* %y)
+__global__ void kernel3(__attribute__((address_space(2))) int *x,
+__attribute__((address_space(1))) int *y) {
+  y[0] = x[0];
+}
+
+// CHECK: define void @_Z4funcPi(i32* %x)
+__device__ void func(int *x) {
+  x[0]++;
+}
+
+struct S {
+  int *x;
+  float *y;
+};
+// `by-val` struct will be coerced into a similar struct with all generic
+// pointers lowerd into global ones.
+// CHECK: define amdgpu_kernel void @_Z7kernel41S(%struct.S.coerce %s.coerce)
+// HOST: define void @_Z7kernel41S.stub(i32* %s.coerce0, float* %s.coerce1)
+__global__ void kernel4(struct S s) {
+  s.x[0]++;
+  s.y[0] += 1.f;
+}
+
+// If a pointer to struct is passed, only the pointer itself is coerced into the global one.
+// CHECK: define amdgpu_kernel void @_Z7kernel5P1S(%struct.S addrspace(1)* %s.coerce)
+// HOST: define void @_Z7kernel5P1S.stub(%struct.S* %s)
+__global__ void kernel5(struct S *s) {
+  s->x[0]++;
+  s->y[0] += 1.f;
+}
+
+struct T {
+  float *x[2];
+};
+// `by-val` array is also coerced.
+// CHECK: define amdgpu_kernel void @_Z7kernel61T(%struct.T.coerce %t.coerce)
+// HOST: define void @_Z7kernel61T.stub(float* %t.coerce0, float* %t.coerce1)
+__global__ void kernel6(struct T t) {
+  t.x[0][0] += 1.f;
+  t.x[1][0] += 2.f;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7685,6 +7685,42 @@
   bool isHomogeneousAggregateSmallEnough(const Type *Base,
  uint64_t Members) const override;
 
+  // Coerce HIP pointer arguments from generic pointers to global ones.
+  llvm::Type *coerceKernelArgumentType(llvm::Type *Ty, unsigned FromAS,
+   unsigned ToAS) const {
+// Structure types.
+if (auto STy = dyn_cast(Ty)) {
+  SmallVector EltTys;
+  bool Changed = false;
+  for (auto T : STy->elements()) {
+auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+EltTys.push_back(NT);
+Changed |= (NT != T);
+  }
+  // Skip if there is no change in element types.
+  if (!Changed)
+return STy;
+  if (STy->hasName())
+return llvm::StructType::create(
+EltTys, (STy->getName() + ".coerce").str(), STy->isPacked());
+  return llvm::StructType::get(getVMContext(), EltTys, STy->isPacked());
+}
+// Arrary types.
+if (auto ATy = dyn_cast(Ty)) {
+  auto T = ATy->getElementType();
+  auto NT = coerceKernelArgumentType(T, FromAS, ToAS);
+  // Skip if there is no change in that element type.
+  if (NT == T)
+return ATy;
+  return llvm::ArrayType::get(NT, ATy->getNumElements());
+}
+// Single value types.
+if (Ty->isPointerTy() && Ty->getPointerAddressSpace() == FromAS)
+  return llvm::PointerType::get(
+  cast(Ty)->getElementType(), ToAS);
+return Ty;
+  }
+
 public:
   explicit AMDGPUABIInfo(CodeGen::CodeGenTypes &CGT) :
 DefaultABIInfo(CGT) {}
@@ -7812,14 +7848,22 @@
 
   // TODO: C

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

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

In D69785#1734205 , @ABataev wrote:

> Also, I think it would better to split LLVM part and clang part into separate 
> patches.


What do you mean exactly and why?




Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:29
+  /// not have an effect on \p M (see initialize).
+  OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Do we need a new `Builder` here or we can reuse the one from clang 
> > > CodeGenFunction?
> > If you have a "simple" way to do it, we can think about it but I am still 
> > unsure if that is actually useful. The clang (=frontend) builder is used 
> > for callbacks so user code is build with it either way. We could set up 
> > ours here differently if we wish to and I'm a little afraid we would 
> > generate some unwanted interactions.
> > 
> > That being said, I tried to reuse the one in clang but struggled *a long 
> > time* to make it work. The problem is that it is a templated class with 
> > Clang specific template parameters. We would need to make this a template 
> > class as well (I think) and that comes with a long tail of problems.
> > 
> You can make this class a template and instantiate it with the type of the 
> CodeGenFunction IRBuilder and pass it by reference in the constructor. But 
> only if it really worth it.
That doesn't work as easily because the implementation is not part of this 
header, so we need an extern template and we'll open up a link nightmare that I 
would like to avoid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

FWIW, *I will enable the new pass in some tests before this goes in*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[PATCH] D69763: [Clang][Test]: Remaining "lld-link2" -> "lld-link"

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

In D69763#1733382 , @Ericson2314 wrote:

> I am curious, how did this work since there is no longer an `lld-link2`? Were 
> these tests failing or not being run?


These tests don't actually need to run the linker. They were constructing a 
command line to run "lld-link2", which didn't exist. Actually performing the 
link would've failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69763



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

[PATCH] D69673: [clangd] Implement semantic highlightings via findExplicitReferences

[Ilya Biryukov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG87e0cb4f1ad2: [clangd] Implement semantic highlightings via 
findExplicitReferences (authored by ilya-biryukov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69673

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -589,6 +589,15 @@
   R"cpp(
   void $Function[[foo]]();
   using ::$Function[[foo]];
+)cpp",
+  // Highlighting of template template arguments.
+  R"cpp(
+  template  class $TemplateParameter[[TT]],
+template  class ...$TemplateParameter[[TTs]]>
+  struct $Class[[Foo]] {
+$Class[[Foo]]<$TemplateParameter[[TT]], $TemplateParameter[[TTs]]...>
+  *$Field[[t]];
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "SemanticHighlighting.h"
+#include "FindTarget.h"
 #include "Logger.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
@@ -15,10 +16,16 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Casting.h"
 #include 
 
 namespace clang {
@@ -103,12 +110,12 @@
 return kindForDecl(TD);
   return llvm::None;
 }
-// Given a set of candidate declarations, if the declarations all have the same
-// highlighting kind, return that highlighting kind, otherwise return None.
-template 
-llvm::Optional kindForCandidateDecls(IteratorRange Decls) {
+
+llvm::Optional kindForReference(const ReferenceLoc &R) {
   llvm::Optional Result;
-  for (NamedDecl *Decl : Decls) {
+  for (const NamedDecl *Decl : R.Targets) {
+if (!canHighlightName(Decl->getDeclName()))
+  return llvm::None;
 auto Kind = kindForDecl(Decl);
 if (!Kind || (Result && Kind != Result))
   return llvm::None;
@@ -117,27 +124,49 @@
   return Result;
 }
 
-// Collects all semantic tokens in an ASTContext.
-class HighlightingTokenCollector
-: public RecursiveASTVisitor {
-  std::vector Tokens;
-  ParsedAST &AST;
-
+/// Consumes source locations and maps them to text ranges for highlightings.
+class HighlightingsBuilder {
 public:
-  HighlightingTokenCollector(ParsedAST &AST) : AST(AST) {}
-
-  std::vector collectTokens() {
-Tokens.clear();
-TraverseAST(AST.getASTContext());
-// Add highlightings for macro expansions as they are not traversed by the
-// visitor.
-for (const auto &M : AST.getMacros().Ranges)
-  Tokens.push_back({HighlightingKind::Macro, M});
+  HighlightingsBuilder(const SourceManager &SourceMgr,
+   const LangOptions &LangOpts)
+  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+
+  void addToken(HighlightingToken T) { Tokens.push_back(T); }
+
+  void addToken(SourceLocation Loc, HighlightingKind Kind) {
+if (Loc.isInvalid())
+  return;
+if (Loc.isMacroID()) {
+  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
+  if (!SourceMgr.isMacroArgExpansion(Loc))
+return;
+  Loc = SourceMgr.getSpellingLoc(Loc);
+}
+
+// Non top level decls that are included from a header are not filtered by
+// topLevelDecls. (example: method declarations being included from
+// another file for a class from another file).
+// There are also cases with macros where the spelling loc will not be in
+// the main file and the highlighting would be incorrect.
+if (!isInsideMainFile(Loc, SourceMgr))
+  return;
+
+auto Range = getTokenRange(SourceMgr, LangOpts, Loc);
+if (!Range) {
+  // R should always have a value, if it doesn't something is very wrong.
+  elog("Tried to add semantic token with an invalid range");
+  return;
+}
+Tokens.push_back(HighlightingToken{Kind, *Range});
+  }
+
+  std::vector collect() && {
 // Initializer lists can gi

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

I am a little bit concerned that user may have such code:

  struct A { int *p; }
  __global__ kernel(A a) {
int x;
a.p = &x;
f(a);
  }

@arsenm what happens if a private pointer is mis-used as a global pointer?

I am wondering if we should coerce byval struct kernel arg to global only if 
they are const, e.g.

  __global__ kernel(const A a);

I understand this may lose performance. Or should we introduce an option to let 
user disable coerce of non-const struct kernel arg to global.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826



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

[PATCH] D69858: [AArch64][SVE] Implement floating-point comparison & reduction intrinsics

[Kerry McLaughlin via Phabricator via cfe-commits]

kmclaughlin created this revision.
kmclaughlin added reviewers: sdesmalen, huntergr, dancgr, mgudim.
Herald added subscribers: psnobl, rkruppe, hiraditya, kristof.beyls, tschuett.
Herald added a project: LLVM.

Adds intrinsics for the following:

- fadda & faddv
- fminv, fmaxv, fminnmv & fmaxnmv
- facge & facgt
- fcmp[eq|ge|gt|ne|uo]


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69858

Files:
  llvm/include/llvm/IR/IntrinsicsAArch64.td
  llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
  llvm/lib/Target/AArch64/SVEInstrFormats.td
  llvm/test/CodeGen/AArch64/sve-intrinsics-fp-compares.ll
  llvm/test/CodeGen/AArch64/sve-intrinsics-fp-reduce.ll

Index: llvm/test/CodeGen/AArch64/sve-intrinsics-fp-reduce.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/sve-intrinsics-fp-reduce.ll
@@ -0,0 +1,214 @@
+; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve < %s | FileCheck %s
+
+;
+; FADDA
+;
+
+define half @fadda_f16( %pg, half %init,  %a) {
+; CHECK-LABEL: fadda_f16:
+; CHECK: fadda h0, p0, h0, z1.h
+; CHECK-NEXT: ret
+  %res = call half @llvm.aarch64.sve.fadda.nxv8f16( %pg,
+   half %init,
+%a)
+  ret half %res
+}
+
+define float @fadda_f32( %pg, float %init,  %a) {
+; CHECK-LABEL: fadda_f32:
+; CHECK: fadda s0, p0, s0, z1.s
+; CHECK-NEXT: ret
+  %res = call float @llvm.aarch64.sve.fadda.nxv4f32( %pg,
+float %init,
+ %a)
+  ret float %res
+}
+
+define double @fadda_f64( %pg, double %init,  %a) {
+; CHECK-LABEL: fadda_f64:
+; CHECK: fadda d0, p0, d0, z1.d
+; CHECK-NEXT: ret
+  %res = call double @llvm.aarch64.sve.fadda.nxv2f64( %pg,
+ double %init,
+  %a)
+  ret double %res
+}
+
+;
+; FADDV
+;
+
+define half @faddv_f16( %pg,  %a) {
+; CHECK-LABEL: faddv_f16:
+; CHECK: faddv h0, p0, z0.h
+; CHECK-NEXT: ret
+  %res = call half @llvm.aarch64.sve.faddv.nxv8f16( %pg,
+%a)
+  ret half %res
+}
+
+define float @faddv_f32( %pg,  %a) {
+; CHECK-LABEL: faddv_f32:
+; CHECK: faddv s0, p0, z0.s
+; CHECK-NEXT: ret
+  %res = call float @llvm.aarch64.sve.faddv.nxv4f32( %pg,
+ %a)
+  ret float %res
+}
+
+define double @faddv_f64( %pg,  %a) {
+; CHECK-LABEL: faddv_f64:
+; CHECK: faddv d0, p0, z0.d
+; CHECK-NEXT: ret
+  %res = call double @llvm.aarch64.sve.faddv.nxv2f64( %pg,
+  %a)
+  ret double %res
+}
+
+;
+; FMAXNMV
+;
+
+define half @fmaxnmv_f16( %pg,  %a) {
+; CHECK-LABEL: fmaxnmv_f16:
+; CHECK: fmaxnmv h0, p0, z0.h
+; CHECK-NEXT: ret
+  %res = call half @llvm.aarch64.sve.fmaxnmv.nxv8f16( %pg,
+  %a)
+  ret half %res
+}
+
+define float @fmaxnmv_f32( %pg,  %a) {
+; CHECK-LABEL: fmaxnmv_f32:
+; CHECK: fmaxnmv s0, p0, z0.s
+; CHECK-NEXT: ret
+  %res = call float @llvm.aarch64.sve.fmaxnmv.nxv4f32( %pg,
+   %a)
+  ret float %res
+}
+
+define double @fmaxnmv_f64( %pg,  %a) {
+; CHECK-LABEL: fmaxnmv_f64:
+; CHECK: fmaxnmv d0, p0, z0.d
+; CHECK-NEXT: ret
+  %res = call double @llvm.aarch64.sve.fmaxnmv.nxv2f64( %pg,
+%a)
+  ret double %res
+}
+
+;
+; FMAXV
+;
+
+define half @fmaxv_f16( %pg,  %a) {
+; CHECK-LABEL: fmaxv_f16:
+; CHECK: fmaxv h0, p0, z0.h
+; CHECK-NEXT: ret
+  %res = call half @llvm.aarch64.sve.fmaxv.nxv8f16( %pg,
+%a)
+  ret half %res
+}
+
+define float @fmaxv_f32( %pg,  %a) {
+; CHECK-LABEL: fmaxv_f32:
+; CHECK: fmaxv s0, p0, z0.s
+; CHECK-NEXT: ret
+  %res = call float @llvm.aarch64.sve.fmaxv.nxv4f32( %pg,
+ %a)
+  ret float %res
+}
+
+define double @fmaxv_f64( %pg,  %a) {
+; CHECK-LABEL: fmaxv_f64:
+; CHECK: fmaxv d0, p0, z0.d
+; CHECK-NEXT: ret
+  %res = call double @llvm.aarch64.sve.fmaxv.nxv2f64( %pg,
+  %a)
+  ret double %res
+}
+
+;
+; FMINNMV
+;
+
+define half @fminnmv_f16( %pg,  %a) {
+; CHECK-LABEL: fminnmv_f16:
+; CHECK: fminnmv h0, p0, z0.h
+; CHECK-NEXT: ret
+  %res = call half @llvm.aarch64.sve.fminnmv.nxv8f16( %pg,
+  %a)
+  ret half %res
+}
+
+define float @fminnmv_f32( %pg,  %a) {
+; CHECK-LABEL: fminnmv_f32:
+; CHECK: fminnmv s0, p0, z0.s
+; CHECK-NEXT: ret
+  %res = call float @llvm.aarch64.sve.fminnmv.nxv4f32( %pg,
+   %a)
+  ret float %res
+}
+
+define double @fminnmv_f64( %pg,  %a) {
+; CHECK-LABEL: fminnmv_f64:
+; CHECK: fminnmv d0, p0, z0.d
+; CHECK-

[PATCH] D69770: Add recoverable string parsing errors to APFloat

[Ehud Katz via Phabricator via cfe-commits]

ekatz marked 2 inline comments as done.
ekatz added inline comments.



Comment at: llvm/lib/Support/APFloat.cpp:273
+  if (p != end)
+return createError("Invalid exponent in exponent");
 

arsenm wrote:
> Error message sounds like nonsense 
It is actually incorrect. There should not be an error returned, but 
`absExponent` should still be clamped.



Comment at: llvm/unittests/ADT/APFloatTest.cpp:1322
+  EXPECT_EQ(convertToErrorFromString("+0x1.1p-"), "Exponent has no digits");
+  EXPECT_EQ(convertToErrorFromString("-0x1.1p-"), "Exponent has no digits");
 }

arsenm wrote:
> It’s a gtestism that these operands should be swapped
Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69770



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69785#1734292 , @jdoerfert wrote:

> In D69785#1734205 , @ABataev wrote:
>
> > Also, I think it would better to split LLVM part and clang part into 
> > separate patches.
>
>
> What do you mean exactly and why?


I just think it would be better to split this patch into 2 parts, one for LLVM 
builder (without tests, probably, or just with some kind of unittests) + 
another one for clang-related changes. It will reduce the size of the patches 
and all that stuff related to `keep patches smaller`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[clang-tools-extra] 87e0cb4 - [clangd] Implement semantic highlightings via findExplicitReferences

[Ilya Biryukov via cfe-commits]

Author: Ilya Biryukov
Date: 2019-11-05T19:15:24+01:00
New Revision: 87e0cb4f1ad299c87c3e26676a9b31b3caf58921

URL: 
https://github.com/llvm/llvm-project/commit/87e0cb4f1ad299c87c3e26676a9b31b3caf58921
DIFF: 
https://github.com/llvm/llvm-project/commit/87e0cb4f1ad299c87c3e26676a9b31b3caf58921.diff

LOG: [clangd] Implement semantic highlightings via findExplicitReferences

Summary:
To keep the logic of finding locations of interesting AST nodes in one
place.

The advantage is better coverage of various AST nodes, both now and in
the future: as new nodes get added to `findExplicitReferences`, semantic
highlighting will automatically pick them up.

The drawback of this change is that we have to traverse declarations
inside our file twice in order to highlight dependent names, 'auto'
and 'decltype'. Hopefully, this should not affect the actual latency
too much, most time should be spent in building the AST and not
traversing it.

Reviewers: hokein

Reviewed By: hokein

Subscribers: nridge, merge_guards_bot, MaskRay, jkorous, arphaman, kadircet, 
usaxena95, cfe-commits

Tags: #clang

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

Added: 


Modified: 
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/FindTarget.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/FindTarget.cpp 
b/clang-tools-extra/clangd/FindTarget.cpp
index 4e61d22dd7fb..c536cbf75e5c 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -732,6 +732,10 @@ void findExplicitReferences(const Decl *D,
   assert(D);
   ExplicitReferenceColletor(Out).TraverseDecl(const_cast(D));
 }
+void findExplicitReferences(const ASTContext &AST,
+llvm::function_ref Out) {
+  ExplicitReferenceColletor(Out).TraverseAST(const_cast(AST));
+}
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, DeclRelation R) {
   switch (R) {

diff  --git a/clang-tools-extra/clangd/FindTarget.h 
b/clang-tools-extra/clangd/FindTarget.h
index 9ddd24adb895..bd5d7690ceed 100644
--- a/clang-tools-extra/clangd/FindTarget.h
+++ b/clang-tools-extra/clangd/FindTarget.h
@@ -22,6 +22,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_FINDTARGET_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_FINDTARGET_H
 
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/Stmt.h"
@@ -107,6 +108,8 @@ void findExplicitReferences(const Stmt *S,
 llvm::function_ref Out);
 void findExplicitReferences(const Decl *D,
 llvm::function_ref Out);
+void findExplicitReferences(const ASTContext &AST,
+llvm::function_ref Out);
 
 /// Similar to targetDecl(), however instead of applying a filter, all possible
 /// decls are returned along with their DeclRelationSets.

diff  --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp 
b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index 62d3a164b5b8..9838600584c6 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "SemanticHighlighting.h"
+#include "FindTarget.h"
 #include "Logger.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
@@ -15,10 +16,16 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Casting.h"
 #include 
 
 namespace clang {
@@ -103,12 +110,12 @@ llvm::Optional kindForType(const Type 
*TP) {
 return kindForDecl(TD);
   return llvm::None;
 }
-// Given a set of candidate declarations, if the declarations all have the same
-// highlighting kind, return that highlighting kind, otherwise return None.
-template 
-llvm::Optional kindForCandidateDecls(IteratorRange Decls) {
+
+llvm::Optional kindForReference(const ReferenceLoc &R) {
   llvm::Optional Result;
-  for (NamedDecl *Decl : Decls) {
+  for (const NamedDecl *Decl : R.Targets) {
+if (!canHighlightName(Decl->getDeclName()))
+  return llvm::None;
 auto Kind = kindForDecl(Decl);
 if (!Kind || (Result && Kind != Result))
   return llvm::None;
@@ -117,27 +124,49 @@ llvm::Optional 
kindForCandidateDecls(IteratorRange Decls) {
   return Result;
 }
 
-// Collects all semantic tokens in an ASTContext.
-class HighlightingTokenCollector
-: 

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

In D69826#1734296 , @yaxunl wrote:

> I am a little bit concerned that user may have such code:
>
>   struct A { int *p; }
>   __global__ kernel(A a) {
> int x;
> a.p = &x;
> f(a);
>   }
>
>
> @arsenm what happens if a private pointer is mis-used as a global pointer?
>
> I am wondering if we should coerce byval struct kernel arg to global only if 
> they are const, e.g.
>
>   __global__ kernel(const A a);
>
>
> I understand this may lose performance. Or should we introduce an option to 
> let user disable coerce of non-const struct kernel arg to global.


This should not be a concern. The coercing is only applied to the parameter 
itself. Within the function body, we still use the original `struct A`. The 
preparation in function prolog will copy that coerced argument into the 
original one (alloca-ed.) The modification of that parameter later will be 
applied to the original one due to the by-val nature.

A modified version of your code is compiled into the following code at O0:

  define protected amdgpu_kernel void @_Z3foo1A(%struct.A.coerce %a.coerce) #0 {
  entry:
%a = alloca %struct.A, align 8, addrspace(5)
%a1 = addrspacecast %struct.A addrspace(5)* %a to %struct.A*
%x = alloca i32, align 4, addrspace(5)
%x.ascast = addrspacecast i32 addrspace(5)* %x to i32*
%agg.tmp = alloca %struct.A, align 8, addrspace(5)
%agg.tmp.ascast = addrspacecast %struct.A addrspace(5)* %agg.tmp to 
%struct.A*
%0 = bitcast %struct.A* %a1 to %struct.A.coerce*
%1 = getelementptr inbounds %struct.A.coerce, %struct.A.coerce* %0, i32 0, 
i32 0
%2 = extractvalue %struct.A.coerce %a.coerce, 0
store i32 addrspace(1)* %2, i32 addrspace(1)** %1, align 8
%3 = getelementptr inbounds %struct.A.coerce, %struct.A.coerce* %0, i32 0, 
i32 1
%4 = extractvalue %struct.A.coerce %a.coerce, 1
store i32 addrspace(1)* %4, i32 addrspace(1)** %3, align 8
%p = getelementptr inbounds %struct.A, %struct.A* %a1, i32 0, i32 0
store i32* %x.ascast, i32** %p, align 8
%5 = bitcast %struct.A* %agg.tmp.ascast to i8*
%6 = bitcast %struct.A* %a1 to i8*
call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %5, i8* align 8 %6, i64 
16, i1 false)
%7 = getelementptr inbounds %struct.A, %struct.A* %agg.tmp.ascast, i32 0, 
i32 0
%8 = load i32*, i32** %7, align 8
%9 = getelementptr inbounds %struct.A, %struct.A* %agg.tmp.ascast, i32 0, 
i32 1
%10 = load i32*, i32** %9, align 8
call void @_Z1f1A(i32* %8, i32* %10) #3
ret void
  }

The modification of parameter `a` is applied the alloca-ed one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826



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

[PATCH] D69770: Add recoverable string parsing errors to APFloat

[Ehud Katz via Phabricator via cfe-commits]

ekatz updated this revision to Diff 227917.
ekatz added a comment.

Fixed requested changes.


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

https://reviews.llvm.org/D69770

Files:
  clang/lib/Lex/LiteralSupport.cpp
  llvm/include/llvm/ADT/APFloat.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Support/APFloat.cpp
  llvm/lib/Support/StringRef.cpp
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
  llvm/unittests/ADT/APFloatTest.cpp

Index: llvm/unittests/ADT/APFloatTest.cpp
===
--- llvm/unittests/ADT/APFloatTest.cpp
+++ llvm/unittests/ADT/APFloatTest.cpp
@@ -10,8 +10,8 @@
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
-#include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -20,9 +20,17 @@
 
 using namespace llvm;
 
-static double convertToDoubleFromString(const char *Str) {
+static std::string convertToErrorFromString(StringRef Str) {
   llvm::APFloat F(0.0);
-  F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven);
+  auto ErrOrStatus =
+  F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven);
+  EXPECT_TRUE(!ErrOrStatus);
+  return toString(ErrOrStatus.takeError());
+}
+
+static double convertToDoubleFromString(StringRef Str) {
+  llvm::APFloat F(0.0);
+  EXPECT_FALSE(!F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven));
   return F.convertToDouble();
 }
 
@@ -1147,172 +1155,172 @@
   EXPECT_DEATH(APFloat(APFloat::IEEEsingle(), 0.0f).convertToDouble(), "Float semantics are not IEEEdouble");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), 0.0 ).convertToFloat(),  "Float semantics are not IEEEsingle");
 }
+#endif
+#endif
 
-TEST(APFloatTest, StringDecimalDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  ""), "Invalid string length");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+"), "String has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-"), "String has no digits");
+TEST(APFloatTest, StringDecimalError) {
+  EXPECT_EQ("Invalid string length", convertToErrorFromString(""));
+  EXPECT_EQ("String has no digits", convertToErrorFromString("+"));
+  EXPECT_EQ("String has no digits", convertToErrorFromString("-"));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("\0", 1)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1\0", 2)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1" "\0" "2", 3)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1" "\0" "2e1", 5)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e\0", 3)), "Invalid character in exponent");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e1\0", 4)), "Invalid character in exponent");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e1" "\0" "2", 5)), "Invalid character in exponent");
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("\0", 1)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1\0", 2)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1" "\0" "2", 3)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1" "\0" "2e1", 5)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e\0", 3)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e1\0", 4)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e1" "\0" "2", 5)));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0f"), "Invalid character in significand");
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString("1.0f"));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), ".."), "String contains multiple dots");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "..0"), "String contains multiple dots");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0.0"), "String contains multiple dots");
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString(".."));
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString("..0"));
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString("1.0.0"));
 }
 
-TEST(APFloatTest, StringDecimalSignificandDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "."), "Significand has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+."), "Significand has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-."), "Significand has no digits");
+TEST(APFloatTest, StringDecimalSignificandError) {
+  EXPECT_EQ("Significand has no digits", convertToErrorFromString( "."));
+  EXPEC

[PATCH] D69766: [Clang][MSVC] Use GetLinkerPath like the other toolchains for consistency

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69766



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

[PATCH] D63020: [HIP] Fix visibility for 'extern' device variables.

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

Sam, could you review this? Even though it has no functionality issue so far, 
from the code sequence, once there's an `addrspacecast` is inserted, we lose 
the chance to set target specific attributes if any.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63020



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

[PATCH] D69755: [LLD] Add NetBSD support as a new flavor of LLD (nb.lld)

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D69755#1731420 , @krytarowski wrote:

> In D69755#1731394 , @MaskRay wrote:
>
> > I still have the feeling that such configurations should be added to 
> > clangDriver/gcc specs or a shell script wrapper of lld.
>
>
> OK, as you allow now a shell wrapper of lld, this is a progress. nb.lld is a 
> C++ wrapper on steroids doing the right job and part of the upstream repo 
> (this is required).
>
> We do not allow downstream LLVM/Clang/... patches in our basesystem and 
> keeping this code upstream is required. Also from maintenance burden as LLVM 
> APIs evolve quickly.


Yes, by upstreaming you will share maintenance burden with the upstream.. but 
do we really need it? LLVM and Clang APIs and unstable and evolve quickly, but 
that is unrelated to the linker. I have reviewed the options added in this 
patch. These GNU ld options (with the exception or --no-rosegment and 
--image-base) have been so ingrained that I don't think the lld upstream can 
suddenly stop supporting some options or make disruptive changes that break the 
behaviors.

>> How do you decide to handle "Handling of indirect shared library 
>> dependencies"? It does not seem to be favored by lld contributors.
> 
> The modern Linux/GNU behavior is rejected in NetBSD and we keep the 
> traditional behavior that is considered by our community as desired.

Can you give links about the (1) rejection and (2) community needs? I am under 
the impression that some of the decisions are simply GNU hatred. I've mentioned 
in previous review comments that I don't like many GNU ELF things 
(STB_GNU_UNIQUE, STT_GNU_IFUNC, 4-byte Elf64_Nhdr fields, `strip/objcopy 
--strip-all` not stripping non-SHF_ALLOC, etc), some are bearable, because 
diverging the behavior may just cost more. However, for this particular 
`DT_RUNPATH` case, I don't really understand what it will cost if you follow 
the GNU behavior.

>// force-disable RO segment on NetBSD due to ld.elf_so limitations
>   args.push_back("--no-rosegment");

I hope you can fix the limitation soon. I just implemented the 
--only-keep-debug feature for llvm-objcopy. My observation is that 
`--no-rosegment` can make the debug file produced by --only-keep-debug larger 
(larger readonly sections such as .dynsym, .dynstr, .rodata and .eh_frame will 
occupy much space that can be eliminated). Mixing sections of different 
permission in the same RX segment does not look nice, either.

> This is another topic and indeed probably our next major target to get to 
> work within LLD.

I'm very concerned about this.. This traditional behavior will bring much 
complexity to the linker, while its usefulness is still very unclear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69755



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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1643
   HelpText<"Emit OpenMP code only for SIMD-based constructs.">;
+def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, 
Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
+  HelpText<"Use the experimental OpenMP-IR-Builder codegen path.">;

jdoerfert wrote:
> ABataev wrote:
> > Maybe just `-fopenmp-experimental`?
> I would prefer the option to be self explanatory but I'm not married to the 
> current name.
`enable-openmpirbuilder?`



Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG

Sharing constants between the compiler and the runtime is an interesting 
subproblem. I think the cleanest solution is the one used by libc, where the 
compiler generates header files containing the constants which the runtime 
library includes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785



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

[PATCH] D69826: [hip] Enable pointer argument lowering through coercing type.

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D69826#1734324 , @hliao wrote:

> In D69826#1734296 , @yaxunl wrote:
>
> > I am a little bit concerned that user may have such code:
> >
> >   struct A { int *p; }
> >   __global__ kernel(A a) {
> > int x;
> > a.p = &x;
> > f(a);
> >   }
> >
> >
> > @arsenm what happens if a private pointer is mis-used as a global pointer?
> >
> > I am wondering if we should coerce byval struct kernel arg to global only 
> > if they are const, e.g.
> >
> >   __global__ kernel(const A a);
> >
> >
> > I understand this may lose performance. Or should we introduce an option to 
> > let user disable coerce of non-const struct kernel arg to global.
>
>
> This should not be a concern. The coercing is only applied to the parameter 
> itself. Within the function body, we still use the original `struct A`. The 
> preparation in function prolog will copy that coerced argument into the 
> original one (alloca-ed.) The modification of that parameter later will be 
> applied to the original one due to the by-val nature.
>
> A modified version of your code is compiled into the following code at O0:
>
>   define protected amdgpu_kernel void @_Z3foo1A(%struct.A.coerce %a.coerce) 
> #0 {
>   entry:
> %a = alloca %struct.A, align 8, addrspace(5)
> %a1 = addrspacecast %struct.A addrspace(5)* %a to %struct.A*
> %x = alloca i32, align 4, addrspace(5)
> %x.ascast = addrspacecast i32 addrspace(5)* %x to i32*
> %agg.tmp = alloca %struct.A, align 8, addrspace(5)
> %agg.tmp.ascast = addrspacecast %struct.A addrspace(5)* %agg.tmp to 
> %struct.A*
> %0 = bitcast %struct.A* %a1 to %struct.A.coerce*
> %1 = getelementptr inbounds %struct.A.coerce, %struct.A.coerce* %0, i32 
> 0, i32 0
> %2 = extractvalue %struct.A.coerce %a.coerce, 0
> store i32 addrspace(1)* %2, i32 addrspace(1)** %1, align 8
> %3 = getelementptr inbounds %struct.A.coerce, %struct.A.coerce* %0, i32 
> 0, i32 1
> %4 = extractvalue %struct.A.coerce %a.coerce, 1
> store i32 addrspace(1)* %4, i32 addrspace(1)** %3, align 8
> %p = getelementptr inbounds %struct.A, %struct.A* %a1, i32 0, i32 0
> store i32* %x.ascast, i32** %p, align 8
> %5 = bitcast %struct.A* %agg.tmp.ascast to i8*
> %6 = bitcast %struct.A* %a1 to i8*
> call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %5, i8* align 8 %6, i64 
> 16, i1 false)
> %7 = getelementptr inbounds %struct.A, %struct.A* %agg.tmp.ascast, i32 0, 
> i32 0
> %8 = load i32*, i32** %7, align 8
> %9 = getelementptr inbounds %struct.A, %struct.A* %agg.tmp.ascast, i32 0, 
> i32 1
> %10 = load i32*, i32** %9, align 8
> call void @_Z1f1A(i32* %8, i32* %10) #3
> ret void
>   }
>
>
> The modification of parameter `a` is applied the alloca-ed one.


OK. Thanks for clarification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69826



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

[PATCH] D63020: [HIP] Fix visibility for 'extern' device variables.

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl accepted this revision.
yaxunl added a subscriber: scchan.
yaxunl added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks! @scchan This may fix the undefined symbol in work item struct 
issue at -O0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63020



___
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

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:173
+  }
+
+  // Builds of compiler-rt on bare-metal targets are specialized by specific

CodaFi wrote:
> phosek wrote:
> > Would it be possible to support the [per-target runtimes directory 
> > layout](https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChain.cpp#L391)
> >  as well?
> Does that make sense in the context of bare metal triples?  My understanding 
> of the layout of the resource directory for these targets is hazy, but I was 
> under the impression that each target architecture having its own namespaced 
> copy of compiler-rt in `/lib` *is* how we do per-target 
> runtimes.
The per-target runtimes directory layout always expects a triple, i.e. 
`/lib/`. In case of bare metal, the triple would be e.g. 
`armv6m-none-eabi`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59425



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