[PATCH] D72028: [CodeGen] Emit conj/conjf/confjl libcalls as fneg instructions if possible.

[Craig Topper via Phabricator via cfe-commits]

craig.topper created this revision.
craig.topper added reviewers: spatel, efriedma, rjmccall.
Herald added a project: clang.

We already recognize the __builtin versions of these, might as well
recognize the libcall version.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72028

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/complex-libcalls-2.c
  clang/test/CodeGen/complex-libcalls.c


Index: clang/test/CodeGen/complex-libcalls.c
===
--- clang/test/CodeGen/complex-libcalls.c
+++ clang/test/CodeGen/complex-libcalls.c
@@ -112,12 +112,10 @@
 
   conj(f);   conjf(f);  conjl(f);
 
-// NO__ERRNO: declare { double, double } @conj(double, double) 
[[READNONE:#[0-9]+]]
-// NO__ERRNO: declare <2 x float> @conjf(<2 x float>) [[READNONE]]
-// NO__ERRNO: declare { x86_fp80, x86_fp80 } @conjl({ x86_fp80, x86_fp80 }* 
byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
-// HAS_ERRNO: declare { double, double } @conj(double, double) 
[[READNONE:#[0-9]+]]
-// HAS_ERRNO: declare <2 x float> @conjf(<2 x float>) [[READNONE]]
-// HAS_ERRNO: declare { x86_fp80, x86_fp80 } @conjl({ x86_fp80, x86_fp80 }* 
byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
+// NO__ERRNO-NOT: .conj
+// NO__ERRNO-NOT: @conj
+// HAS_ERRNO-NOT: .conj
+// HAS_ERRNO-NOT: @conj
 
   clog(f);   clogf(f);  clogl(f);
 
@@ -133,7 +131,7 @@
 // NO__ERRNO: declare { double, double } @cproj(double, double) [[READNONE]]
 // NO__ERRNO: declare <2 x float> @cprojf(<2 x float>) [[READNONE]]
 // NO__ERRNO: declare { x86_fp80, x86_fp80 } @cprojl({ x86_fp80, x86_fp80 }* 
byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
-// HAS_ERRNO: declare { double, double } @cproj(double, double) [[READNONE]]
+// HAS_ERRNO: declare { double, double } @cproj(double, double) 
[[READNONE:#[0-9]+]]
 // HAS_ERRNO: declare <2 x float> @cprojf(<2 x float>) [[READNONE]]
 // HAS_ERRNO: declare { x86_fp80, x86_fp80 } @cprojl({ x86_fp80, x86_fp80 }* 
byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
 
Index: clang/test/CodeGen/complex-libcalls-2.c
===
--- /dev/null
+++ clang/test/CodeGen/complex-libcalls-2.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm
  %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm 
-fmath-errno %s | FileCheck %s
+
+float _Complex test_conjf(float _Complex x) {
+// CHECK-LABEL: @test_conjf(
+// CHECK: fneg float %x.imag
+  return conjf(x);
+}
+
+double _Complex test_conj(double _Complex x) {
+// CHECK-LABEL: @test_conj(
+// CHECK: fneg double %x.imag
+  return conj(x);
+}
+
+long double _Complex test_conjl(long double _Complex x) {
+// CHECK-LABEL: @test_conjl(
+// CHECK: fneg x86_fp80 %x.imag
+  return conjl(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1938,7 +1938,10 @@
   }
   case Builtin::BI__builtin_conj:
   case Builtin::BI__builtin_conjf:
-  case Builtin::BI__builtin_conjl: {
+  case Builtin::BI__builtin_conjl:
+  case Builtin::BIconj:
+  case Builtin::BIconjf:
+  case Builtin::BIconjl: {
 ComplexPairTy ComplexVal = EmitComplexExpr(E->getArg(0));
 Value *Real = ComplexVal.first;
 Value *Imag = ComplexVal.second;


Index: clang/test/CodeGen/complex-libcalls.c
===
--- clang/test/CodeGen/complex-libcalls.c
+++ clang/test/CodeGen/complex-libcalls.c
@@ -112,12 +112,10 @@
 
   conj(f);   conjf(f);  conjl(f);
 
-// NO__ERRNO: declare { double, double } @conj(double, double) [[READNONE:#[0-9]+]]
-// NO__ERRNO: declare <2 x float> @conjf(<2 x float>) [[READNONE]]
-// NO__ERRNO: declare { x86_fp80, x86_fp80 } @conjl({ x86_fp80, x86_fp80 }* byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
-// HAS_ERRNO: declare { double, double } @conj(double, double) [[READNONE:#[0-9]+]]
-// HAS_ERRNO: declare <2 x float> @conjf(<2 x float>) [[READNONE]]
-// HAS_ERRNO: declare { x86_fp80, x86_fp80 } @conjl({ x86_fp80, x86_fp80 }* byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
+// NO__ERRNO-NOT: .conj
+// NO__ERRNO-NOT: @conj
+// HAS_ERRNO-NOT: .conj
+// HAS_ERRNO-NOT: @conj
 
   clog(f);   clogf(f);  clogl(f);
 
@@ -133,7 +131,7 @@
 // NO__ERRNO: declare { double, double } @cproj(double, double) [[READNONE]]
 // NO__ERRNO: declare <2 x float> @cprojf(<2 x float>) [[READNONE]]
 // NO__ERRNO: declare { x86_fp80, x86_fp80 } @cprojl({ x86_fp80, x86_fp80 }* byval({ x86_fp80, x86_fp80 }) align 16) [[NOT_READNONE]]
-// HAS_ERRNO: declare { double, double } @cproj(double, double) [[READNONE]]
+// HAS_ERRNO: declare { double, double } @cproj(double, double) [[READNONE:#[0-9]+]]
 // HAS_ERRNO: declare <2 x float> @cprojf(<2 x float>) [[READNONE]]
 // 

[PATCH] D72014: [PowerPC]: Add powerpcspe target triple subarch component

[Jim Lin via Phabricator via cfe-commits]

Jim added a comment.

Could you add tests in llvm/unittests/ADT/TripleTest.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72014



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:62
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),

njames93 wrote:
> xazax.hun wrote:
> > njames93 wrote:
> > > xazax.hun wrote:
> > > > Why do we care if we are inside a template instantiation? 
> > > > 
> > > > Couldn't we trigger the bug with something like:
> > > > ```
> > > > void shouldPass() {
> > > >   if constexpr (constexprFun(1) == 0) {
> > > > handle(0);
> > > >   } else if constexpr (constexprFun(1)  == 1) {
> > > > handle(1);
> > > >   } else {
> > > > handle(2);
> > > >   }
> > > > }
> > > > ```
> > > > 
> > > > ?
> > > We are disabling the bug check when we are in a template instantiation. 
> > > Reason being the template instantiation folds away the false branches to 
> > > null statements which is the cause of the false positives. Also if there 
> > > is a bug in a templated if constexpr, it will be caught when the template 
> > > is defined. 
> > Yeah, be people are free to write `if constexpr` outside of templates 
> > right? So instantiations are not the only places where branches can be 
> > folded away.
> The issue is that only template instantiations cause the false branch to get 
> folded away, observe [[ https://godbolt.org/z/u48xzy | here ]] to see how its 
> handled in templated code that isn't yet instantiated, instantiated template 
> code and normal untemplated code
Oh, I see. This is really weird. I do not really see why should the `if 
constexpr` behave differently outside of instantiations. Thanks for the 
clarification!


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

https://reviews.llvm.org/D71980



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

Please add new lines to the end of the test files. Also please add a test 
similar to the godbolt link you gave me to show that `if consexpr (false)` 
outside of templates work as expected. Otherwise LGTM!


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

https://reviews.llvm.org/D71980



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:62
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),

xazax.hun wrote:
> njames93 wrote:
> > xazax.hun wrote:
> > > Why do we care if we are inside a template instantiation? 
> > > 
> > > Couldn't we trigger the bug with something like:
> > > ```
> > > void shouldPass() {
> > >   if constexpr (constexprFun(1) == 0) {
> > > handle(0);
> > >   } else if constexpr (constexprFun(1)  == 1) {
> > > handle(1);
> > >   } else {
> > > handle(2);
> > >   }
> > > }
> > > ```
> > > 
> > > ?
> > We are disabling the bug check when we are in a template instantiation. 
> > Reason being the template instantiation folds away the false branches to 
> > null statements which is the cause of the false positives. Also if there is 
> > a bug in a templated if constexpr, it will be caught when the template is 
> > defined. 
> Yeah, be people are free to write `if constexpr` outside of templates right? 
> So instantiations are not the only places where branches can be folded away.
The issue is that only template instantiations cause the false branch to get 
folded away, observe [[ https://godbolt.org/z/u48xzy | here ]] to see how its 
handled in templated code that isn't yet instantiated, instantiated template 
code and normal untemplated code


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

https://reviews.llvm.org/D71980



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:62
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),

njames93 wrote:
> xazax.hun wrote:
> > Why do we care if we are inside a template instantiation? 
> > 
> > Couldn't we trigger the bug with something like:
> > ```
> > void shouldPass() {
> >   if constexpr (constexprFun(1) == 0) {
> > handle(0);
> >   } else if constexpr (constexprFun(1)  == 1) {
> > handle(1);
> >   } else {
> > handle(2);
> >   }
> > }
> > ```
> > 
> > ?
> We are disabling the bug check when we are in a template instantiation. 
> Reason being the template instantiation folds away the false branches to null 
> statements which is the cause of the false positives. Also if there is a bug 
> in a templated if constexpr, it will be caught when the template is defined. 
Yeah, be people are free to write `if constexpr` outside of templates right? So 
instantiations are not the only places where branches can be folded away.


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

https://reviews.llvm.org/D71980



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

[PATCH] D72014: [PowerPC]: Add powerpcspe target triple subarch component

[Justin Hibbits via Phabricator via cfe-commits]

jhibbits updated this revision to Diff 235668.
jhibbits added a comment.

Reuse the SPE feature test preprocessor test check instead of duplicating it.
powerpcspe-* is exactly equivalent to the -mspe command line argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72014

Files:
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/test/Preprocessor/init.c
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/lib/Target/PowerPC/PPCSubtarget.cpp


Index: llvm/lib/Target/PowerPC/PPCSubtarget.cpp
===
--- llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -150,6 +150,9 @@
   TargetTriple.isMusl())
 SecurePlt = true;
 
+  if (TargetTriple.getSubArch() == Triple::PPCSubArch_spe)
+HasSPE = true;
+
   if (HasSPE && IsPPC64)
 report_fatal_error( "SPE is only supported for 32-bit targets.\n", false);
   if (HasSPE && (HasAltivec || HasQPX || HasVSX || HasFPU))
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -389,7 +389,7 @@
 // FIXME: Do we need to support these?
 .Cases("i786", "i886", "i986", Triple::x86)
 .Cases("amd64", "x86_64", "x86_64h", Triple::x86_64)
-.Cases("powerpc", "ppc", "ppc32", Triple::ppc)
+.Cases("powerpc", "powerpcspe", "ppc", "ppc32", Triple::ppc)
 .Cases("powerpc64", "ppu", "ppc64", Triple::ppc64)
 .Cases("powerpc64le", "ppc64le", Triple::ppc64le)
 .Case("xscale", Triple::arm)
@@ -563,6 +563,9 @@
   (SubArchName.endswith("r6el") || SubArchName.endswith("r6")))
 return Triple::MipsSubArch_r6;
 
+  if (SubArchName == "powerpcspe")
+return Triple::PPCSubArch_spe;
+
   StringRef ARMSubArch = ARM::getCanonicalArchName(SubArchName);
 
   // For now, this is the small part. Early return.
Index: llvm/include/llvm/ADT/Triple.h
===
--- llvm/include/llvm/ADT/Triple.h
+++ llvm/include/llvm/ADT/Triple.h
@@ -128,7 +128,9 @@
 KalimbaSubArch_v4,
 KalimbaSubArch_v5,
 
-MipsSubArch_r6
+MipsSubArch_r6,
+
+PPCSubArch_spe
   };
   enum VendorType {
 UnknownVendor,
Index: clang/test/Preprocessor/init.c
===
--- clang/test/Preprocessor/init.c
+++ clang/test/Preprocessor/init.c
@@ -7617,6 +7617,7 @@
 // PPC32-LINUX-NOT: _CALL_LINUX
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu 
-target-feature +spe < /dev/null | FileCheck -match-full-lines -check-prefix 
PPC32-SPE %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpcspe-unknown-linux-gnu 
< /dev/null | FileCheck -match-full-lines -check-prefix PPC32-SPE %s
 //
 // PPC32-SPE:#define __NO_FPRS__ 1
 // PPC32-SPE:#define __SPE__ 1
Index: clang/lib/Basic/Targets/PPC.h
===
--- clang/lib/Basic/Targets/PPC.h
+++ clang/lib/Basic/Targets/PPC.h
@@ -87,8 +87,7 @@
 
   // Note: GCC recognizes the following additional cpus:
   //  401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
-  //  821, 823, 8540, 8548, e300c2, e300c3, e500mc64, e6500, 860, cell,
-  //  titan, rs64.
+  //  821, 823, 8540, e300c2, e300c3, e500mc64, e6500, 860, cell, titan, rs64.
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl ) const override;
 
Index: clang/lib/Basic/Targets/PPC.cpp
===
--- clang/lib/Basic/Targets/PPC.cpp
+++ clang/lib/Basic/Targets/PPC.cpp
@@ -316,7 +316,8 @@
 .Case("pwr8", true)
 .Default(false);
 
-  Features["spe"] = llvm::StringSwitch(CPU)
+  Features["spe"] = getTriple().getSubArch() == llvm::Triple::PPCSubArch_spe ||
+llvm::StringSwitch(CPU)
 .Case("8548", true)
 .Case("e500", true)
 .Default(false);


Index: llvm/lib/Target/PowerPC/PPCSubtarget.cpp
===
--- llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -150,6 +150,9 @@
   TargetTriple.isMusl())
 SecurePlt = true;
 
+  if (TargetTriple.getSubArch() == Triple::PPCSubArch_spe)
+HasSPE = true;
+
   if (HasSPE && IsPPC64)
 report_fatal_error( "SPE is only supported for 32-bit targets.\n", false);
   if (HasSPE && (HasAltivec || HasQPX || HasVSX || HasFPU))
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -389,7 +389,7 @@
 // FIXME: Do we need to support these?
 .Cases("i786", 

[PATCH] D72014: [PowerPC]: Add powerpcspe target triple subarch component

[Justin Hibbits via Phabricator via cfe-commits]

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



Comment at: clang/test/Preprocessor/init.c:7628
+// PPC32-SPE-TGT:#define __SPE__ 1
 // 
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu 
-target-cpu 8548 < /dev/null | FileCheck -match-full-lines -check-prefix 
PPC8548 %s

arichardson wrote:
> These seem to be checking the same defines. It's it possible to reuse the 
> existing `-check-prefix PPC32-SPE` for new newly added command?
Yeah, it's better to reuse.  I waffled on it a bit before doing it this way, 
and didn't really like it anyway.  Reverting to the reuse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72014



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

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Jim Lin via Phabricator via cfe-commits]

Jim added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:66
 
-Once you are done, change to the ``llvm/tools/clang/tools/extra`` directory, 
and
+Once you are done, change to the ``llvm/clang-tools-extra`` directory, and
 let's start!

I am no sure that "llvm/clang-tools-extra" should be replaced as 
"llvm-project/clang-tools-extra".
Maybe someone would confuse with "llvm", "llvm-project" and "llvm-project/llvm"



Comment at: clang/www/hacking.html:301
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git 
to contribute to Clang.
 

This change should be in a separate patch.


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

https://reviews.llvm.org/D71982



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Nathan James via Phabricator via cfe-commits]

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

In D71980#1799437 , @xazax.hun wrote:

> Thanks for working on this, please see one of my concerns inline.
>
> If you are trying to fix this problem, alternatively, you could also fix it 
> in Clang.
>
> Richard outlined the best possible way to solve this class of errors by 
> introducing a new type of AST node in: https://reviews.llvm.org/D46234


I'm going to look into adding a DiscardedStmt, see how that goes. would 
probably be best just creating a new Stmt rather than trying to inherit from 
NullStmt.




Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:62
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),

xazax.hun wrote:
> Why do we care if we are inside a template instantiation? 
> 
> Couldn't we trigger the bug with something like:
> ```
> void shouldPass() {
>   if constexpr (constexprFun(1) == 0) {
> handle(0);
>   } else if constexpr (constexprFun(1)  == 1) {
> handle(1);
>   } else {
> handle(2);
>   }
> }
> ```
> 
> ?
We are disabling the bug check when we are in a template instantiation. Reason 
being the template instantiation folds away the false branches to null 
statements which is the cause of the false positives. Also if there is a bug in 
a templated if constexpr, it will be caught when the template is defined. 


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

https://reviews.llvm.org/D71980



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235661.
njames93 added a comment.

Fixed a few of the test cases failing


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+  int i = 0;
+expr(nullPointerConstant())
+  matches 

[PATCH] D71991: Fix external-names.c test when separator is \\

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

I'm still not seeing how the backslashes got in there for you.  They don't 
happen for me.  It looks like these paths come (almost) directly from the temp 
.yaml files created by the `sed` commands at the beginning of the test, so--for 
these particular tests--I'd expect forward slashes regardless of platform.

I made a set of interelated VFS changes for Windows compatibility.  The last of 
those was on December 18.  Could your repo be older than that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71991



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks for working on this, please see one of my concerns inline.

If you are trying to fix this problem, alternatively, you could also fix it in 
Clang.

Richard outlined the best possible way to solve this class of errors by 
introducing a new type of AST node in: https://reviews.llvm.org/D46234




Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:62
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),

Why do we care if we are inside a template instantiation? 

Couldn't we trigger the bug with something like:
```
void shouldPass() {
  if constexpr (constexprFun(1) == 0) {
handle(0);
  } else if constexpr (constexprFun(1)  == 1) {
handle(1);
  } else {
handle(2);
  }
}
```

?


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

https://reviews.llvm.org/D71980



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

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

[Samuel Thibault via Phabricator via cfe-commits]

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

> [Gnu toolchain]
>  maybe this can be removed in the title?

I thought that commits usually have such kind of title prefixes?




Comment at: clang/lib/Driver/ToolChains/Linux.cpp:36
 ///
 /// Debian-based systems are starting to use a multiarch setup where they use
 /// a target-triple directory in the library and header search paths.

sylvestre.ledru wrote:
> are using a multiarch
> 
Well, I can change that, but it's unrelated to the other change of the patch?


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

https://reviews.llvm.org/D69758



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

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: aaron.ballman, NoQ, haowei.
xazax.hun added a project: clang.
Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

While the static analyzer is doing a great job modeling functions most of the 
time, sometimes we have global dynamic invariants that might be infeasible to 
reason about statically (or in some cases just to complex to worth to implement 
it).

It would be great to have a way to tell the checkers that certain functions are 
not worth modeling. In case of fuchsia.HandleCheck, we could achieve the same 
by simply removing all the annotations. This is not always desired though. The 
annotations are useful on their own, they document the ownership semantics of 
the handles. So instead of removing the annotations for these functions, we 
think it might be better to introduce yet another annotation for this use case.

What do you think? Is this reasonable or do you have any alternative ideas?

For the curious the syscall I have problem with in Fuchsia is the 
`zx_channel_read`: 
https://fuchsia.dev/fuchsia-src/reference/syscalls/channel_read.md

The problem is mainly with the `handles` parameter.  We might not receive 
handles at all. And we might know that in advance that we will not receive 
handles, so we do not need to check the `actual_handles`. So assuming `handles` 
contains open handles will end up producing spurious handle leak errors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72018

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -15,6 +15,7 @@
 // CHECK-NEXT: AllocSize (SubjectMatchRule_function)
 // CHECK-NEXT: AlwaysDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: AlwaysInline (SubjectMatchRule_function)
+// CHECK-NEXT: AnalyzerCheckerIgnore (SubjectMatchRule_function)
 // CHECK-NEXT: Annotate ()
 // CHECK-NEXT: AnyX86NoCfCheck (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: ArcWeakrefUnavailable (SubjectMatchRule_objc_interface)
Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -12,10 +12,12 @@
 #define ZX_HANDLE_ACQUIRE  __attribute__((acquire_handle("Fuchsia")))
 #define ZX_HANDLE_RELEASE  __attribute__((release_handle("Fuchsia")))
 #define ZX_HANDLE_USE  __attribute__((use_handle("Fuchsia")))
+#define ZX_HANDLE_IGNORE __attribute__((analyzer_checker_ignore("fuchsia.HandleChecker")))
 #else
 #define ZX_HANDLE_ACQUIRE
 #define ZX_HANDLE_RELEASE
 #define ZX_HANDLE_USE
+#define ZX_HANDLE_IGNORE
 #endif
 
 zx_status_t zx_channel_create(
@@ -23,6 +25,12 @@
 zx_handle_t *out0 ZX_HANDLE_ACQUIRE,
 zx_handle_t *out1 ZX_HANDLE_ACQUIRE);
 
+ZX_HANDLE_IGNORE
+zx_status_t zx_channel_create_complicated(
+uint32_t options,
+zx_handle_t *out0 ZX_HANDLE_ACQUIRE,
+zx_handle_t *out1 ZX_HANDLE_ACQUIRE);
+
 zx_status_t zx_handle_close(
 zx_handle_t handle ZX_HANDLE_RELEASE);
 
@@ -327,3 +335,11 @@
 return;
   zx_handle_close(h2);
 } // *h should be escaped here. Right?
+
+// Ignored functions
+
+void ignore_function() {
+  zx_handle_t sa, sb;
+  if (zx_channel_create_complicated(0, , ))
+return;
+}
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -302,6 +302,12 @@
   if (!FuncDecl)
 return;
 
+  if (const auto *Attr = FuncDecl->getAttr())
+if (llvm::any_of(Attr->checkers(), [this](StringRef Checker) {
+  return Checker == getCheckerName();
+}))
+  return;
+
   ProgramStateRef State = C.getState();
 
   std::vector> Notes;
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2065,6 +2065,25 @@
   D->addAttr(::new (S.Context) AnalyzerNoReturnAttr(S.Context, AL));
 }
 
+static void HandleAnalyzerCheckerIgnoreAttr(Sema , Decl *D,
+const ParsedAttr ) {
+  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+return;
+
+  SmallVector Checkers;
+  for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
+StringRef CheckerName;
+
+if 

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

[Sylvestre Ledru via Phabricator via cfe-commits]

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

//[Gnu toolchain] //
maybe this can be removed in the title?




Comment at: clang/lib/Driver/ToolChains/Linux.cpp:36
 ///
 /// Debian-based systems are starting to use a multiarch setup where they use
 /// a target-triple directory in the library and header search paths.

are using a multiarch



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

https://reviews.llvm.org/D69758



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

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

[Samuel Thibault via Phabricator via cfe-commits]

sthibaul added a comment.

I have updated the patch to the latest git and re-ran checks. Could somebody 
have a look at it please?


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

https://reviews.llvm.org/D69758



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235645.
njames93 retitled this revision from "[clang-tidy] Disable Checks on If 
constexpr statements in template Instantiations for BugproneBranchClone, 
ReadabilityBracesAroundStatements and ReadabilityMisleadingIndentation" to 
"[clang-tidy] Disable Checks on If constexpr statements in template 
Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements".
njames93 edited the summary of this revision.
njames93 added a comment.

I can't seem to reproduce the bug for misleading-indentation so I have removed 
that check. Test cases have been added


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

https://reviews.llvm.org/D71980

Files:
  clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-branch-clone-if-constexpr-template.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements-constexpr-if-templates.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements-constexpr-if-templates.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements-constexpr-if-templates.cpp
@@ -0,0 +1,29 @@
+// RUN: %check_clang_tidy %s readability-braces-around-statements %t -- -- -std=c++17
+
+void handle(bool);
+
+template 
+void shouldFail() {
+  if constexpr (branch)
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: statement should be inside braces [readability-braces-around-statements]
+handle(true);
+  else
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: statement should be inside braces [readability-braces-around-statements]
+handle(false);
+}
+
+template 
+void shouldPass() {
+  if constexpr (branch) {
+handle(true);
+  } else {
+handle(false);
+  }
+}
+
+void run() {
+shouldFail();
+shouldFail();
+shouldPass();
+shouldPass();
+}
\ No newline at end of file
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-branch-clone-if-constexpr-template.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-branch-clone-if-constexpr-template.cpp
@@ -0,0 +1,35 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t -- -- -std=c++17
+
+void handle(int);
+
+template 
+void shouldFail() {
+  if constexpr (T == 0) {
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: repeated branch in conditional chain [bugprone-branch-clone]
+handle(0);
+  } else if constexpr (T == 1) {
+handle(1);
+  } else {
+handle(0);
+  }
+}
+
+template 
+void shouldPass() {
+  if constexpr (T == 0) {
+handle(0);
+  } else if constexpr (T == 1) {
+handle(1);
+  } else {
+handle(2);
+  }
+}
+
+void run() {
+shouldFail<0>();
+shouldFail<1>();
+shouldFail<2>();
+shouldPass<0>();
+shouldPass<1>();
+shouldPass<2>();
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -123,7 +123,10 @@
 }
 
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt().bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation(
+  .bind("if"),
+  this);
   Finder->addMatcher(whileStmt().bind("while"), this);
   Finder->addMatcher(doStmt().bind("do"), this);
   Finder->addMatcher(forStmt().bind("for"), this);
Index: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -59,7 +59,8 @@
 
 void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),
  hasParent(stmt(unless(ifStmt(hasElse(equalsBoundNode("if")),
  hasElse(stmt().bind("else"))),
   this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

[Samuel Thibault via Phabricator via cfe-commits]

sthibaul updated this revision to Diff 235644.
sthibaul marked an inline comment as done.

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

https://reviews.llvm.org/D69758

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Gnu.h
  clang/lib/Driver/ToolChains/Hurd.cpp
  clang/lib/Driver/ToolChains/Hurd.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h

Index: clang/lib/Driver/ToolChains/Linux.h
===
--- clang/lib/Driver/ToolChains/Linux.h
+++ clang/lib/Driver/ToolChains/Linux.h
@@ -26,9 +26,6 @@
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const override;
-  void addLibCxxIncludePaths(
-  const llvm::opt::ArgList ,
-  llvm::opt::ArgStringList ) const override;
   void addLibStdCxxIncludePaths(
   const llvm::opt::ArgList ,
   llvm::opt::ArgStringList ) const override;
@@ -52,6 +49,10 @@
 protected:
   Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
+
+  std::string getMultiarchTriple(const Driver ,
+ const llvm::Triple ,
+ StringRef SysRoot) const override;
 };
 
 } // end namespace toolchains
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -37,9 +37,9 @@
 /// a target-triple directory in the library and header search paths.
 /// Unfortunately, this triple does not align with the vanilla target triple,
 /// so we provide a rough mapping here.
-static std::string getMultiarchTriple(const Driver ,
+std::string Linux::getMultiarchTriple(const Driver ,
   const llvm::Triple ,
-  StringRef SysRoot) {
+  StringRef SysRoot) const {
   llvm::Triple::EnvironmentType TargetEnvironment =
   TargetTriple.getEnvironment();
   bool IsAndroid = TargetTriple.isAndroid();
@@ -865,86 +865,23 @@
 addSystemInclude(DriverArgs, CC1Args, ResourceDirInclude);
 }
 
-static std::string DetectLibcxxIncludePath(llvm::vfs::FileSystem ,
-   StringRef base) {
-  std::error_code EC;
-  int MaxVersion = 0;
-  std::string MaxVersionString = "";
-  for (llvm::vfs::directory_iterator LI = vfs.dir_begin(base, EC), LE;
-   !EC && LI != LE; LI = LI.increment(EC)) {
-StringRef VersionText = llvm::sys::path::filename(LI->path());
-int Version;
-if (VersionText[0] == 'v' &&
-!VersionText.slice(1, StringRef::npos).getAsInteger(10, Version)) {
-  if (Version > MaxVersion) {
-MaxVersion = Version;
-MaxVersionString = VersionText;
-  }
-}
-  }
-  return MaxVersion ? (base + "/" + MaxVersionString).str() : "";
-}
-
-void Linux::addLibCxxIncludePaths(const llvm::opt::ArgList ,
-  llvm::opt::ArgStringList ) const {
-  const std::string& SysRoot = computeSysRoot();
-  auto AddIncludePath = [&](std::string Path) {
-std::string IncludePath = DetectLibcxxIncludePath(getVFS(), Path);
-if (IncludePath.empty() || !getVFS().exists(IncludePath))
-  return false;
-addSystemInclude(DriverArgs, CC1Args, IncludePath);
-return true;
-  };
-  // Android never uses the libc++ headers installed alongside the toolchain,
-  // which are generally incompatible with the NDK libraries anyway.
-  if (!getTriple().isAndroid())
-if (AddIncludePath(getDriver().Dir + "/../include/c++"))
-  return;
-  // If this is a development, non-installed, clang, libcxx will
-  // not be found at ../include/c++ but it likely to be found at
-  // one of the following two locations:
-  if (AddIncludePath(SysRoot + "/usr/local/include/c++"))
-return;
-  if (AddIncludePath(SysRoot + "/usr/include/c++"))
-return;
-}
-
 void Linux::addLibStdCxxIncludePaths(const llvm::opt::ArgList ,
  llvm::opt::ArgStringList ) const {
+  // Try generic GCC detection first.
+  if (Generic_GCC::addGCCLibStdCxxIncludePaths(DriverArgs, CC1Args))
+return;
+
   // We need a detected GCC installation on Linux to provide libstdc++'s
-  // headers.
+  // headers in odd Linuxish places.
   if (!GCCInstallation.isValid())
 return;
 
-  // By default, look for the C++ headers in an include directory adjacent to
-  // the lib directory of the GCC installation. Note that this is expect to be
-  // equivalent to '/usr/include/c++/X.Y' in almost all cases.
   StringRef LibDir = GCCInstallation.getParentLibPath();
-  StringRef InstallDir = GCCInstallation.getInstallPath();
   StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib  = GCCInstallation.getMultilib();
-  const std::string GCCMultiarchTriple = getMultiarchTriple(
-  

[PATCH] D72014: [PowerPC]: Add powerpcspe target triple subarch component

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: clang/test/Preprocessor/init.c:7628
+// PPC32-SPE-TGT:#define __SPE__ 1
 // 
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu 
-target-cpu 8548 < /dev/null | FileCheck -match-full-lines -check-prefix 
PPC8548 %s

These seem to be checking the same defines. It's it possible to reuse the 
existing `-check-prefix PPC32-SPE` for new newly added command?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72014



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

[PATCH] D71775: [ThreadPool] On Windows, extend usage to all CPU sockets and all NUMA groups

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: inglorion, gbiv.
rnk added a comment.
Herald added a subscriber: george.burgess.iv.

+@gbiv @inglorion, other ThinLTO users.

> To solve this situation, we capture (and retain) the initial intention until 
> the point of usage, through a new ThreadPoolStrategy class. The number of 
> threads to use is deferred as late as possible, until the moment where the 
> std::threads are created (ThreadPool in the case of ThinLTO).

That seems reasonable to me.




Comment at: llvm/include/llvm/ADT/SmallBitVector.h:487
+
+  int compare(const SmallBitVector ) const {
+for (unsigned I = 0; I < std::max(size(), RHS.size()); ++I) {

I guess we don't need these changes, since it looks like you use BitVector only 
below.



Comment at: llvm/include/llvm/ADT/SmallBitVector.h:500
+
+  bool operator<(const SmallBitVector ) const { return compare(RHS) == -1; 
}
+

You return `A - B` but then compare for equality to -1 and 1. I guess it works 
because you are doing it bit by bit, but it's exciting.



Comment at: llvm/include/llvm/Support/Threading.h:153
+// runtime might use a lower value (not higher).
+unsigned ThreadCount = 0;
+

Let's name the fields in a way that indicates that these numbers are the 
requested number of threads, not the final number. So, `ThreadsRequested` or 
something like that.



Comment at: llvm/include/llvm/Support/Threading.h:158
+// specific core).
+bool HyperThreads = true;
+

This could be `UseHyperThreads`. The first time I read it, I guessed that it 
indicated if the system has hyperthreads.



Comment at: llvm/lib/Support/Host.cpp:1277
  << "/proc/cpuinfo: " << EC.message() << "\n";
 return -1;
   }

Another -1 case.



Comment at: llvm/lib/Support/Host.cpp:1334
 // On other systems, return -1 to indicate unknown.
 static int computeHostNumPhysicalCores() { return -1; }
 #endif

Note: the -1 case.



Comment at: llvm/lib/Support/Threading.cpp:85-86
 
-#include 
-unsigned llvm::heavyweight_hardware_concurrency() {
-  // Since we can't get here unless LLVM_ENABLE_THREADS == 1, it is safe to use
-  // `std::thread` directly instead of `llvm::thread` (and indeed, doing so
-  // allows us to not define `thread` in the llvm namespace, which conflicts
-  // with some platforms such as FreeBSD whose headers also define a struct
-  // called `thread` in the global namespace which can cause ambiguity due to
-  // ADL.
-  int NumPhysical = sys::getHostNumPhysicalCores();
-  if (NumPhysical == -1)
-return std::thread::hardware_concurrency();
-  return NumPhysical;
-}
+int computeHostNumPhysicalCores();
+int computeHostNumPhysicalThreads();
 

We already have a public API, getHostNumPhysicalCores. Can this use that?



Comment at: llvm/lib/Support/Threading.cpp:89
+unsigned llvm::ThreadPoolStrategy::compute_thread_count() const {
+  unsigned MaxThreadCount = HyperThreads ? computeHostNumPhysicalThreads()
+ : computeHostNumPhysicalCores();

I see these `computeHostNum*` methods return int, and some versions seem to 
return -1 to indicate errors. I think you'll want to use `int` here and check 
if `MaxThreadCount <= 0`. Otherwise below we may do a signed/unsigned 
comparison mismatch, and return ~0U for MaxThreadCount.



Comment at: llvm/lib/Support/Unix/Threading.inc:273
+
+int computeHostNumPhysicalThreads() {
+#if defined(HAVE_SCHED_GETAFFINITY) && defined(HAVE_CPU_COUNT)

I'm not sure it makes sense to say "physical threads". I think "physical cores" 
was meant to distinguish between hardware threads and cores. Describing 
hardware execution resources as physical or non-physical isn't very precise or 
meaningful in the first place, but I don't think it should apply to hyper 
threads.



Comment at: llvm/lib/Support/Windows/Threading.inc:160
+
+static ArrayRef computeProcessorGroups() {
+  auto computeGroups = []() {

This is cached, so maybe `getProcessorGroups` to indicate that it is not 
intended to be expensive.



Comment at: llvm/lib/Support/Windows/Threading.inc:212
+template 
+unsigned aggregate(R &, UnaryPredicate P) {
+  unsigned I{};

Seems like `static` can be used here.



Comment at: llvm/unittests/Support/ThreadPool.cpp:31
 protected:
   // This is intended for platform as a temporary "XFAIL"
   bool isUnsupportedOSOrEnvironment() {

"Temporary" >_>



Comment at: llvm/unittests/Support/ThreadPool.cpp:180
+
+  //std::set ThreadsUsed;
+  llvm::DenseSet ThreadsUsed;

I guess this is why you added comparison operators. In any case, let's remove 
the commented out code in the 

[PATCH] D72012: [CodeGen] Use IRBuilder::CreateFNeg for __builtin_conj

[Craig Topper via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG70f8dd4cf604: [CodeGen] Use IRBuilder::CreateFNeg for 
__builtin_conj (authored by craig.topper).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72012

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/complex-builtins-2.c


Index: clang/test/CodeGen/complex-builtins-2.c
===
--- /dev/null
+++ clang/test/CodeGen/complex-builtins-2.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm
  %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm 
-fmath-errno %s | FileCheck %s
+
+float _Complex test__builtin_conjf(float _Complex x) {
+// CHECK-LABEL: @test__builtin_conjf(
+// CHECK: fneg float %x.imag
+  return __builtin_conjf(x);
+}
+
+double _Complex test__builtin_conj(double _Complex x) {
+// CHECK-LABEL: @test__builtin_conj(
+// CHECK: fneg double %x.imag
+  return __builtin_conj(x);
+}
+
+long double _Complex test__builtin_conjl(long double _Complex x) {
+// CHECK-LABEL: @test__builtin_conjl(
+// CHECK: fneg x86_fp80 %x.imag
+  return __builtin_conjl(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1942,12 +1942,7 @@
 ComplexPairTy ComplexVal = EmitComplexExpr(E->getArg(0));
 Value *Real = ComplexVal.first;
 Value *Imag = ComplexVal.second;
-Value *Zero =
-  Imag->getType()->isFPOrFPVectorTy()
-? llvm::ConstantFP::getZeroValueForNegation(Imag->getType())
-: llvm::Constant::getNullValue(Imag->getType());
-
-Imag = Builder.CreateFSub(Zero, Imag, "sub");
+Imag = Builder.CreateFNeg(Imag, "neg");
 return RValue::getComplex(std::make_pair(Real, Imag));
   }
   case Builtin::BI__builtin_creal:


Index: clang/test/CodeGen/complex-builtins-2.c
===
--- /dev/null
+++ clang/test/CodeGen/complex-builtins-2.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm  %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm -fmath-errno %s | FileCheck %s
+
+float _Complex test__builtin_conjf(float _Complex x) {
+// CHECK-LABEL: @test__builtin_conjf(
+// CHECK: fneg float %x.imag
+  return __builtin_conjf(x);
+}
+
+double _Complex test__builtin_conj(double _Complex x) {
+// CHECK-LABEL: @test__builtin_conj(
+// CHECK: fneg double %x.imag
+  return __builtin_conj(x);
+}
+
+long double _Complex test__builtin_conjl(long double _Complex x) {
+// CHECK-LABEL: @test__builtin_conjl(
+// CHECK: fneg x86_fp80 %x.imag
+  return __builtin_conjl(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1942,12 +1942,7 @@
 ComplexPairTy ComplexVal = EmitComplexExpr(E->getArg(0));
 Value *Real = ComplexVal.first;
 Value *Imag = ComplexVal.second;
-Value *Zero =
-  Imag->getType()->isFPOrFPVectorTy()
-? llvm::ConstantFP::getZeroValueForNegation(Imag->getType())
-: llvm::Constant::getNullValue(Imag->getType());
-
-Imag = Builder.CreateFSub(Zero, Imag, "sub");
+Imag = Builder.CreateFNeg(Imag, "neg");
 return RValue::getComplex(std::make_pair(Real, Imag));
   }
   case Builtin::BI__builtin_creal:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72010: [CodeGen] Use CreateFNeg in buildFMulAdd

[Craig Topper via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b23b2bbd962: [CodeGen] Use CreateFNeg in buildFMulAdd 
(authored by craig.topper).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72010

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/fp-contract-pragma.cpp


Index: clang/test/CodeGen/fp-contract-pragma.cpp
===
--- clang/test/CodeGen/fp-contract-pragma.cpp
+++ clang/test/CodeGen/fp-contract-pragma.cpp
@@ -74,3 +74,18 @@
   return (a = 2 * b) - c;
 }
 
+float fp_contract_8(float a, float b, float c) {
+// CHECK: _Z13fp_contract_8fff
+// CHECK: fneg float %c
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return a * b - c;
+}
+
+float fp_contract_9(float a, float b, float c) {
+// CHECK: _Z13fp_contract_9fff
+// CHECK: fneg float %a
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return c - a * b;
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3345,17 +3345,10 @@
 
   Value *MulOp0 = MulOp->getOperand(0);
   Value *MulOp1 = MulOp->getOperand(1);
-  if (negMul) {
-MulOp0 =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(MulOp0->getType()), MulOp0,
-"neg");
-  } else if (negAdd) {
-Addend =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(Addend->getType()), Addend,
-"neg");
-  }
+  if (negMul)
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");
 
   Value *FMulAdd = Builder.CreateCall(
   CGF.CGM.getIntrinsic(llvm::Intrinsic::fmuladd, Addend->getType()),


Index: clang/test/CodeGen/fp-contract-pragma.cpp
===
--- clang/test/CodeGen/fp-contract-pragma.cpp
+++ clang/test/CodeGen/fp-contract-pragma.cpp
@@ -74,3 +74,18 @@
   return (a = 2 * b) - c;
 }
 
+float fp_contract_8(float a, float b, float c) {
+// CHECK: _Z13fp_contract_8fff
+// CHECK: fneg float %c
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return a * b - c;
+}
+
+float fp_contract_9(float a, float b, float c) {
+// CHECK: _Z13fp_contract_9fff
+// CHECK: fneg float %a
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return c - a * b;
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3345,17 +3345,10 @@
 
   Value *MulOp0 = MulOp->getOperand(0);
   Value *MulOp1 = MulOp->getOperand(1);
-  if (negMul) {
-MulOp0 =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(MulOp0->getType()), MulOp0,
-"neg");
-  } else if (negAdd) {
-Addend =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(Addend->getType()), Addend,
-"neg");
-  }
+  if (negMul)
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");
 
   Value *FMulAdd = Builder.CreateCall(
   CGF.CGM.getIntrinsic(llvm::Intrinsic::fmuladd, Addend->getType()),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 70f8dd4 - [CodeGen] Use IRBuilder::CreateFNeg for __builtin_conj

[Craig Topper via cfe-commits]

Author: Craig Topper
Date: 2019-12-30T13:25:23-08:00
New Revision: 70f8dd4cf604b2be3488895ef0d261154c1c1124

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

LOG: [CodeGen] Use IRBuilder::CreateFNeg for __builtin_conj

This replaces the fsub -0.0 idiom with an fneg instruction. We didn't see to 
have a test that showed the current codegen. Just some tests for constant 
folding and a test that was only checking the declare lines for libcalls. The 
latter just checked that we did not have a declare for @conj when using 
__builtin_conj.

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

Added: 
clang/test/CodeGen/complex-builtins-2.c

Modified: 
clang/lib/CodeGen/CGBuiltin.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 17b8cab71294..fe0607bbd027 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -1942,12 +1942,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl 
GD, unsigned BuiltinID,
 ComplexPairTy ComplexVal = EmitComplexExpr(E->getArg(0));
 Value *Real = ComplexVal.first;
 Value *Imag = ComplexVal.second;
-Value *Zero =
-  Imag->getType()->isFPOrFPVectorTy()
-? llvm::ConstantFP::getZeroValueForNegation(Imag->getType())
-: llvm::Constant::getNullValue(Imag->getType());
-
-Imag = Builder.CreateFSub(Zero, Imag, "sub");
+Imag = Builder.CreateFNeg(Imag, "neg");
 return RValue::getComplex(std::make_pair(Real, Imag));
   }
   case Builtin::BI__builtin_creal:

diff  --git a/clang/test/CodeGen/complex-builtins-2.c 
b/clang/test/CodeGen/complex-builtins-2.c
new file mode 100644
index ..d112637a6cb5
--- /dev/null
+++ b/clang/test/CodeGen/complex-builtins-2.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm
  %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm 
-fmath-errno %s | FileCheck %s
+
+float _Complex test__builtin_conjf(float _Complex x) {
+// CHECK-LABEL: @test__builtin_conjf(
+// CHECK: fneg float %x.imag
+  return __builtin_conjf(x);
+}
+
+double _Complex test__builtin_conj(double _Complex x) {
+// CHECK-LABEL: @test__builtin_conj(
+// CHECK: fneg double %x.imag
+  return __builtin_conj(x);
+}
+
+long double _Complex test__builtin_conjl(long double _Complex x) {
+// CHECK-LABEL: @test__builtin_conjl(
+// CHECK: fneg x86_fp80 %x.imag
+  return __builtin_conjl(x);
+}



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

[clang] 8b23b2b - [CodeGen] Use CreateFNeg in buildFMulAdd

[Craig Topper via cfe-commits]

Author: Craig Topper
Date: 2019-12-30T13:24:11-08:00
New Revision: 8b23b2bbd9622c5f079a71c7078d167052f6a70c

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

LOG: [CodeGen] Use CreateFNeg in buildFMulAdd

We have an fneg instruction now and should use it instead of the fsub -0.0 
idiom. Looks like we had no test that showed that we handled the negation cases 
here so I've added new tests.

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

Added: 


Modified: 
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGen/fp-contract-pragma.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index b8846162508e..12bf37e5343c 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -3345,17 +3345,10 @@ static Value* buildFMulAdd(llvm::BinaryOperator *MulOp, 
Value *Addend,
 
   Value *MulOp0 = MulOp->getOperand(0);
   Value *MulOp1 = MulOp->getOperand(1);
-  if (negMul) {
-MulOp0 =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(MulOp0->getType()), MulOp0,
-"neg");
-  } else if (negAdd) {
-Addend =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(Addend->getType()), Addend,
-"neg");
-  }
+  if (negMul)
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");
 
   Value *FMulAdd = Builder.CreateCall(
   CGF.CGM.getIntrinsic(llvm::Intrinsic::fmuladd, Addend->getType()),

diff  --git a/clang/test/CodeGen/fp-contract-pragma.cpp 
b/clang/test/CodeGen/fp-contract-pragma.cpp
index 1c5921a53f97..805cc5d9c299 100644
--- a/clang/test/CodeGen/fp-contract-pragma.cpp
+++ b/clang/test/CodeGen/fp-contract-pragma.cpp
@@ -74,3 +74,18 @@ float fp_contract_7(float a, float b, float c) {
   return (a = 2 * b) - c;
 }
 
+float fp_contract_8(float a, float b, float c) {
+// CHECK: _Z13fp_contract_8fff
+// CHECK: fneg float %c
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return a * b - c;
+}
+
+float fp_contract_9(float a, float b, float c) {
+// CHECK: _Z13fp_contract_9fff
+// CHECK: fneg float %a
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return c - a * b;
+}



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

[PATCH] D72014: [PowerPC]: Add powerpcspe target triple subarch component

[Justin Hibbits via Phabricator via cfe-commits]

jhibbits updated this revision to Diff 235637.
jhibbits added a comment.

Fix a test typo made at the last minute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72014

Files:
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/test/Preprocessor/init.c
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/lib/Target/PowerPC/PPCSubtarget.cpp

Index: llvm/lib/Target/PowerPC/PPCSubtarget.cpp
===
--- llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -150,6 +150,9 @@
   TargetTriple.isMusl())
 SecurePlt = true;
 
+  if (TargetTriple.getSubArch() == Triple::PPCSubArch_spe)
+HasSPE = true;
+
   if (HasSPE && IsPPC64)
 report_fatal_error( "SPE is only supported for 32-bit targets.\n", false);
   if (HasSPE && (HasAltivec || HasQPX || HasVSX || HasFPU))
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -389,7 +389,7 @@
 // FIXME: Do we need to support these?
 .Cases("i786", "i886", "i986", Triple::x86)
 .Cases("amd64", "x86_64", "x86_64h", Triple::x86_64)
-.Cases("powerpc", "ppc", "ppc32", Triple::ppc)
+.Cases("powerpc", "powerpcspe", "ppc", "ppc32", Triple::ppc)
 .Cases("powerpc64", "ppu", "ppc64", Triple::ppc64)
 .Cases("powerpc64le", "ppc64le", Triple::ppc64le)
 .Case("xscale", Triple::arm)
@@ -563,6 +563,9 @@
   (SubArchName.endswith("r6el") || SubArchName.endswith("r6")))
 return Triple::MipsSubArch_r6;
 
+  if (SubArchName == "powerpcspe")
+return Triple::PPCSubArch_spe;
+
   StringRef ARMSubArch = ARM::getCanonicalArchName(SubArchName);
 
   // For now, this is the small part. Early return.
Index: llvm/include/llvm/ADT/Triple.h
===
--- llvm/include/llvm/ADT/Triple.h
+++ llvm/include/llvm/ADT/Triple.h
@@ -128,7 +128,9 @@
 KalimbaSubArch_v4,
 KalimbaSubArch_v5,
 
-MipsSubArch_r6
+MipsSubArch_r6,
+
+PPCSubArch_spe
   };
   enum VendorType {
 UnknownVendor,
Index: clang/test/Preprocessor/init.c
===
--- clang/test/Preprocessor/init.c
+++ clang/test/Preprocessor/init.c
@@ -7616,10 +7616,15 @@
 //
 // PPC32-LINUX-NOT: _CALL_LINUX
 //
-// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu -target-feature +spe < /dev/null | FileCheck -match-full-lines -check-prefix PPC32-SPE %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu -target-feature +spe < /dev/null | FileCheck -match-full-lines -check-prefix PPC32-SPE-FEAT %s
 //
-// PPC32-SPE:#define __NO_FPRS__ 1
-// PPC32-SPE:#define __SPE__ 1
+// PPC32-SPE-FEAT:#define __NO_FPRS__ 1
+// PPC32-SPE-FEAT:#define __SPE__ 1
+// 
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpcspe-unknown-linux-gnu < /dev/null | FileCheck -match-full-lines -check-prefix PPC32-SPE-TGT %s
+//
+// PPC32-SPE-TGT:#define __NO_FPRS__ 1
+// PPC32-SPE-TGT:#define __SPE__ 1
 // 
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu -target-cpu 8548 < /dev/null | FileCheck -match-full-lines -check-prefix PPC8548 %s
 //
Index: clang/lib/Basic/Targets/PPC.h
===
--- clang/lib/Basic/Targets/PPC.h
+++ clang/lib/Basic/Targets/PPC.h
@@ -87,8 +87,7 @@
 
   // Note: GCC recognizes the following additional cpus:
   //  401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
-  //  821, 823, 8540, 8548, e300c2, e300c3, e500mc64, e6500, 860, cell,
-  //  titan, rs64.
+  //  821, 823, 8540, e300c2, e300c3, e500mc64, e6500, 860, cell, titan, rs64.
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl ) const override;
 
Index: clang/lib/Basic/Targets/PPC.cpp
===
--- clang/lib/Basic/Targets/PPC.cpp
+++ clang/lib/Basic/Targets/PPC.cpp
@@ -316,7 +316,8 @@
 .Case("pwr8", true)
 .Default(false);
 
-  Features["spe"] = llvm::StringSwitch(CPU)
+  Features["spe"] = getTriple().getSubArch() == llvm::Triple::PPCSubArch_spe ||
+llvm::StringSwitch(CPU)
 .Case("8548", true)
 .Case("e500", true)
 .Default(false);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72014: [PowerPC]: Add powerpcspe target triple subarch component

[Justin Hibbits via Phabricator via cfe-commits]

jhibbits created this revision.
jhibbits added a reviewer: PowerPC.
Herald added subscribers: llvm-commits, cfe-commits, steven.zhang, shchenz, 
jsji, dexonsmith, kbarton, hiraditya, krytarowski, arichardson, nemanjai, 
emaste.
Herald added projects: clang, LLVM.

This allows the use of '-target powerpcspe-unknown-linux-gnu' or
'powerpcspe-unknown-freebsd' to be used, instead of
'-target powerpc-unknown-linux-gnu -mspe'.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72014

Files:
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/test/Preprocessor/init.c
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Support/Triple.cpp
  llvm/lib/Target/PowerPC/PPCSubtarget.cpp

Index: llvm/lib/Target/PowerPC/PPCSubtarget.cpp
===
--- llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -150,6 +150,9 @@
   TargetTriple.isMusl())
 SecurePlt = true;
 
+  if (TargetTriple.getSubArch() == Triple::PPCSubArch_spe)
+HasSPE = true;
+
   if (HasSPE && IsPPC64)
 report_fatal_error( "SPE is only supported for 32-bit targets.\n", false);
   if (HasSPE && (HasAltivec || HasQPX || HasVSX || HasFPU))
Index: llvm/lib/Support/Triple.cpp
===
--- llvm/lib/Support/Triple.cpp
+++ llvm/lib/Support/Triple.cpp
@@ -389,7 +389,7 @@
 // FIXME: Do we need to support these?
 .Cases("i786", "i886", "i986", Triple::x86)
 .Cases("amd64", "x86_64", "x86_64h", Triple::x86_64)
-.Cases("powerpc", "ppc", "ppc32", Triple::ppc)
+.Cases("powerpc", "powerpcspe", "ppc", "ppc32", Triple::ppc)
 .Cases("powerpc64", "ppu", "ppc64", Triple::ppc64)
 .Cases("powerpc64le", "ppc64le", Triple::ppc64le)
 .Case("xscale", Triple::arm)
@@ -563,6 +563,9 @@
   (SubArchName.endswith("r6el") || SubArchName.endswith("r6")))
 return Triple::MipsSubArch_r6;
 
+  if (SubArchName == "powerpcspe")
+return Triple::PPCSubArch_spe;
+
   StringRef ARMSubArch = ARM::getCanonicalArchName(SubArchName);
 
   // For now, this is the small part. Early return.
Index: llvm/include/llvm/ADT/Triple.h
===
--- llvm/include/llvm/ADT/Triple.h
+++ llvm/include/llvm/ADT/Triple.h
@@ -128,7 +128,9 @@
 KalimbaSubArch_v4,
 KalimbaSubArch_v5,
 
-MipsSubArch_r6
+MipsSubArch_r6,
+
+PPCSubArch_spe
   };
   enum VendorType {
 UnknownVendor,
Index: clang/test/Preprocessor/init.c
===
--- clang/test/Preprocessor/init.c
+++ clang/test/Preprocessor/init.c
@@ -7616,10 +7616,15 @@
 //
 // PPC32-LINUX-NOT: _CALL_LINUX
 //
-// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu -target-feature +spe < /dev/null | FileCheck -match-full-lines -check-prefix PPC32-SPE %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu -target-feature +spe < /dev/null | FileCheck -match-full-lines -check-prefix PPC32-SPE-FEAT %s
 //
-// PPC32-SPE:#define __NO_FPRS__ 1
-// PPC32-SPE:#define __SPE__ 1
+// PPC32-SPE-FEAT:#define __NO_FPRS__ 1
+// PPC32-SPE-FEAT:#define __SPE__ 1
+// 
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpcspe-unknown-linux-gnu < /dev/null | FileCheck -match-full-lines -check-prefix PPCSPE-TGT %s
+//
+// PPC32-SPE-TGT:#define __NO_FPRS__ 1
+// PPC32-SPE-TGT:#define __SPE__ 1
 // 
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc-unknown-linux-gnu -target-cpu 8548 < /dev/null | FileCheck -match-full-lines -check-prefix PPC8548 %s
 //
Index: clang/lib/Basic/Targets/PPC.h
===
--- clang/lib/Basic/Targets/PPC.h
+++ clang/lib/Basic/Targets/PPC.h
@@ -87,8 +87,7 @@
 
   // Note: GCC recognizes the following additional cpus:
   //  401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
-  //  821, 823, 8540, 8548, e300c2, e300c3, e500mc64, e6500, 860, cell,
-  //  titan, rs64.
+  //  821, 823, 8540, e300c2, e300c3, e500mc64, e6500, 860, cell, titan, rs64.
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl ) const override;
 
Index: clang/lib/Basic/Targets/PPC.cpp
===
--- clang/lib/Basic/Targets/PPC.cpp
+++ clang/lib/Basic/Targets/PPC.cpp
@@ -316,7 +316,8 @@
 .Case("pwr8", true)
 .Default(false);
 
-  Features["spe"] = llvm::StringSwitch(CPU)
+  Features["spe"] = getTriple().getSubArch() == llvm::Triple::PPCSubArch_spe ||
+llvm::StringSwitch(CPU)
 .Case("8548", true)
 .Case("e500", true)
 .Default(false);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[clang] 6bd1fcd - [OpenMP][FIX] Generalize a test check line

[Johannes Doerfert via cfe-commits]

Author: Johannes Doerfert
Date: 2019-12-30T15:01:14-06:00
New Revision: 6bd1fcd795994f484e8f974be566edbbbf23927d

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

LOG: [OpenMP][FIX] Generalize a test check line

The new check line is compatible with the clang code generation check
line as it allows a 64 and 32 bit value.

I hope this makes the llvm-clang-win-x-armv7l buildbot happy.

Added: 


Modified: 
clang/test/OpenMP/parallel_codegen.cpp

Removed: 




diff  --git a/clang/test/OpenMP/parallel_codegen.cpp 
b/clang/test/OpenMP/parallel_codegen.cpp
index 55246bfac46f..f96ad406c25f 100644
--- a/clang/test/OpenMP/parallel_codegen.cpp
+++ b/clang/test/OpenMP/parallel_codegen.cpp
@@ -112,7 +112,7 @@ int main (int argc, char **argv) {
 // ALL:   define linkonce_odr {{[a-z\_\b]*[ ]?i32}} [[TMAIN]](i8** %argc)
 // ALL:   store i8** %argc, i8*** [[ARGC_ADDR:%.+]],
 // CHECK:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* 
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], 
i{{64|32}} %{{.+}})
-// IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i64)* [[OMP_OUTLINED:@.+]] 
to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i64 %{{.+}})
+// IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* 
[[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], 
i{{64|32}} %{{.+}})
 // ALL:  ret i32 0
 // ALL-NEXT:  }
 // ALL-DEBUG:   define linkonce_odr i32 [[TMAIN]](i8** %argc)
@@ -124,7 +124,7 @@ int main (int argc, char **argv) {
 // CHECK-DEBUG:  [[KMPC_LOC_PSOURCE_REF:%.+]] = getelementptr inbounds 
%struct.ident_t, %struct.ident_t* [[LOC_2_ADDR]], i32 0, i32 4
 // CHECK-DEBUG-NEXT:  store i8* getelementptr inbounds ([{{.+}} x i8], [{{.+}} 
x i8]* [[LOC2]], i32 0, i32 0), i8** [[KMPC_LOC_PSOURCE_REF]]
 // CHECK-DEBUG-NEXT:  call void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[LOC_2_ADDR]], i32 2, void 
(i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i64)* [[OMP_OUTLINED:@.+]] 
to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i64 %{{.+}})
-// IRBUILDER-DEBUG:  call void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* @{{.*}}, i32 2, void (i32*, 
i32*, ...)* bitcast (void (i32*, i32*, i8***, i64)* [[OMP_OUTLINED:@.+]] to 
void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i64 %{{.+}})
+// IRBUILDER-DEBUG:   call void (%struct.ident_t*, i32, void (i32*, i32*, 
...)*, ...) @__kmpc_fork_call(%struct.ident_t* @{{.*}}, i32 2, void (i32*, 
i32*, ...)* bitcast (void (i32*, i32*, i8***, i64)* [[OMP_OUTLINED:@.+]] to 
void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i64 %{{.+}})
 // ALL-DEBUG:  ret i32 0
 // ALL-DEBUG-NEXT:  }
 



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

[PATCH] D72010: [CodeGen] Use CreateFNeg in buildFMulAdd

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3350
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");

craig.topper wrote:
> I removed the 'else' here because logically it didn't make sense that whether 
> we looked at negAdd should be dependent on negMul being false. The assert at 
> the beginning of the function still assures they are mutex. But the code 
> shouldn't need that assumption.
Fine by me, although it's not clear why the assertion's restriction is 
required, then.


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

https://reviews.llvm.org/D72010



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

[PATCH] D72012: [CodeGen] Use IRBuilder::CreateFNeg for __builtin_conj

[Craig Topper via Phabricator via cfe-commits]

craig.topper created this revision.
craig.topper added reviewers: spatel, efriedma, rjmccall, cameron.mcinally.

This replaces the fsub -0.0 idiom with an fneg instruction. We didn't see to 
have a test that showed the current codegen. Just some tests for constant 
folding and a test that was only checking the declare lines for libcalls. The 
latter just checked that we did not have a declare for @conj when using 
__builtin_conj.


https://reviews.llvm.org/D72012

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/complex-builtins-2.c


Index: clang/test/CodeGen/complex-builtins-2.c
===
--- /dev/null
+++ clang/test/CodeGen/complex-builtins-2.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm
  %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm 
-fmath-errno %s | FileCheck %s
+
+float _Complex test__builtin_conjf(float _Complex x) {
+// CHECK-LABEL: @test__builtin_conjf(
+// CHECK: fneg float %x.imag
+  return __builtin_conjf(x);
+}
+
+double _Complex test__builtin_conj(double _Complex x) {
+// CHECK-LABEL: @test__builtin_conj(
+// CHECK: fneg double %x.imag
+  return __builtin_conj(x);
+}
+
+long double _Complex test__builtin_conjl(long double _Complex x) {
+// CHECK-LABEL: @test__builtin_conjl(
+// CHECK: fneg x86_fp80 %x.imag
+  return __builtin_conjl(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1942,12 +1942,7 @@
 ComplexPairTy ComplexVal = EmitComplexExpr(E->getArg(0));
 Value *Real = ComplexVal.first;
 Value *Imag = ComplexVal.second;
-Value *Zero =
-  Imag->getType()->isFPOrFPVectorTy()
-? llvm::ConstantFP::getZeroValueForNegation(Imag->getType())
-: llvm::Constant::getNullValue(Imag->getType());
-
-Imag = Builder.CreateFSub(Zero, Imag, "sub");
+Imag = Builder.CreateFNeg(Imag, "neg");
 return RValue::getComplex(std::make_pair(Real, Imag));
   }
   case Builtin::BI__builtin_creal:


Index: clang/test/CodeGen/complex-builtins-2.c
===
--- /dev/null
+++ clang/test/CodeGen/complex-builtins-2.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm  %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -w -S -o - -emit-llvm -fmath-errno %s | FileCheck %s
+
+float _Complex test__builtin_conjf(float _Complex x) {
+// CHECK-LABEL: @test__builtin_conjf(
+// CHECK: fneg float %x.imag
+  return __builtin_conjf(x);
+}
+
+double _Complex test__builtin_conj(double _Complex x) {
+// CHECK-LABEL: @test__builtin_conj(
+// CHECK: fneg double %x.imag
+  return __builtin_conj(x);
+}
+
+long double _Complex test__builtin_conjl(long double _Complex x) {
+// CHECK-LABEL: @test__builtin_conjl(
+// CHECK: fneg x86_fp80 %x.imag
+  return __builtin_conjl(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1942,12 +1942,7 @@
 ComplexPairTy ComplexVal = EmitComplexExpr(E->getArg(0));
 Value *Real = ComplexVal.first;
 Value *Imag = ComplexVal.second;
-Value *Zero =
-  Imag->getType()->isFPOrFPVectorTy()
-? llvm::ConstantFP::getZeroValueForNegation(Imag->getType())
-: llvm::Constant::getNullValue(Imag->getType());
-
-Imag = Builder.CreateFSub(Zero, Imag, "sub");
+Imag = Builder.CreateFNeg(Imag, "neg");
 return RValue::getComplex(std::make_pair(Real, Imag));
   }
   case Builtin::BI__builtin_creal:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71903: [Coroutines][6/6] Clang schedules new passes

[Wenlei He via Phabricator via cfe-commits]

wenlei added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1228
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroElidePass()));
+  
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(CoroSplitPass()));
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));

Manually scheduling the 2nd coro-split passes in the same CGSCC pipeline would 
make the resume/suspend funclet ineligible for the bulk of CSGSS opts, given 
the split point is relatively early. The implication would be discouraging the 
use of coroutine in performance critical path. I would love to see this being 
addressed before we claim coroutine is ready for new PM.

As commented in the 2nd patch, we may not need the devirt trick used with 
legacy PM, instead for new PM, we could try to leverage `CGSCCUpdateResult` 
without involving artificial indirect call and devirt (or follow how outlining 
is handled by new PM).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71903



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

[PATCH] D70290: [OpenMP] Use the OpenMPIRBuilder for "omp parallel"

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10fedd94b432: [OpenMP] Use the OpenMPIRBuilder for `omp 
parallel` (authored by jdoerfert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70290

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/cancel_codegen.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -376,6 +376,10 @@
   Function *OutlinedFn = PrivAI->getFunction();
   EXPECT_NE(F, OutlinedFn);
   EXPECT_FALSE(verifyModule(*M));
+  EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoUnwind));
+  EXPECT_TRUE(OutlinedFn->hasFnAttribute(Attribute::NoRecurse));
+  EXPECT_TRUE(OutlinedFn->hasParamAttribute(0, Attribute::NoAlias));
+  EXPECT_TRUE(OutlinedFn->hasParamAttribute(1, Attribute::NoAlias));
 
   EXPECT_TRUE(OutlinedFn->hasInternalLinkage());
   EXPECT_EQ(OutlinedFn->arg_size(), 3U);
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -18,6 +18,7 @@
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/DebugInfo.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
@@ -501,10 +502,21 @@
   dbgs() << " PBR: " << BB->getName() << "\n";
   });
 
+  // Add some known attributes to the outlined function.
   Function *OutlinedFn = Extractor.extractCodeRegion(CEAC);
+  OutlinedFn->addParamAttr(0, Attribute::NoAlias);
+  OutlinedFn->addParamAttr(1, Attribute::NoAlias);
+  OutlinedFn->addFnAttr(Attribute::NoUnwind);
+  OutlinedFn->addFnAttr(Attribute::NoRecurse);
+
   LLVM_DEBUG(dbgs() << "After  outlining: " << *UI->getFunction() << "\n");
   LLVM_DEBUG(dbgs() << "   Outlined function: " << *OutlinedFn << "\n");
 
+  // For compability with the clang CG we move the outlined function after the
+  // one with the parallel region.
+  OutlinedFn->removeFromParent();
+  M.getFunctionList().insertAfter(OuterFn->getIterator(), OutlinedFn);
+
   // Remove the artificial entry introduced by the extractor right away, we
   // made our own entry block after all.
   {
@@ -535,6 +547,23 @@
   RealArgs.append(CI->arg_begin() + /* tid & bound tid */ 2, CI->arg_end());
 
   FunctionCallee RTLFn = getOrCreateRuntimeFunction(OMPRTL___kmpc_fork_call);
+  if (auto *F = dyn_cast(RTLFn.getCallee())) {
+if (!F->hasMetadata(llvm::LLVMContext::MD_callback)) {
+  llvm::LLVMContext  = F->getContext();
+  MDBuilder MDB(Ctx);
+  // Annotate the callback behavior of the __kmpc_fork_call:
+  //  - The callback callee is argument number 2 (microtask).
+  //  - The first two arguments of the callback callee are unknown (-1).
+  //  - All variadic arguments to the __kmpc_fork_call are passed to the
+  //callback callee.
+  F->addMetadata(
+  llvm::LLVMContext::MD_callback,
+  *llvm::MDNode::get(Ctx, {MDB.createCallbackEncoding(
+  2, {-1, -1},
+  /* VarArgsArePassed */ true)}));
+}
+  }
+
   Builder.CreateCall(RTLFn, RealArgs);
 
   LLVM_DEBUG(dbgs() << "With fork_call placed: "
Index: llvm/lib/Frontend/OpenMP/OMPConstants.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPConstants.cpp
+++ llvm/lib/Frontend/OpenMP/OMPConstants.cpp
@@ -65,7 +65,7 @@
 #define OMP_TYPE(VarName, InitValue) VarName = InitValue;
 #define OMP_FUNCTION_TYPE(VarName, IsVarArg, ReturnType, ...)  \
   VarName = FunctionType::get(ReturnType, {__VA_ARGS__}, IsVarArg);\
-  VarName##Ptr = PointerType::getUnqual(T);
+  VarName##Ptr = PointerType::getUnqual(VarName);
 #define OMP_STRUCT_TYPE(VarName, StructName, ...)  \
   T = M.getTypeByName(StructName); \
   if (!T)  \
Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -205,6 +205,9 @@
 #define __OMP_RTL_ATTRS(Name, FnAttrSet, RetAttrSet, ArgAttrSets)  \
   OMP_RTL_ATTRS(OMPRTL_##Name, FnAttrSet, RetAttrSet, ArgAttrSets)
 
+__OMP_RTL_ATTRS(__kmpc_fork_call, 

[PATCH] D71948: [OpenMP] Use the OpenMPIRBuilder for `cancel` directives

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
jdoerfert marked an inline comment as done.
Closed by commit rG000c6a5038bc: [OpenMP] Use the OpenMPIRBuilder for `omp 
cancel` (authored by jdoerfert).

Changed prior to commit:
  https://reviews.llvm.org/D71948?vs=235448=235629#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71948

Files:
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -99,6 +99,122 @@
   EXPECT_FALSE(verifyModule(*M));
 }
 
+TEST_F(OpenMPIRBuilderTest, CreateCancel) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  auto FiniCB = [&](InsertPointTy IP) {
+ASSERT_NE(IP.getBlock(), nullptr);
+ASSERT_EQ(IP.getBlock()->end(), IP.getPoint());
+BranchInst::Create(CBB, IP.getBlock());
+  };
+  OMPBuilder.pushFinalizationCB({FiniCB, OMPD_parallel, true});
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateCancel(Loc, nullptr, OMPD_parallel);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 4U);
+  EXPECT_EQ(BB->size(), 4U);
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Cancel = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Cancel, nullptr);
+  EXPECT_EQ(Cancel->getNumArgOperands(), 3U);
+  EXPECT_EQ(Cancel->getCalledFunction()->getName(), "__kmpc_cancel");
+  EXPECT_FALSE(Cancel->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Cancel->getCalledFunction()->doesNotFreeMemory());
+  EXPECT_EQ(Cancel->getNumUses(), 1U);
+  Instruction *CancelBBTI = Cancel->getParent()->getTerminator();
+  EXPECT_EQ(CancelBBTI->getNumSuccessors(), 2U);
+  EXPECT_EQ(CancelBBTI->getSuccessor(0), NewIP.getBlock());
+  EXPECT_EQ(CancelBBTI->getSuccessor(1)->size(), 1U);
+  EXPECT_EQ(CancelBBTI->getSuccessor(1)->getTerminator()->getNumSuccessors(),
+1U);
+  EXPECT_EQ(CancelBBTI->getSuccessor(1)->getTerminator()->getSuccessor(0),
+CBB);
+
+  EXPECT_EQ(cast(Cancel)->getArgOperand(1), GTID);
+
+  OMPBuilder.popFinalizationCB();
+
+  Builder.CreateUnreachable();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
+TEST_F(OpenMPIRBuilderTest, CreateCancelIfCond) {
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+
+  BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
+  new UnreachableInst(Ctx, CBB);
+  auto FiniCB = [&](InsertPointTy IP) {
+ASSERT_NE(IP.getBlock(), nullptr);
+ASSERT_EQ(IP.getBlock()->end(), IP.getPoint());
+BranchInst::Create(CBB, IP.getBlock());
+  };
+  OMPBuilder.pushFinalizationCB({FiniCB, OMPD_parallel, true});
+
+  IRBuilder<> Builder(BB);
+
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP()});
+  auto NewIP = OMPBuilder.CreateCancel(Loc, Builder.getTrue(), OMPD_parallel);
+  Builder.restoreIP(NewIP);
+  EXPECT_FALSE(M->global_empty());
+  EXPECT_EQ(M->size(), 3U);
+  EXPECT_EQ(F->size(), 7U);
+  EXPECT_EQ(BB->size(), 1U);
+  ASSERT_TRUE(isa(BB->getTerminator()));
+  ASSERT_EQ(BB->getTerminator()->getNumSuccessors(), 2U);
+  BB = BB->getTerminator()->getSuccessor(0);
+  EXPECT_EQ(BB->size(), 4U);
+
+
+  CallInst *GTID = dyn_cast(>front());
+  EXPECT_NE(GTID, nullptr);
+  EXPECT_EQ(GTID->getNumArgOperands(), 1U);
+  EXPECT_EQ(GTID->getCalledFunction()->getName(), "__kmpc_global_thread_num");
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(GTID->getCalledFunction()->doesNotFreeMemory());
+
+  CallInst *Cancel = dyn_cast(GTID->getNextNode());
+  EXPECT_NE(Cancel, nullptr);
+  EXPECT_EQ(Cancel->getNumArgOperands(), 3U);
+  EXPECT_EQ(Cancel->getCalledFunction()->getName(), "__kmpc_cancel");
+  EXPECT_FALSE(Cancel->getCalledFunction()->doesNotAccessMemory());
+  EXPECT_FALSE(Cancel->getCalledFunction()->doesNotFreeMemory());
+  EXPECT_EQ(Cancel->getNumUses(), 1U);
+  Instruction *CancelBBTI = Cancel->getParent()->getTerminator();
+  EXPECT_EQ(CancelBBTI->getNumSuccessors(), 2U);
+  EXPECT_EQ(CancelBBTI->getSuccessor(0)->size(), 1U);
+  

[clang] 10fedd9 - [OpenMP] Use the OpenMPIRBuilder for `omp parallel`

[Johannes Doerfert via cfe-commits]

Author: Johannes Doerfert
Date: 2019-12-30T13:57:13-06:00
New Revision: 10fedd94b4326225de4a8a1fc53594cebd501246

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

LOG: [OpenMP] Use the OpenMPIRBuilder for `omp parallel`

This allows to use the OpenMPIRBuilder for parallel regions. Code was
extracted from D61953 and adapted to work with the new version (D70109).

All but one feature should be supported. An update of this patch will
provide test coverage and privatization other than shared.

Reviewed By: fghanim

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

Added: 


Modified: 
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/cancel_codegen.cpp
clang/test/OpenMP/parallel_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
llvm/lib/Frontend/OpenMP/OMPConstants.cpp
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGStmtOpenMP.cpp 
b/clang/lib/CodeGen/CGStmtOpenMP.cpp
index a2a59b64ddb3..a38a79b6454c 100644
--- a/clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ b/clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtOpenMP.h"
 #include "clang/Basic/PrettyStackTrace.h"
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm::omp;
@@ -1318,6 +1319,87 @@ static void emitEmptyBoundParameters(CodeGenFunction &,
  llvm::SmallVectorImpl &) {}
 
 void CodeGenFunction::EmitOMPParallelDirective(const OMPParallelDirective ) {
+
+  if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
+// Check if we have any if clause associated with the directive.
+llvm::Value *IfCond = nullptr;
+if (const auto *C = S.getSingleClause())
+  IfCond = EmitScalarExpr(C->getCondition(),
+  /*IgnoreResultAssign=*/true);
+
+llvm::Value *NumThreads = nullptr;
+if (const auto *NumThreadsClause = 
S.getSingleClause())
+  NumThreads = EmitScalarExpr(NumThreadsClause->getNumThreads(),
+  /*IgnoreResultAssign=*/true);
+
+ProcBindKind ProcBind = OMP_PROC_BIND_default;
+if (const auto *ProcBindClause = S.getSingleClause())
+  ProcBind = ProcBindClause->getProcBindKind();
+
+using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
+
+// The cleanup callback that finalizes all variabels at the given location,
+// thus calls destructors etc.
+auto FiniCB = [this](InsertPointTy IP) {
+  CGBuilderTy::InsertPointGuard IPG(Builder);
+  assert(IP.getBlock()->end() != IP.getPoint() &&
+ "OpenMP IR Builder should cause terminated block!");
+  llvm::BasicBlock *IPBB = IP.getBlock();
+  llvm::BasicBlock *DestBB = IPBB->splitBasicBlock(IP.getPoint());
+  IPBB->getTerminator()->eraseFromParent();
+  Builder.SetInsertPoint(IPBB);
+  CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
+  EmitBranchThroughCleanup(Dest);
+};
+
+// Privatization callback that performs appropriate action for
+// shared/private/firstprivate/lastprivate/copyin/... variables.
+//
+// TODO: This defaults to shared right now.
+auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+ llvm::Value , llvm::Value *) {
+  // The next line is appropriate only for variables (Val) with the
+  // data-sharing attribute "shared".
+  ReplVal = 
+
+  return CodeGenIP;
+};
+
+const CapturedStmt *CS = S.getCapturedStmt(OMPD_parallel);
+const Stmt *ParallelRegionBodyStmt = CS->getCapturedStmt();
+
+auto BodyGenCB = [ParallelRegionBodyStmt,
+  this](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+llvm::BasicBlock ) {
+  auto OldAllocaIP = AllocaInsertPt;
+  AllocaInsertPt = &*AllocaIP.getPoint();
+
+  auto OldReturnBlock = ReturnBlock;
+  ReturnBlock = getJumpDestInCurrentScope();
+
+  llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+  CodeGenIPBB->splitBasicBlock(CodeGenIP.getPoint());
+  llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator();
+  CodeGenIPBBTI->removeFromParent();
+
+  Builder.SetInsertPoint(CodeGenIPBB);
+
+  EmitStmt(ParallelRegionBodyStmt);
+
+  Builder.Insert(CodeGenIPBBTI);
+
+  AllocaInsertPt = OldAllocaIP;
+  ReturnBlock = OldReturnBlock;
+};
+
+CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);
+CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, );
+Builder.restoreIP(OMPBuilder->CreateParallel(Builder, BodyGenCB, PrivCB,
+ FiniCB, 

[PATCH] D72010: [CodeGen] Use CreateFNeg in buildFMulAdd

[Craig Topper via Phabricator via cfe-commits]

craig.topper marked an inline comment as done.
craig.topper added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3350
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");

I removed the 'else' here because logically it didn't make sense that whether 
we looked at negAdd should be dependent on negMul being false. The assert at 
the beginning of the function still assures they are mutex. But the code 
shouldn't need that assumption.


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

https://reviews.llvm.org/D72010



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

[PATCH] D72010: [CodeGen] Use CreateFNeg in buildFMulAdd

[Craig Topper via Phabricator via cfe-commits]

craig.topper created this revision.
craig.topper added reviewers: cameron.mcinally, rjmccall, efriedma, rsmith.

We have an fneg instruction now and should use it instead of the fsub -0.0 
idiom. Looks like we had no test that showed that we handled the negation cases 
here so I've added new tests.


https://reviews.llvm.org/D72010

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/fp-contract-pragma.cpp


Index: clang/test/CodeGen/fp-contract-pragma.cpp
===
--- clang/test/CodeGen/fp-contract-pragma.cpp
+++ clang/test/CodeGen/fp-contract-pragma.cpp
@@ -74,3 +74,18 @@
   return (a = 2 * b) - c;
 }
 
+float fp_contract_8(float a, float b, float c) {
+// CHECK: _Z13fp_contract_8fff
+// CHECK: fneg float %c
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return a * b - c;
+}
+
+float fp_contract_9(float a, float b, float c) {
+// CHECK: _Z13fp_contract_9fff
+// CHECK: fneg float %a
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return c - a * b;
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3345,17 +3345,10 @@
 
   Value *MulOp0 = MulOp->getOperand(0);
   Value *MulOp1 = MulOp->getOperand(1);
-  if (negMul) {
-MulOp0 =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(MulOp0->getType()), MulOp0,
-"neg");
-  } else if (negAdd) {
-Addend =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(Addend->getType()), Addend,
-"neg");
-  }
+  if (negMul)
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");
 
   Value *FMulAdd = Builder.CreateCall(
   CGF.CGM.getIntrinsic(llvm::Intrinsic::fmuladd, Addend->getType()),


Index: clang/test/CodeGen/fp-contract-pragma.cpp
===
--- clang/test/CodeGen/fp-contract-pragma.cpp
+++ clang/test/CodeGen/fp-contract-pragma.cpp
@@ -74,3 +74,18 @@
   return (a = 2 * b) - c;
 }
 
+float fp_contract_8(float a, float b, float c) {
+// CHECK: _Z13fp_contract_8fff
+// CHECK: fneg float %c
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return a * b - c;
+}
+
+float fp_contract_9(float a, float b, float c) {
+// CHECK: _Z13fp_contract_9fff
+// CHECK: fneg float %a
+// CHECK: tail call float @llvm.fmuladd
+  #pragma STDC FP_CONTRACT ON
+  return c - a * b;
+}
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3345,17 +3345,10 @@
 
   Value *MulOp0 = MulOp->getOperand(0);
   Value *MulOp1 = MulOp->getOperand(1);
-  if (negMul) {
-MulOp0 =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(MulOp0->getType()), MulOp0,
-"neg");
-  } else if (negAdd) {
-Addend =
-  Builder.CreateFSub(
-llvm::ConstantFP::getZeroValueForNegation(Addend->getType()), Addend,
-"neg");
-  }
+  if (negMul)
+MulOp0 = Builder.CreateFNeg(MulOp0, "neg");
+  if (negAdd)
+Addend = Builder.CreateFNeg(Addend, "neg");
 
   Value *FMulAdd = Builder.CreateCall(
   CGF.CGM.getIntrinsic(llvm::Intrinsic::fmuladd, Addend->getType()),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4a7aa252a32a: [X86][AsmParser] re-introduce 
offset operator (authored by epastor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436

Files:
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/ms-inline-asm-64.c
  clang/test/CodeGen/ms-inline-asm.c
  clang/test/CodeGen/ms-inline-asm.cpp
  clang/test/Parser/ms-inline-asm.c
  llvm/include/llvm/MC/MCParser/MCParsedAsmOperand.h
  llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86Operand.h
  llvm/lib/Target/X86/X86AsmPrinter.cpp
  llvm/test/CodeGen/X86/ms-inline-asm.ll
  llvm/test/CodeGen/X86/offset-operator.ll
  llvm/test/MC/X86/pr32530.s

Index: llvm/test/MC/X86/pr32530.s
===
--- /dev/null
+++ llvm/test/MC/X86/pr32530.s
@@ -0,0 +1,13 @@
+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s | FileCheck %s
+
+.text
+// CHECK: movq$msg, %rsi
+// CHECK: movq$msg+314159, %rax
+// CHECK: movq$msg-89793, msg-6535(%rax,%rbx,2)
+  mov rsi,  offset msg
+	mov rax,  offset "msg" + 314159
+	mov qword ptr [rax + 2*rbx + offset msg - 6535],  offset msg - 89793
+.data
+msg:
+  .ascii "Hello, world!\n"
+
Index: llvm/test/CodeGen/X86/offset-operator.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/offset-operator.ll
@@ -0,0 +1,15 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -x86-asm-syntax=intel -relocation-model=static < %s | FileCheck %s
+
+; Test we are emitting the 'offset' operator upon an immediate reference of a label:
+; The emitted 'att-equivalent' of this one is "movl $.L.str, %eax"
+
+@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
+
+define i8* @test_offset_operator() {
+; CHECK-LABEL: test_offset_operator:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:mov eax, offset .L.str
+; CHECK-NEXT:ret
+entry:
+  ret i8* getelementptr inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0)
+}
Index: llvm/test/CodeGen/X86/ms-inline-asm.ll
===
--- llvm/test/CodeGen/X86/ms-inline-asm.ll
+++ llvm/test/CodeGen/X86/ms-inline-asm.ll
@@ -92,7 +92,7 @@
 ; CHECK-LABEL: t30:
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
-; CHECK: lea edi, dword ptr [{{_?}}results]
+; CHECK: lea edi, dword ptr [offset {{_?}}results]
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 ; CHECK: {{## InlineAsm Start|#APP}}
Index: llvm/lib/Target/X86/X86AsmPrinter.cpp
===
--- llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -220,8 +220,16 @@
 
   case MachineOperand::MO_ConstantPoolIndex:
   case MachineOperand::MO_GlobalAddress: {
-if (IsATT)
+switch (MI->getInlineAsmDialect()) {
+default:
+  llvm_unreachable("unknown assembly dialect!");
+case InlineAsm::AD_ATT:
   O << '$';
+  break;
+case InlineAsm::AD_Intel:
+  O << "offset ";
+  break;
+}
 PrintSymbolOperand(MO, O);
 break;
   }
Index: llvm/lib/Target/X86/AsmParser/X86Operand.h
===
--- llvm/lib/Target/X86/AsmParser/X86Operand.h
+++ llvm/lib/Target/X86/AsmParser/X86Operand.h
@@ -53,6 +53,7 @@
 
   struct ImmOp {
 const MCExpr *Val;
+bool LocalRef;
   };
 
   struct MemOp {
@@ -279,13 +280,9 @@
 return isImmUnsignedi8Value(CE->getValue());
   }
 
-  bool isOffsetOf() const override {
-return OffsetOfLoc.getPointer();
-  }
+  bool isOffsetOfLocal() const override { return isImm() && Imm.LocalRef; }
 
-  bool needAddressOf() const override {
-return AddressOf;
-  }
+  bool needAddressOf() const override { return AddressOf; }
 
   bool isCallOperand() const override { return CallOperand; }
   void setCallOperand(bool IsCallOperand) { CallOperand = IsCallOperand; }
@@ -617,9 +614,16 @@
   }
 
   static std::unique_ptr CreateImm(const MCExpr *Val,
-   SMLoc StartLoc, SMLoc EndLoc) {
+   SMLoc StartLoc, SMLoc EndLoc,
+   StringRef SymName = StringRef(),
+   void *OpDecl = nullptr,
+   bool GlobalRef = true) {
 auto Res = std::make_unique(Immediate, StartLoc, EndLoc);
-Res->Imm.Val = Val;
+Res->Imm.Val  = Val;
+Res->Imm.LocalRef = !GlobalRef;
+Res->SymName  = SymName;
+Res->OpDecl   = OpDecl;
+Res->AddressOf= true;
 return Res;
   }
 
Index: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

[clang] 4a7aa25 - [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via cfe-commits]

Author: Eric Astor
Date: 2019-12-30T14:35:26-05:00
New Revision: 4a7aa252a32a94b1bb61b3dc7f027b4a27ae334f

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

LOG: [X86][AsmParser] re-introduce 'offset' operator

Summary:
Amend MS offset operator implementation, to more closely fit with its MS 
counterpart:

1. InlineAsm: evaluate non-local source entities to their (address) location
2. Provide a mean with which one may acquire the address of an assembly 
label via MS syntax, rather than yielding a memory reference (i.e. "offset 
asm_label" and "$asm_label" should be synonymous
3. address PR32530

Based on http://llvm.org/D37461

Fix broken test where the break appears unrelated.

- Set up appropriate memory-input rewrites for variable references.

- Intel-dialect assembly printing now correctly handles addresses by adding 
"offset".

- Pass offsets as immediate operands (using "r" constraint for offsets of 
locals).

Reviewed By: rnk

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

Added: 
llvm/test/CodeGen/X86/offset-operator.ll
llvm/test/MC/X86/pr32530.s

Modified: 
clang/lib/Sema/SemaStmtAsm.cpp
clang/test/CodeGen/ms-inline-asm-64.c
clang/test/CodeGen/ms-inline-asm.c
clang/test/CodeGen/ms-inline-asm.cpp
clang/test/Parser/ms-inline-asm.c
llvm/include/llvm/MC/MCParser/MCParsedAsmOperand.h
llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h
llvm/lib/MC/MCParser/AsmParser.cpp
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
llvm/lib/Target/X86/AsmParser/X86Operand.h
llvm/lib/Target/X86/X86AsmPrinter.cpp
llvm/test/CodeGen/X86/ms-inline-asm.ll

Removed: 




diff  --git a/clang/lib/Sema/SemaStmtAsm.cpp b/clang/lib/Sema/SemaStmtAsm.cpp
index 5161b8001918..93faf2d151f9 100644
--- a/clang/lib/Sema/SemaStmtAsm.cpp
+++ b/clang/lib/Sema/SemaStmtAsm.cpp
@@ -707,8 +707,13 @@ void Sema::FillInlineAsmIdentifierInfo(Expr *Res,
   if (T->isFunctionType() || T->isDependentType())
 return Info.setLabel(Res);
   if (Res->isRValue()) {
-if (isa(T) && Res->EvaluateAsRValue(Eval, Context))
+bool IsEnum = isa(T);
+if (DeclRefExpr *DRE = dyn_cast(Res))
+  if (DRE->getDecl()->getKind() == Decl::EnumConstant)
+IsEnum = true;
+if (IsEnum && Res->EvaluateAsRValue(Eval, Context))
   return Info.setEnum(Eval.Val.getInt().getSExtValue());
+
 return Info.setLabel(Res);
   }
   unsigned Size = Context.getTypeSizeInChars(T).getQuantity();

diff  --git a/clang/test/CodeGen/ms-inline-asm-64.c 
b/clang/test/CodeGen/ms-inline-asm-64.c
index 5b144eb7bb68..ce46b8821dee 100644
--- a/clang/test/CodeGen/ms-inline-asm-64.c
+++ b/clang/test/CodeGen/ms-inline-asm-64.c
@@ -12,10 +12,10 @@ void t1() {
 
 void t2() {
   int var = 10;
-  __asm mov [eax], offset var
+  __asm mov qword ptr [eax], offset var
 // CHECK: t2
 // CHECK: call void asm sideeffect inteldialect
-// CHECK-SAME: mov [eax], $0
+// CHECK-SAME: mov qword ptr [eax], $0
 // CHECK-SAME: "r,~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}})
 }
 

diff  --git a/clang/test/CodeGen/ms-inline-asm.c 
b/clang/test/CodeGen/ms-inline-asm.c
index 0c9b35a64523..17526f522311 100644
--- a/clang/test/CodeGen/ms-inline-asm.c
+++ b/clang/test/CodeGen/ms-inline-asm.c
@@ -190,14 +190,20 @@ void t15() {
 // CHECK: mov eax, $1
   __asm mov eax, offset gvar ; eax = address of gvar
 // CHECK: mov eax, $2
-// CHECK: "*m,r,r,~{eax},~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}}, i32* 
%{{.*}}, i32* @{{.*}})
+  __asm mov eax, offset gvar+1 ; eax = 1 + address of gvar
+// CHECK: mov eax, $3 + $$1
+  __asm mov eax, 1+offset gvar ; eax = 1 + address of gvar
+// CHECK: mov eax, $4 + $$1
+  __asm mov eax, 1+offset gvar+1 ; eax = 2 + address of gvar
+// CHECK: mov eax, $5 + $$2
+// CHECK: "*m,r,i,i,i,i,~{eax},~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}}, i32* 
%{{.*}}, i32* @{{.*}}, i32* @{{.*}}, i32* @{{.*}}, i32* @{{.*}})
 }
 
 void t16() {
   int var = 10;
-  __asm mov [eax], offset var
+  __asm mov dword ptr [eax], offset var
 // CHECK: t16
-// CHECK: call void asm sideeffect inteldialect "mov [eax], $0", 
"r,~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}})
+// CHECK: call void asm sideeffect inteldialect "mov dword ptr [eax], $0", 
"r,~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}})
 }
 
 void t17() {

diff  --git a/clang/test/CodeGen/ms-inline-asm.cpp 
b/clang/test/CodeGen/ms-inline-asm.cpp
index 58796ed6378d..463ff0f6e349 100644
--- a/clang/test/CodeGen/ms-inline-asm.cpp
+++ b/clang/test/CodeGen/ms-inline-asm.cpp
@@ -40,7 +40,7 @@ void t2() {
 // CHECK: call void asm sideeffect inteldialect
 // CHECK-SAME: mov eax, $0
 // CHECK-SAME: mov eax, $1
-// CHECK-SAME: "r,r,~{eax},~{dirflag},~{fpsr},~{flags}"(i32** @_ZN3Foo3ptrE, 
i32** @_ZN3Foo3Bar3ptrE)
+// CHECK-SAME: "i,i,~{eax},~{dirflag},~{fpsr},~{flags}"(i32** 

[PATCH] D71991: Fix external-names.c test when separator is \\

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth accepted this revision.
amccarth added a comment.

I've made a similar change in several other tests, but this test works for me 
without this change.  I'm trying to understand why, but I don't see any harm in 
landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71991



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

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Pavlo Shkrabliuk via Phabricator via cfe-commits]

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

@MyDeveloperDay, thanks for looking into this. Added a comment


Repository:
  rC Clang

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

https://reviews.llvm.org/D71939



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

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Pavlo Shkrabliuk via Phabricator via cfe-commits]

pastey updated this revision to Diff 235621.

Repository:
  rC Clang

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

https://reviews.llvm.org/D71939

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column 
limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception ) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column 
limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"}\n"
+"catch (...) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(...){baz();}", Style));
 }
 
 
//===--===//
@@ -14881,7 +14916,7 @@
   verifyFormat("auto lambda = [ = a]() { a = 2; };", AlignStyle);
 }
 
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInConditionalStatement = true;
   verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -326,6 +326,19 @@
   FormatStyle::BWACS_Always)
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
+} else if (I[1]->First->is(tok::l_brace) &&
+   TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
+   Style.BraceWrapping.AfterControlStatement ==
+   FormatStyle::BWACS_MultiLine) {
+  // This case if different from the upper BWACS_MultiLine processing
+  // in that a preceding r_brace is not on the same line as else/catch
+  // most likely because of BeforeElse/BeforeCatch set to true.
+  // If the line length doesn't fit ColumnLimit, leave l_brace on the
+  // next line to respect the BWACS_MultiLine.
+  return (Style.ColumnLimit == 0 ||
+  TheLine->Last->TotalLength <= Style.ColumnLimit)
+ ? 1
+ : 0;
 }
 // Try to merge either empty or one-line block if is precedeed by control
 // statement token


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception ) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"}\n"
+"catch (...) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(...){baz();}", Style));
 }
 
 //===--===//
@@ -14881,7 +14916,7 @@
   verifyFormat("auto lambda = [ = a]() { a = 2; };", AlignStyle);
 }
 
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInConditionalStatement = 

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

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

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61144 tests passed, 0 failed 
and 728 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436



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

[PATCH] D71969: [OpenMP] diagnose zero-length array section in the depend clause

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 added a comment.

In D71969#1798224 , @jdoerfert wrote:

> Is there a reason not to put this check right next to the one that issues 
> `err_omp_section_length_negative`. SemaExpr.cpp +4668


It is a good idea to group the checking together however this check is specific 
for the depend clause.  The map clause allows the zero-length array section.  
Isn't it better to keep it in ActOnOpenMPDependClause?


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

https://reviews.llvm.org/D71969



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

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

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

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436



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

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via Phabricator via cfe-commits]

epastor updated this revision to Diff 235620.
epastor added a comment.

- The three-argument version of find_if is std::find_if, and the qualification 
is required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436

Files:
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/ms-inline-asm-64.c
  clang/test/CodeGen/ms-inline-asm.c
  clang/test/CodeGen/ms-inline-asm.cpp
  clang/test/Parser/ms-inline-asm.c
  llvm/include/llvm/MC/MCParser/MCParsedAsmOperand.h
  llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86Operand.h
  llvm/lib/Target/X86/X86AsmPrinter.cpp
  llvm/test/CodeGen/X86/ms-inline-asm.ll
  llvm/test/CodeGen/X86/offset-operator.ll
  llvm/test/MC/X86/pr32530.s

Index: llvm/test/MC/X86/pr32530.s
===
--- /dev/null
+++ llvm/test/MC/X86/pr32530.s
@@ -0,0 +1,13 @@
+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s | FileCheck %s
+
+.text
+// CHECK: movq$msg, %rsi
+// CHECK: movq$msg+314159, %rax
+// CHECK: movq$msg-89793, msg-6535(%rax,%rbx,2)
+  mov rsi,  offset msg
+	mov rax,  offset "msg" + 314159
+	mov qword ptr [rax + 2*rbx + offset msg - 6535],  offset msg - 89793
+.data
+msg:
+  .ascii "Hello, world!\n"
+
Index: llvm/test/CodeGen/X86/offset-operator.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/offset-operator.ll
@@ -0,0 +1,15 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -x86-asm-syntax=intel -relocation-model=static < %s | FileCheck %s
+
+; Test we are emitting the 'offset' operator upon an immediate reference of a label:
+; The emitted 'att-equivalent' of this one is "movl $.L.str, %eax"
+
+@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
+
+define i8* @test_offset_operator() {
+; CHECK-LABEL: test_offset_operator:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:mov eax, offset .L.str
+; CHECK-NEXT:ret
+entry:
+  ret i8* getelementptr inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0)
+}
Index: llvm/test/CodeGen/X86/ms-inline-asm.ll
===
--- llvm/test/CodeGen/X86/ms-inline-asm.ll
+++ llvm/test/CodeGen/X86/ms-inline-asm.ll
@@ -92,7 +92,7 @@
 ; CHECK-LABEL: t30:
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
-; CHECK: lea edi, dword ptr [{{_?}}results]
+; CHECK: lea edi, dword ptr [offset {{_?}}results]
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 ; CHECK: {{## InlineAsm Start|#APP}}
Index: llvm/lib/Target/X86/X86AsmPrinter.cpp
===
--- llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -220,8 +220,16 @@
 
   case MachineOperand::MO_ConstantPoolIndex:
   case MachineOperand::MO_GlobalAddress: {
-if (IsATT)
+switch (MI->getInlineAsmDialect()) {
+default:
+  llvm_unreachable("unknown assembly dialect!");
+case InlineAsm::AD_ATT:
   O << '$';
+  break;
+case InlineAsm::AD_Intel:
+  O << "offset ";
+  break;
+}
 PrintSymbolOperand(MO, O);
 break;
   }
Index: llvm/lib/Target/X86/AsmParser/X86Operand.h
===
--- llvm/lib/Target/X86/AsmParser/X86Operand.h
+++ llvm/lib/Target/X86/AsmParser/X86Operand.h
@@ -53,6 +53,7 @@
 
   struct ImmOp {
 const MCExpr *Val;
+bool LocalRef;
   };
 
   struct MemOp {
@@ -279,13 +280,9 @@
 return isImmUnsignedi8Value(CE->getValue());
   }
 
-  bool isOffsetOf() const override {
-return OffsetOfLoc.getPointer();
-  }
+  bool isOffsetOfLocal() const override { return isImm() && Imm.LocalRef; }
 
-  bool needAddressOf() const override {
-return AddressOf;
-  }
+  bool needAddressOf() const override { return AddressOf; }
 
   bool isCallOperand() const override { return CallOperand; }
   void setCallOperand(bool IsCallOperand) { CallOperand = IsCallOperand; }
@@ -617,9 +614,16 @@
   }
 
   static std::unique_ptr CreateImm(const MCExpr *Val,
-   SMLoc StartLoc, SMLoc EndLoc) {
+   SMLoc StartLoc, SMLoc EndLoc,
+   StringRef SymName = StringRef(),
+   void *OpDecl = nullptr,
+   bool GlobalRef = true) {
 auto Res = std::make_unique(Immediate, StartLoc, EndLoc);
-Res->Imm.Val = Val;
+Res->Imm.Val  = Val;
+Res->Imm.LocalRef = !GlobalRef;
+Res->SymName  = SymName;
+Res->OpDecl   = OpDecl;
+Res->AddressOf= true;
 return Res;
   }
 
Index: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

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

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61144 tests passed, 0 failed 
and 728 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436



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

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via Phabricator via cfe-commits]

epastor updated this revision to Diff 235619.
epastor marked 5 inline comments as done.
epastor added a comment.

- Fix a few final issues.
  - Pack the AsmRewrite struct more optimally.
  - Only check if we can fuse rewrites with ones we haven't already reached!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436

Files:
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/ms-inline-asm-64.c
  clang/test/CodeGen/ms-inline-asm.c
  clang/test/CodeGen/ms-inline-asm.cpp
  clang/test/Parser/ms-inline-asm.c
  llvm/include/llvm/MC/MCParser/MCParsedAsmOperand.h
  llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86Operand.h
  llvm/lib/Target/X86/X86AsmPrinter.cpp
  llvm/test/CodeGen/X86/ms-inline-asm.ll
  llvm/test/CodeGen/X86/offset-operator.ll
  llvm/test/MC/X86/pr32530.s

Index: llvm/test/MC/X86/pr32530.s
===
--- /dev/null
+++ llvm/test/MC/X86/pr32530.s
@@ -0,0 +1,13 @@
+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s | FileCheck %s
+
+.text
+// CHECK: movq$msg, %rsi
+// CHECK: movq$msg+314159, %rax
+// CHECK: movq$msg-89793, msg-6535(%rax,%rbx,2)
+  mov rsi,  offset msg
+	mov rax,  offset "msg" + 314159
+	mov qword ptr [rax + 2*rbx + offset msg - 6535],  offset msg - 89793
+.data
+msg:
+  .ascii "Hello, world!\n"
+
Index: llvm/test/CodeGen/X86/offset-operator.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/offset-operator.ll
@@ -0,0 +1,15 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -x86-asm-syntax=intel -relocation-model=static < %s | FileCheck %s
+
+; Test we are emitting the 'offset' operator upon an immediate reference of a label:
+; The emitted 'att-equivalent' of this one is "movl $.L.str, %eax"
+
+@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
+
+define i8* @test_offset_operator() {
+; CHECK-LABEL: test_offset_operator:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:mov eax, offset .L.str
+; CHECK-NEXT:ret
+entry:
+  ret i8* getelementptr inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0)
+}
Index: llvm/test/CodeGen/X86/ms-inline-asm.ll
===
--- llvm/test/CodeGen/X86/ms-inline-asm.ll
+++ llvm/test/CodeGen/X86/ms-inline-asm.ll
@@ -92,7 +92,7 @@
 ; CHECK-LABEL: t30:
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
-; CHECK: lea edi, dword ptr [{{_?}}results]
+; CHECK: lea edi, dword ptr [offset {{_?}}results]
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 ; CHECK: {{## InlineAsm Start|#APP}}
Index: llvm/lib/Target/X86/X86AsmPrinter.cpp
===
--- llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -220,8 +220,16 @@
 
   case MachineOperand::MO_ConstantPoolIndex:
   case MachineOperand::MO_GlobalAddress: {
-if (IsATT)
+switch (MI->getInlineAsmDialect()) {
+default:
+  llvm_unreachable("unknown assembly dialect!");
+case InlineAsm::AD_ATT:
   O << '$';
+  break;
+case InlineAsm::AD_Intel:
+  O << "offset ";
+  break;
+}
 PrintSymbolOperand(MO, O);
 break;
   }
Index: llvm/lib/Target/X86/AsmParser/X86Operand.h
===
--- llvm/lib/Target/X86/AsmParser/X86Operand.h
+++ llvm/lib/Target/X86/AsmParser/X86Operand.h
@@ -53,6 +53,7 @@
 
   struct ImmOp {
 const MCExpr *Val;
+bool LocalRef;
   };
 
   struct MemOp {
@@ -279,13 +280,9 @@
 return isImmUnsignedi8Value(CE->getValue());
   }
 
-  bool isOffsetOf() const override {
-return OffsetOfLoc.getPointer();
-  }
+  bool isOffsetOfLocal() const override { return isImm() && Imm.LocalRef; }
 
-  bool needAddressOf() const override {
-return AddressOf;
-  }
+  bool needAddressOf() const override { return AddressOf; }
 
   bool isCallOperand() const override { return CallOperand; }
   void setCallOperand(bool IsCallOperand) { CallOperand = IsCallOperand; }
@@ -617,9 +614,16 @@
   }
 
   static std::unique_ptr CreateImm(const MCExpr *Val,
-   SMLoc StartLoc, SMLoc EndLoc) {
+   SMLoc StartLoc, SMLoc EndLoc,
+   StringRef SymName = StringRef(),
+   void *OpDecl = nullptr,
+   bool GlobalRef = true) {
 auto Res = std::make_unique(Immediate, StartLoc, EndLoc);
-Res->Imm.Val = Val;
+Res->Imm.Val  = Val;
+Res->Imm.LocalRef = !GlobalRef;
+Res->SymName  = SymName;
+Res->OpDecl   = OpDecl;
+Res->AddressOf= 

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via Phabricator via cfe-commits]

epastor added inline comments.



Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1821
+getParser().parsePrimaryExpr(Val, End))
+  return Error(Start, "unexpected token!");
+  } else if (ParseIntelInlineAsmIdentifier(Val, ID, Info, false, End, true)) {

rnk wrote:
> epastor wrote:
> > epastor wrote:
> > > rnk wrote:
> > > > Please test this corner case, I imagine it looks like:
> > > >   mov eax, offset 3
> > > Interesting. This corner case didn't trigger in that scenario; we get an 
> > > "expected identifier" error message with good source location, followed 
> > > by another error "use of undeclared label '3'" in debug builds... and in 
> > > release builds, we instead get a crash. On tracing the crash, it's a 
> > > AsmStrRewrite applying to a SMLoc not coming from the same string...
> > > 
> > > As near as I can tell, the issue is that we end up trying to parse "3" as 
> > > a not-yet-declared label, as such expand it to 
> > > `__MSASMLABEL_.${:uid}__3`, and then end up in a bad state because the 
> > > operand rewrite is applying to the expanded symbol... which isn't in the 
> > > same AsmString. If you use an actual undeclared label, you hit the same 
> > > crash in release builds.
> > > 
> > > This is going to take some work; I'll get back to it in a day or two.
> > Fixed; we now get the same errors in this scenario as we do in the current 
> > LLVM trunk, reporting "expected identifier" and "use of undeclared label 
> > '3'".
> > 
> > On the other hand, I'm still working on finding a scenario that DOES 
> > trigger this corner case.
> I see. Well, it sounds like it was a useful test. :)
Very useful indeed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436



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

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via Phabricator via cfe-commits]

epastor updated this revision to Diff 235617.
epastor added a comment.

Rebase on top of HEAD

- Add compatibility with dc5b614fa9a1 
 (which 
changed handling for call operands in x86 assembly)
- Remove another artifact of local testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436

Files:
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/CodeGen/ms-inline-asm-64.c
  clang/test/CodeGen/ms-inline-asm.c
  clang/test/CodeGen/ms-inline-asm.cpp
  clang/test/Parser/ms-inline-asm.c
  llvm/include/llvm/MC/MCParser/MCParsedAsmOperand.h
  llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
  llvm/lib/Target/X86/AsmParser/X86Operand.h
  llvm/lib/Target/X86/X86AsmPrinter.cpp
  llvm/test/CodeGen/X86/ms-inline-asm.ll
  llvm/test/CodeGen/X86/offset-operator.ll
  llvm/test/MC/X86/pr32530.s

Index: llvm/test/MC/X86/pr32530.s
===
--- /dev/null
+++ llvm/test/MC/X86/pr32530.s
@@ -0,0 +1,13 @@
+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s | FileCheck %s
+
+.text
+// CHECK: movq$msg, %rsi
+// CHECK: movq$msg+314159, %rax
+// CHECK: movq$msg-89793, msg-6535(%rax,%rbx,2)
+  mov rsi,  offset msg
+	mov rax,  offset "msg" + 314159
+	mov qword ptr [rax + 2*rbx + offset msg - 6535],  offset msg - 89793
+.data
+msg:
+  .ascii "Hello, world!\n"
+
Index: llvm/test/CodeGen/X86/offset-operator.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/offset-operator.ll
@@ -0,0 +1,15 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu -x86-asm-syntax=intel -relocation-model=static < %s | FileCheck %s
+
+; Test we are emitting the 'offset' operator upon an immediate reference of a label:
+; The emitted 'att-equivalent' of this one is "movl $.L.str, %eax"
+
+@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
+
+define i8* @test_offset_operator() {
+; CHECK-LABEL: test_offset_operator:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:mov eax, offset .L.str
+; CHECK-NEXT:ret
+entry:
+  ret i8* getelementptr inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0)
+}
Index: llvm/test/CodeGen/X86/ms-inline-asm.ll
===
--- llvm/test/CodeGen/X86/ms-inline-asm.ll
+++ llvm/test/CodeGen/X86/ms-inline-asm.ll
@@ -92,7 +92,7 @@
 ; CHECK-LABEL: t30:
 ; CHECK: {{## InlineAsm Start|#APP}}
 ; CHECK: .intel_syntax
-; CHECK: lea edi, dword ptr [{{_?}}results]
+; CHECK: lea edi, dword ptr [offset {{_?}}results]
 ; CHECK: .att_syntax
 ; CHECK: {{## InlineAsm End|#NO_APP}}
 ; CHECK: {{## InlineAsm Start|#APP}}
Index: llvm/lib/Target/X86/X86AsmPrinter.cpp
===
--- llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -220,8 +220,16 @@
 
   case MachineOperand::MO_ConstantPoolIndex:
   case MachineOperand::MO_GlobalAddress: {
-if (IsATT)
+switch (MI->getInlineAsmDialect()) {
+default:
+  llvm_unreachable("unknown assembly dialect!");
+case InlineAsm::AD_ATT:
   O << '$';
+  break;
+case InlineAsm::AD_Intel:
+  O << "offset ";
+  break;
+}
 PrintSymbolOperand(MO, O);
 break;
   }
Index: llvm/lib/Target/X86/AsmParser/X86Operand.h
===
--- llvm/lib/Target/X86/AsmParser/X86Operand.h
+++ llvm/lib/Target/X86/AsmParser/X86Operand.h
@@ -53,6 +53,7 @@
 
   struct ImmOp {
 const MCExpr *Val;
+bool LocalRef;
   };
 
   struct MemOp {
@@ -279,13 +280,9 @@
 return isImmUnsignedi8Value(CE->getValue());
   }
 
-  bool isOffsetOf() const override {
-return OffsetOfLoc.getPointer();
-  }
+  bool isOffsetOfLocal() const override { return isImm() && Imm.LocalRef; }
 
-  bool needAddressOf() const override {
-return AddressOf;
-  }
+  bool needAddressOf() const override { return AddressOf; }
 
   bool isCallOperand() const override { return CallOperand; }
   void setCallOperand(bool IsCallOperand) { CallOperand = IsCallOperand; }
@@ -617,9 +614,16 @@
   }
 
   static std::unique_ptr CreateImm(const MCExpr *Val,
-   SMLoc StartLoc, SMLoc EndLoc) {
+   SMLoc StartLoc, SMLoc EndLoc,
+   StringRef SymName = StringRef(),
+   void *OpDecl = nullptr,
+   bool GlobalRef = true) {
 auto Res = std::make_unique(Immediate, StartLoc, EndLoc);
-Res->Imm.Val = Val;
+Res->Imm.Val  = Val;
+Res->Imm.LocalRef = !GlobalRef;
+Res->SymName  = SymName;
+Res->OpDecl 

[PATCH] D71991: Fix external-names.c test when separator is \\

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

lgtm, but why doesn't this show up on existing clang windows bots? For example:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/13276

IIRC clang often joins paths with `/`, so maybe this separator is always `/` 
upstream. Do you have some downstream patch or setting to change that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71991



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

[PATCH] D71948: [OpenMP] Use the OpenMPIRBuilder for `cancel` directives

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:241
+  // This seems to be used only once without much change of reuse, could live 
in
+  // OMPKinds.def but seems not necessary.
+  Value *CancelKind = nullptr;

JonChesterfield wrote:
> The integer numbers correspond to the `kmp_cancel_kind_t` enum in 
> `runtime/src/kmp.h`. The target offloading presently ignores this argument, 
> but the host version has a control flow dependency on it.
> 
> I think the enum should be shared between the compiler and the runtime, or 
> failing that, some test code should include kmp.h and check the numbers still 
> match.
> 
> This feels like a familiar point - I've just sent an email to openmp-dev to 
> discuss whether we can share constants between the two without copy & paste.
I move them into OMPKinds.def, we can share them from there. I'll respond to 
the mailing list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71948



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

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I think generally speaking this looks ok, I'm ok with the extra clause, as long 
as it's clear what it's doing




Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:329
  : 0;
+} else if (I[1]->First->is(tok::l_brace) &&
+   TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&

nit: maybe leave a comment explaining the need for the extra clause


Repository:
  rC Clang

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

https://reviews.llvm.org/D71939



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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 235611.
JonasToth added a comment.

- fix brace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,982 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  StringRef T = "using MyInt = int*;";
+  StringRef S = "MyInt target = 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 235610.
JonasToth marked 14 inline comments as done.
JonasToth added a comment.

- create fresh branch with latest diff of the revision
- fix review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,982 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:34-41
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {
+  return isa(QT.getTypePtr());
+}
+static bool isPointerType(const Type *T) { return isa(T); }
+static bool isPointerType(QualType QT) {
+  return isPointerType(QT.getTypePtr());

aaron.ballman wrote:
> I don't see how these are improvements over checking `QT->isArrayType()` and 
> friends within the caller.
True!



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:82
+return (llvm::Twine(' ') + DeclSpec::getSpecifierName(Qualifier)).str();
+  return (llvm::Twine(DeclSpec::getSpecifierName(Qualifier)) + llvm::Twine(' 
'))
+  .str();

aaron.ballman wrote:
> Do you need both casts for `Twine`? I would imagine only the first is needed.
In case of `' '` it is necessary, in case of `" "` its not. I go with `" "`, 
should not be more expensive or anything, right?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:166
+
+  llvm_unreachable("All paths should have been handled");
+}

aaron.ballman wrote:
> I think this path is reachable -- it should be an `assert()` instead and 
> return `None`.
When testing LLVM i saw some path triggering the assertions and unreachables.

I want to go the `None`-route for now and only emit warnings without fixit. 
That will help to find false-positives unhandled cases better and is less 
problematic.

Is it only objc-code that could trigger this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

There are many comments not marked as done. could you please do that with 
addressed issues? It is hard to otherwise see whats still left.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:110
+   "making it const")
+  << MemberVariable; // FIXME: Add fix-it hint to MemberVariable
+}

vingeldal wrote:
> JonasToth wrote:
> > missing `return`?
> No, one of the below matchers may also be valid, in case we have a non-const 
> member variable which  is also a pointer or reference to non-const data.
I see. Please add a `FALLTHROUGH` comment or the like, as it breaks the pattern 
a lot and is not obvious from local code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth abandoned this revision.
JonasToth added a comment.
Herald added a subscriber: whisperity.

Abonding this revision in favor of: https://reviews.llvm.org/D54943
It doesn't make sense to have both open at the same time, as the 
const-transformation happens in its own patch and no heavy lifting is done here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45444



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

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

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

xbolva00 added a comment.

if(num != 0) memmove is missed optimization opportunity. Such branch can be 
removed (if profitable).


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

https://reviews.llvm.org/D63324



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

[PATCH] D71842: Allow newlines in AST Matchers in clang-query files

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: clang/include/clang/ASTMatchers/Dynamic/Parser.h:167
   static llvm::Optional
-  parseMatcherExpression(StringRef MatcherCode, Sema *S,
- const NamedValueMap *NamedValues,
- Diagnostics *Error);
+  parseMatcherExpression(StringRef , Sema *S,
+ const NamedValueMap *NamedValues, Diagnostics *Error);

Sorry for the late review comment, but I'd really appreciate if you could 
update the doc comments to explain why MatcherCode is taken by reference, and 
how it is mutated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71842



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

[PATCH] D71969: [OpenMP] diagnose zero-length array section in the depend clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15031-15033
+Length->EvaluateAsInt(Result, Context)) {
+  llvm::APSInt ConstLength = Result.Val.getInt();
+  if (ConstLength.getSExtValue() == 0) {

Just `.. Length->EvaluateAsInt(Result, Context) && 
Result.Val.getInt().isNullValue()) {`


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

https://reviews.llvm.org/D71969



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

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thinking about it more, i have some negative reservations about this :/

As it can be seen form https://godbolt.org/z/3BZmCM, it seems any and every(?) 
alternative C++ algorithm
replacement is a performance pessimization in the general case, because 
`memcpy` requires/'guarantees'
that there must be no overlap between source and destination ranges,
while there is no such restriction for C++ algorithms (=> they will get 
optimized into `memmove`).
That is unless optimizer also manages to prove that it is safe to optimize 
`memmove` into `memcpy` there.
(Extra bloat `if(num != 0) memmove` is also not always good)


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

https://reviews.llvm.org/D63324



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:110
+   "making it const")
+  << MemberVariable; // FIXME: Add fix-it hint to MemberVariable
+}

JonasToth wrote:
> missing `return`?
No, one of the below matchers may also be valid, in case we have a non-const 
member variable which  is also a pointer or reference to non-const data.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal updated this revision to Diff 235600.
vingeldal marked 2 inline comments as done.
vingeldal added a comment.

Updating D70265 : [clang-tidy] Add clang tidy 
check I.2 to cppcoreguidelines

- Fixed code example in documentation to make it valid code.
- Fixed the documentation link to I.2 C++ Core Guidelines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables-IgnoreDataMembers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -0,0 +1,268 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-non-const-global-variables %t
+
+int nonConstInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int  = nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'nonConstIntReference' provides global access to non-const type, consider making the referenced data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int *pointerToNonConstInt = 
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'pointerToNonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: variable 'pointerToNonConstInt' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int *const constPointerToNonConstInt = 
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'constPointerToNonConstInt' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+namespace namespace_name {
+int nonConstNamespaceInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+const int constNamespaceInt = 0;
+} // namespace namespace_name
+
+const int constInt = 0;
+
+const int *pointerToConstInt = 
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'pointerToConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+const int *const constPointerToConstInt = 
+
+const int  = constInt;
+
+constexpr int constexprInt = 0;
+
+int function() {
+  int nonConstReturnValue = 0;
+  return nonConstReturnValue;
+}
+
+namespace {
+int nonConstAnonymousNamespaceInt = 0;
+}
+
+class DummyClass {
+public:
+  int nonConstMemberVariable = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: member variable 'nonConstMemberVariable' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int constMemberVariable = 0;
+  int *const constMemberPointerToNonConst = 
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member variable 'constMemberPointerToNonConst' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+  int *memberPointerToNonConst = 
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member variable 'memberPointerToNonConst' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: member variable 'memberPointerToNonConst' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int *const constMemberPointerToConst = 
+  int  = nonConstInt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member variable 'nonConstMemberReferenceToNonConst' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: member variable 

[PATCH] D61989: [clang-tidy] enable modernize-concat-nested-namespaces on header files

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Sorry for not responding to the review. I was very busy with other things and 
overlooked the mails :(




Comment at: 
clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:82
 ast_matchers::MatchFinder *Finder) {
-  if (!getLangOpts().CPlusPlus17)
+  if (!getLangOpts().CPlusPlus17 && !getLangOpts().CPlusPlus2a)
 return;

We would need to update that line for each new standard, that won't happen :/
I think the opposite logic should be used here and blacklist known-not working 
standards (and C).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61989



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

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.
Herald added subscribers: wuzish, whisperity.

You need tests for this check. Please harden them against macro-interference as 
well. I think the transformation should happen as function call that return 
`Optional` and if any failure happens the diagnostic can still be emitted.




Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:27
+void ReplaceMemcpyByStdCopy::registerMatchers(MatchFinder *Finder) {
+  assert(Finder != nullptr);
+

this assertion is not necessary. `Finder` is always not-null.



Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:33
+  auto MemcpyMatcher =
+  callExpr(hasDeclaration(functionDecl(hasName("memcpy"),
+   isExpansionInSystemHeader())),

please match on `::memcpy` and `::std::memcpy`.



Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:53
+  const auto *MemcpyNode = Result.Nodes.getNodeAs("memcpy_function");
+  assert(MemcpyNode != nullptr);
+

this assertion is not necessary, as you only match on one thing and that means 
it exists. (otherwise everything is broken already).


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

https://reviews.llvm.org/D63324



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

[PATCH] D65912: [clang-tidy] Add new check for math constants

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.
Herald added a subscriber: whisperity.

In D65912#1623375 , @ZaMaZaN4iK wrote:

> > ... then I have a script that runs clang-tidy over all the compilation 
> > units in a compilation database
>
> Can you share this script please? :)


I did a bit of work on infrastructure for testing at some point, but never 
really finished something that works great and nice, just some script-ideas you 
can utilize.

You can use https://reviews.llvm.org/D54141 to deduplicate messages, especially 
such constants might be declared in headers that are included often.
Once you deduped the diagnostics you can pipe the output of `run-clang-tidy.py` 
in a file and do simple line counts and scroll though to get an idea of your 
checks output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65912



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:110
+   "making it const")
+  << MemberVariable; // FIXME: Add fix-it hint to MemberVariable
+}

missing `return`?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:6
+
+Finds non-const global variables as described in check I.2 of C++ Core
+Guidelines.

I would prefer if the `I.2 of C++ Core Guidelines` is a link and to remove the 
link-string below this paragraph.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:29
+  char * e_ptr1 =   // Warns!
+  char *const e_const_ptr  // Warns!
+  char & e_reference = e;  // Warns!

This is technically not valid code, as the pointer needs to be initialized and 
a `;` is missing. I think that should be fixed to not mislead inexperienced 
programmers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

Re: [clang] 878a24e - Reapply "Fix crash on switch conditions of non-integer types in templates"

[Alexander Kornienko via cfe-commits]

Any news here? The problem is still there.

On Fri, Dec 13, 2019 at 12:33 AM Andrews, Elizabeth <
elizabeth.andr...@intel.com> wrote:

> Hi Alexander,
>
>
>
> I am debugging this now but I assume the checks for the if condition were
> skipped before this commit (therefore no crash) because ‘c’ was set as type
> dependent. The checks are probably run now since c’s type dependency
> changed with this patch. The checks might need to be guarded better, but I
> am not sure.  I will take a look and get back to you ASAP.
>
>
>
> Elizabeth
>
>
>
> *From:* Alexander Kornienko 
> *Sent:* Wednesday, December 11, 2019 2:35 PM
> *To:* Andrews, Elizabeth ; Elizabeth Andrews
> 
> *Cc:* cfe-commits 
> *Subject:* Re: [clang] 878a24e - Reapply "Fix crash on switch conditions
> of non-integer types in templates"
>
>
>
> After this commit clang started generating assertion failures on valid
> code. There are tons of instances in our codebase. Here's a reduced test
> case:
>
> template
> class a {
>   int c : b;
>   void f() {
> if (c)
>   ;
>   }
> };
>
>
>
> Please take a look.
>
>
>
> On Wed, Dec 4, 2019 at 12:39 AM Elizabeth Andrews via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>
> Author: Elizabeth Andrews
> Date: 2019-12-03T15:27:19-08:00
> New Revision: 878a24ee244a24c39d1c57e9af2e88c621f7cce9
>
> URL:
> https://github.com/llvm/llvm-project/commit/878a24ee244a24c39d1c57e9af2e88c621f7cce9
> DIFF:
> https://github.com/llvm/llvm-project/commit/878a24ee244a24c39d1c57e9af2e88c621f7cce9.diff
>
> LOG: Reapply "Fix crash on switch conditions of non-integer types in
> templates"
>
> This patch reapplies commit 759948467ea. Patch was reverted due to a
> clang-tidy test fail on Windows. The test has been modified. There
> are no additional code changes.
>
> Patch was tested with ninja check-all on Windows and Linux.
>
> Summary of code changes:
>
> Clang currently crashes for switch statements inside a template when the
> condition is a non-integer field member because contextual implicit
> conversion is skipped when parsing the condition. This conversion is
> however later checked in an assert when the case statement is handled.
> The conversion is skipped when parsing the condition because
> the field member is set as type-dependent based on its containing class.
> This patch sets the type dependency based on the field's type instead.
>
> This patch fixes Bug 40982.
>
> Added:
> clang/test/SemaTemplate/non-integral-switch-cond.cpp
>
> Modified:
>
> clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> clang/lib/AST/Expr.cpp
> clang/lib/Sema/SemaChecking.cpp
> clang/test/SemaCXX/constant-expression-cxx2a.cpp
> clang/test/SemaTemplate/dependent-names.cpp
> clang/test/SemaTemplate/enum-argument.cpp
> clang/test/SemaTemplate/member-access-expr.cpp
>
> Removed:
>
>
>
>
> 
> diff  --git
> a/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> b/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> index 18fe5ef4e5c2..2c288e0bbddf 100644
> ---
> a/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> +++
> b/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> @@ -1,4 +1,4 @@
> -// RUN: %check_clang_tidy %s bugprone-string-integer-assignment %t
> +// RUN: %check_clang_tidy %s bugprone-string-integer-assignment %t -- --
> -fno-delayed-template-parsing
>
>  namespace std {
>  template
> @@ -103,6 +103,8 @@ struct S {
>static constexpr T t = 0x8000;
>std::string s;
>void f(char c) { s += c | static_cast(t); }
> +  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: an integer is interpreted
> as a chara
> +  // CHECK-FIXES: {{^}}  void f(char c) { s += std::to_string(c |
> static_cast(t)); }
>  };
>
>  template S;
>
> diff  --git
> a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> index 119eff67318e..8e546b44ab74 100644
> --- a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> +++ b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> @@ -233,7 +233,7 @@ struct a {
>  template 
>  class d {
>a e;
> -  void f() { e.b(); }
> +  void f() { e.b(0); }
>  };
>  }  // namespace
>  }  // namespace PR38055
>
> diff  --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
> index 322b3a7fa740..a73531ad5fad 100644
> --- a/clang/lib/AST/Expr.cpp
> +++ b/clang/lib/AST/Expr.cpp
> @@ -1678,6 +1678,15 @@ MemberExpr *MemberExpr::Create(
>MemberExpr *E = new (Mem) MemberExpr(Base, IsArrow, OperatorLoc,
> MemberDecl,
> NameInfo, T, VK, OK, NOUR);
>
> +  if (FieldDecl *Field = dyn_cast(MemberDecl)) {
> +DeclContext 

[PATCH] D71963: clang-tidy doc: Add the severities description

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

@whisperity I think you misunderstood my comment. I was not trying to give a 
more correct description of the current definition of style-level severity in 
CodeChecker. I was trying to propose **new** definitions of the different 
severity levels that this patch propose for clang-tidy.
So the claim that my suggestion is not "true" is invalid since that isn't a 
matter of true or false, but merely a matter of how one defines the style level.

My reason for proposing these new definitions is that I think my proposals 
highlight how I think that the levels could be more distinctly different in 
their definition instead. I also think these severity levels need more distinct 
definitions to be more useful than they are problematic. I'm afraid that 
definitions which people may interpret differently are at significant risk of 
causing more confusion than clarification.
If severity levels must be exactly like they are currently defined in 
CodeChecker then IMO that is a rather strong reason not to introduce them in 
clang-tidy and just keep that stuff in CodeChecker.

> A `low` diagnostics (and everything "above", assuming a(n at least) partial 
> ordering on severities) should mean the coding construct is problematic: 
> there is a chance -- perhaps once in a lifetime, perhaps not -- that doing 
> this will cause a real error. A `style` thing should mean //"Hey, whatever 
> you have written, is pretty much A-Ok! and works, congrats for writing valid 
> (Obj)?C(++)?, but please realise that we are writing DERP/C++-42 and not C89 
> and please move your loop variable inside the loop's statement"//. No 
> **real** "game-breaking" issue should ever arise from deciding on fixing or 
> ignoring a `style` check's output.

If we are to only distinguish severity level based on the probability that 
violating the rule will cause an error then we should support every choice of 
severity level with data, something which may be quite hard, for most 
contributors writing a new check, to actually achieve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71963



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

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:19
+
+static constexpr llvm::StringLiteral CallExprName = "call";
+

I think you can remove the constant and hardcode it in the matcher. that is 
common practice and because its used twice not a big issue (and shorter).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:21
+
+static bool hasPadding(const clang::ASTContext , const RecordDecl *RD,
+   uint64_t ComparedBits);

You are in `clang` namespace, so you can ellide the `clang::` here and in the 
following functions/decls.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:27
+ const clang::ASTContext ) {
+  if (FD.isBitField())
+return FD.getBitWidthValue(Ctx);

Maybe `return FD.isBitField() ? FD.getBitWidthValue(Ctx) : 
Ctx.getTypeSize(FieldType);`? I find it cleaner, but its preference i guess.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:46
+  NonEmptyBaseIt->getType()->getAsCXXRecordDecl();
+  const uint64_t SizeOfBase = Ctx.getTypeSize(BaseRD->getTypeForDecl());
+  TotalSize += SizeOfBase;

value-types are not defined as `const` per coding guideline.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:62
+  for (const auto  : RD->fields()) {
+const uint64_t FieldOffset = Ctx.getFieldOffset(Field);
+assert(FieldOffset >= TotalSize);

plz no `const`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:63
+const uint64_t FieldOffset = Ctx.getFieldOffset(Field);
+assert(FieldOffset >= TotalSize);
+

a short descriptive error message for this assertion would be great.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:66
+if (FieldOffset > TotalSize && TotalSize < ComparedBits)
+  // Padding before this field
+  return true;

Please make this comment a full sentence with punctuation. I think it should be 
above the `if`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:72
+
+const uint64_t SizeOfField = getFieldSize(*Field, Field->getType(), Ctx);
+TotalSize += SizeOfField;

`const`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:76
+if (Field->getType()->isRecordType()) {
+  // Check if comparing padding in nested record
+  if (hasPadding(Ctx, Field->getType()->getAsRecordDecl()->getDefinition(),

Please make this comment a full sentence with punctuation.

Both ifs can be merged to one with an `&&`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:88
+   uint64_t ComparedBits) {
+  assert(RD && RD->isCompleteDefinition());
+  if (RD->isUnion())

Please provide an error message



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:98
+
+  // check for trailing padding
+  return TotalSize < Ctx.getTypeSize(RD->getTypeForDecl()) &&

Full sentence for comment.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:109
+CharUnits::fromQuantity(Result.Val.getInt().getExtValue()));
+  else
+return None;

No `else` after return. You can simply `return None;`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:125
+  const auto *CE = Result.Nodes.getNodeAs(CallExprName);
+  assert(CE != nullptr);
+

Always true, otherwise matcher don't work / didn't fire. I think this assert is 
not required.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:128
+  const Expr *SizeExpr = CE->getArg(2);
+  assert(SizeExpr != nullptr);
+  const llvm::Optional ComparedBits =

please provide a short error message why this should always be true (i guess 
because memcmp is standardized and stuff, but when reading this code alone it 
might not be super obvious).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:129
+  assert(SizeExpr != nullptr);
+  const llvm::Optional ComparedBits =
+  tryEvaluateSizeExpr(SizeExpr, Ctx);

value-type that is `const`, please remove the `const`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:134
+const Expr 

[PATCH] D71111: [Sema] Improve diagnostic about addr spaces for overload candidates

[Adhemerval Zanella via Phabricator via cfe-commits]

zatrazz added a comment.

Hi,

I am investigating a recurring regression on aarch64-linux bots and bisecting 
the commits on the build [1] that introduced the regression it points to this 
one. I don't understand exactly what is triggering the issue, but it only 
happen on the stage2 build with stage1 clang built with different compilers 
(gcc 5.4, gcc-7.4.0, and gcc-8.3.0). The issue is on clang itself while trying 
to execute the Analysis/uninit-sometimes.cpp test:

   TEST 'Clang :: Analysis/uninit-sometimes.cpp' FAILED 

  Script:
  --
  : 'RUN: at line 1';   
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 
-internal-isystem 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include
 -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -verify 
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp
  : 'RUN: at line 2';   
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 
-internal-isystem 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include
 -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized 
-fdiagnostics-parseable-fixits 
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp
 2>&1 | 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/FileCheck 
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp
  --
  Exit Code: 134
  
  Command Output (stderr):
  --
  clang: 
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/lib/Basic/Diagnostic.cpp:591:
 void HandleSelectModifier(const clang::Diagnostic &, unsigned int, const char 
*, unsigned int, SmallVectorImpl &): Assertion `NextVal != ArgumentEnd && 
"Value for integer select modifier was" " larger than the number of options in 
the diagnostic string!"' failed.
  Stack dump:
  0.Program arguments: 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 
-internal-isystem 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include
 -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -verify 
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp
 
  1.
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp:161:1:
 current parser token 'int'
  2.
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp:146:32:
 parsing function body 'test_for_range_true'
  #0 0x0182be04 PrintStackTraceSignalHandler(void*) 
(/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang+0x182be04)
  
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/tools/clang/test/Analysis/Output/uninit-sometimes.cpp.script:
 line 2: 63297 Aborted (core dumped) 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 
-internal-isystem 
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include
 -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -verify 
/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp

The bots run a ubuntu 16.04 container and I can't reproduce on a different one, 
I am still trying to come up with a better reproduce case. Also, moving the 
both DiagnosticBuilder::AddString and DiagnosticBuilder::AddTaggedVal outside 
clang/include/clang/Basic/Diagnostic.h to clang/lib/Basic/Diagnostic.cpp 
"fixes" the issue (indicating the something is wrong at code generation). This 
issue has been shown for some time on aarch64-linux bot [2]. Any idea if this 
is an environmental issue or a real issue?

[1] http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/8562
[2] http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



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

[PATCH] D71963: clang-tidy doc: Add the severities description

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D71963#1798199 , @vingeldal wrote:

> - Style: things that are handled by clang-format rather than clang-tidy.


This is not true, for two reasons. The shorter answer: In case it was true, the 
"severity category" `STYLE` would be a moot point in CodeChecker, as 
CodeChecker (currently) catalogues and displays CSA and CT diagnostics.

Moreover, there are plenty of stylistic constructs (`readability-`, some 
`modernise-`, etc.) which are not handled, and maybe can not be handled by 
`clang-format` on a "token sequence" level.
For example, using `record-member-init-expr`s instead of initializers in the 
constructor 

 is not something `clang-format` could reasonably fix as per my understanding. 
It is also not something it //should// fix, whereas a Tidy rule being enabled 
(or not...) for a project is the project's decision about what stylistics or 
nomenclature 
 (hah, 
maybe this check, in particular, should be moved from `low` to `style` 
@sylvestre.ledru ) they want to embrace.

A `low` diagnostics (and everything "above", assuming a(n at least) partial 
ordering on severities) should mean the coding construct is problematic: there 
is a chance -- perhaps once in a lifetime, perhaps not -- that doing this will 
cause a real error. A `style` thing should mean //"Hey, whatever you have 
written, is pretty much A-Ok! and works, congrats for writing valid 
(Obj)?C(++)?, but please realise that we are writing DERP/C++-42 and not C89 
and please move your loop variable inside the loop's statement"//. No **real** 
"game-breaking" issue should ever arise from deciding on fixing or ignoring a 
`style` check's output.

-

One thing which I forgot to mention (because 99.999% of the people don't do 
it, so it's understandable that it's been missed) is that CodeChecker 
severities can be fine-tuned as it's literally just a JSON file. I haven't 
touched actual CodeChecker code for a while now, but I think in in case you 
rewrite this JSON as you see fit before running an analysis or starting a 
server, you can go as arbitrary as you want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71963



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

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone, ReadabilityBracesAroundStatements and ReadabilityMisleadingIndentation

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you for working on this!

In D71980#1798680 , @njames93 wrote:

> In D71980#1798517 , @lebedev.ri 
> wrote:
>
> > Missing tests; please upload patches with full context (`-U9`); just 
> > referencing bug# is non-informative - best to say `"[clang-tidy] 
> > check-name: fix 'constexpr if' handling (PR#)"`; possibly you want to split 
> > this up into per-check patches
>
>
> I was thinking as these are all manifestations of the same bug they could go 
> as one patch. As for the test cases, how do you write an effective test when 
> you are dealing with template instantiations given that the code will 
> effectively be checked multiple times for the definition and each 
> instantiation


The bug-reports had cases that reproduced the bugs, you could use those (or 
minimal versions of them).
When you write the templates, you just need to ensure that they are 
instantiated at some point.

  template 
  T my_sqrt(T val) {
// This is fake, just to make a templated condition that does not rely on 
the standard library or anything else (tests should not have dependencies in 
LLVM and we do not allow the STL in tests)
if constexpr (sizeof(T) == 8) {
return 42.0;
}
else {
  return 42.0f;
}
  }
  
  void instantiate() {
my_sqrt(10.);
my_sqrt(10.f);
  }

These should trigger the problem. A test with `if constexpr () ... else if 
constexpr () ...` would be required as well.

To test each of these checks you can use multiple `RUN: ` lines in a single 
test-file. 
You can see 
`clang-tools-extra/test/clang-tidy/checkers/misc-non-private-member-variables-in-classes.cpp`
 for a test that takes heavy advantage from that.
When writing the test please add them to the same directory 
(test/clang-tidy/checkers/) and follow the same naming 
conventions as those files.


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

https://reviews.llvm.org/D71980



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

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 235577.
AlexanderLanin added a comment.

Hi,
I don't see the error message you mentioned anywhere, but I'm gonna assume it 
involves the number of surrounding lines. Here is another attempt with

> git show HEAD -U99 > mypatch.patch



---

Source of problem:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
 tells me to use

> git format-patch -U99 @ {u}

But then upload fails:

> Diff Parse Exception: Expected a hunk header, like 'Index: /path/to/file.ext' 
> (svn), 'Property changes on: /path/to/file.ext' (svn properties), 'commit 
> 59bcc3ad6775562f845953cf01624225' (git show), 'diff --git' (git diff), '--- 
> filename' (unified diff), or 'diff -r' (hg diff or patch).
> 
>   1885for more information.
>   1886
>   1887
>   1888
>   1889
>   >>> 1890   -- 
>   1891   2.24.1.windows.2
>   1892   

https://llvm.org/docs/GettingStarted.html#sending-patches just casually 
mentions git show and git format-patch without telling anything about options 
to be used.

Assuming 'git show HEAD -U99' is the one and only correct way? 'git diff 
HEAD~1 -U99' seems to work ok also.
However 'git format-patch' doesn't work as mentioned above. At least for me 
with git version 2.24.1.windows.2


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

https://reviews.llvm.org/D71982

Files:
  clang-tools-extra/docs/clang-include-fixer.rst
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/docs/pp-trace.rst
  clang/www/hacking.html

Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -298,7 +298,7 @@
   Note that the paths embedded in the patch depend on where you run it,
   so changing directory to the llvm/tools/clang directory is recommended.
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git to contribute to Clang.
 
   
   LLVM IR Generation
Index: clang-tools-extra/docs/pp-trace.rst
===
--- clang-tools-extra/docs/pp-trace.rst
+++ clang-tools-extra/docs/pp-trace.rst
@@ -104,16 +104,16 @@
 
   ---
   - Callback: FileChanged
-Loc: "c:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "c:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
 (etc.)
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:5:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:5:1"
 Reason: ExitFile
 FileType: C_User
-PrevFID: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
+PrevFID: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/Input/Level1B.h"
   - Callback: EndOfMainFile
   ...
 
@@ -172,7 +172,7 @@
 Example:::
 
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
@@ -248,8 +248,8 @@
 FileName: "Input/Level1B.h"
 IsAngled: false
 FilenameRange: "Input/Level1B.h"
-File: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
-SearchPath: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace"
+File: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/Input/Level1B.h"
+SearchPath: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace"
 RelativePath: "Input/Level1B.h"
 Imported: (null)
 
@@ -271,8 +271,8 @@
 Example:::
 
   - Callback: moduleImport
-ImportLoc: "d:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:2"
-Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
+ImportLoc: "d:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:2"
+Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
 Imported: Level2B
 
 `EndOfMainFile `_ Callback
@@ -309,7 +309,7 @@
 Example:::
 
   - Callback: Ident
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-ident.cpp:3:1"
+Loc: 

[PATCH] D71948: [OpenMP] Use the OpenMPIRBuilder for `cancel` directives

[Jon Chesterfield via Phabricator via cfe-commits]

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

I like this, thanks. Very clear.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:241
+  // This seems to be used only once without much change of reuse, could live 
in
+  // OMPKinds.def but seems not necessary.
+  Value *CancelKind = nullptr;

The integer numbers correspond to the `kmp_cancel_kind_t` enum in 
`runtime/src/kmp.h`. The target offloading presently ignores this argument, but 
the host version has a control flow dependency on it.

I think the enum should be shared between the compiler and the runtime, or 
failing that, some test code should include kmp.h and check the numbers still 
match.

This feels like a familiar point - I've just sent an email to openmp-dev to 
discuss whether we can share constants between the two without copy & paste.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71948



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

[PATCH] D71991: Fix external-names.c test when separator is \\

[Mikhail Maltsev via Phabricator via cfe-commits]

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

LGTM. Let's wait for someone else to approve (or commit after holidays if no 
one objects).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71991



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235573.
njames93 added a comment.

Fixed a few more edge cases with test cases


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+  int i = 0;
+expr(nullPointerConstant())
+  

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone, ReadabilityBracesAroundStatements and ReadabilityMisleadingIndentation

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235570.
njames93 retitled this revision from "[clang-tidy] Fix bug - 44229, 33203 and 
32204" to "[clang-tidy] Disable Checks on If constexpr statements in template 
Instantiations for BugproneBranchClone, ReadabilityBracesAroundStatements and 
ReadabilityMisleadingIndentation".
njames93 added a comment.

In D71980#1798517 , @lebedev.ri wrote:

> Missing tests; please upload patches with full context (`-U9`); just 
> referencing bug# is non-informative - best to say `"[clang-tidy] check-name: 
> fix 'constexpr if' handling (PR#)"`; possibly you want to split this up into 
> per-check patches


I was thinking as these are all manifestations of the same bug they could go as 
one patch. As for the test cases, how do you write an effective test when you 
are dealing with template instantiations given that the code will effectively 
be checked multiple times for the definition and each instantiation


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

https://reviews.llvm.org/D71980

Files:
  clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp


Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ hasElse(stmt()))
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),
Index: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -123,7 +123,10 @@
 }
 
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt().bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation(
+  .bind("if"),
+  this);
   Finder->addMatcher(whileStmt().bind("while"), this);
   Finder->addMatcher(doStmt().bind("do"), this);
   Finder->addMatcher(forStmt().bind("for"), this);
Index: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -59,7 +59,8 @@
 
 void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-  ifStmt(stmt().bind("if"),
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ stmt().bind("if"),
  hasParent(stmt(unless(ifStmt(hasElse(equalsBoundNode("if")),
  hasElse(stmt().bind("else"))),
   this);


Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
+ hasElse(stmt()))
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),
Index: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -123,7 +123,10 @@
 }
 
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt().bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation(
+  .bind("if"),
+  this);
   Finder->addMatcher(whileStmt().bind("while"), this);
   Finder->addMatcher(doStmt().bind("do"), this);
   Finder->addMatcher(forStmt().bind("for"), this);
Index: 

[clang] 4a188fd - [OpenCL] Add mipmap builtin functions

[Sven van Haastregt via cfe-commits]

Author: Sven van Haastregt
Date: 2019-12-30T10:47:58Z
New Revision: 4a188fdfa79b4c1044cbb6fe0ede79583c71a56f

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

LOG: [OpenCL] Add mipmap builtin functions

Add the mipmap builtin functions from the OpenCL extension
specification.

Patch by Pierre Gondois and Sven van Haastregt.

Added: 


Modified: 
clang/lib/Sema/OpenCLBuiltins.td

Removed: 




diff  --git a/clang/lib/Sema/OpenCLBuiltins.td 
b/clang/lib/Sema/OpenCLBuiltins.td
index 38e07b21cb85..72b72f60a662 100644
--- a/clang/lib/Sema/OpenCLBuiltins.td
+++ b/clang/lib/Sema/OpenCLBuiltins.td
@@ -59,6 +59,10 @@ def FuncExtKhrLocalInt32BaseAtomics  : 
FunctionExtension<"cl_khr_local_int32
 def FuncExtKhrLocalInt32ExtendedAtomics  : 
FunctionExtension<"cl_khr_local_int32_extended_atomics">;
 def FuncExtKhrInt64BaseAtomics   : 
FunctionExtension<"cl_khr_int64_base_atomics">;
 def FuncExtKhrInt64ExtendedAtomics   : 
FunctionExtension<"cl_khr_int64_extended_atomics">;
+def FuncExtKhrMipmapImage: 
FunctionExtension<"cl_khr_mipmap_image">;
+
+// Multiple extensions
+def FuncExtKhrMipmapAndWrite3d   : 
FunctionExtension<"cl_khr_mipmap_image cl_khr_3d_image_writes">;
 
 // Qualified Type.  These map to ASTContext::QualType.
 class QualType {
@@ -1179,3 +1183,176 @@ let MinVersion = CL20 in {
 def get_num_sub_groups : Builtin<"get_num_sub_groups", [UInt]>;
   }
 }
+
+//
+// End of the builtin functions defined in the OpenCL C specification.
+// Builtin functions defined in the OpenCL C Extension are below.
+//
+
+
+// OpenCL Extension v2.0 s9.18 - Mipmaps
+let Extension = FuncExtKhrMipmapImage in {
+  // Added to section 6.13.14.2.
+  foreach aQual = ["RO"] in {
+foreach imgTy = [Image2d] in {
+  foreach name = ["read_imagef"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, VectorType, VectorType], 
Attr.Pure>;
+  }
+  foreach name = ["read_imagei"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, VectorType, VectorType], 
Attr.Pure>;
+  }
+  foreach name = ["read_imageui"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, VectorType, VectorType], 
Attr.Pure>;
+  }
+}
+foreach imgTy = [Image2dDepth] in {
+  foreach name = ["read_imagef"] in {
+def : Builtin, Sampler, 
VectorType, Float], Attr.Pure>;
+def : Builtin, Sampler, 
VectorType, VectorType, VectorType], Attr.Pure>;
+  }
+}
+foreach imgTy = [Image1d] in {
+  foreach name = ["read_imagef"] in {
+def : Builtin, ImageType, 
Sampler, Float, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, Float, Float, Float], Attr.Pure>;
+  }
+  foreach name = ["read_imagei"] in {
+def : Builtin, ImageType, 
Sampler, Float, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, Float, Float, Float], Attr.Pure>;
+  }
+  foreach name = ["read_imageui"] in {
+def : Builtin, ImageType, 
Sampler, Float, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, Float, Float, Float], Attr.Pure>;
+  }
+}
+foreach imgTy = [Image3d] in {
+  foreach name = ["read_imagef"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, VectorType, VectorType], 
Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+  }
+  foreach name = ["read_imagei"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, VectorType, VectorType], 
Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+  }
+  foreach name = ["read_imageui"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, VectorType, VectorType], 
Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+  }
+}
+foreach imgTy = [Image1dArray] in {
+  foreach name = ["read_imagef"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, Float, Float], Attr.Pure>;
+  }
+  foreach name = ["read_imagei"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, Float], Attr.Pure>;
+def : Builtin, ImageType, 
Sampler, VectorType, Float, Float], Attr.Pure>;
+  }
+  foreach name = ["read_imageui"] in {
+def : Builtin, ImageType, 
Sampler, VectorType, 

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71499



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Adding checks to see if there are any declarations already in the ifs contained 
scope is really starting to bloat the code while trying to cover every edge 
case.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71965: include missing for std::abort

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. Do you have commit access or should we land this for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71965



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

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

+1, please upload a diff with full context 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71982



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

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 235564.
gbencze added a comment.

ReleaseNotes: move alias after new checks


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

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,352 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+
+typedef unsigned long long size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+namespace std {
+int memcmp(const void *lhs, const void *rhs, size_t count);
+}
+
+namespace sei_cert_example_exp42_c {
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+
+void noncompliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: comparing padding bytes
+  }
+}
+
+void compliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (left->c == right->c) && (left->i == right->i) &&
+  (0 == memcmp(left->buffer, right->buffer, 13))) {
+  }
+}
+} // namespace sei_cert_example_exp42_c
+
+namespace sei_cert_example_exp42_c_ex1 {
+#pragma pack(push, 1)
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+#pragma pack(pop)
+
+void compare(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// no-warning
+  }
+}
+} // namespace sei_cert_example_exp42_c_ex1
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace trailing_padding {
+struct S {
+  int i;
+  char c;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(int));
+  memcmp(, , sizeof(int) + sizeof(char));
+  memcmp(, , 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace trailing_padding
+
+namespace inner_padding {
+struct S {
+  char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char) + sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char));
+  memcmp(, , 2 * sizeof(char));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace inner_padding
+
+namespace bitfield {
+struct S {
+  int x : 10;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2); // no-warning: no padding in first 2 bytes
+}
+} // namespace bitfield
+
+namespace bitfield_2 {
+struct S {
+  int x : 10;
+  int y : 7;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2);
+  memcmp(, , 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_2
+
+namespace bitfield_3 {
+struct S {
+  int x : 2;
+  int : 0;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_3
+
+namespace array_no_padding {
+struct S {
+  int x;
+  int y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  memcmp(a, b, 3 * sizeof(S));
+}
+} // namespace array_no_padding
+
+namespace array_with_padding {
+struct S {
+  int x;
+  char y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  memcmp(a, b, 3 * sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace array_with_padding
+
+namespace unknown_count {
+struct S {
+  char c;
+  int i;
+};
+
+void test(size_t c) {
+  S a;
+  S b;
+  

[PATCH] D71991: Fix external-names.c test when separator is \\

[Michael Platings via Phabricator via cfe-commits]

michaelplatings created this revision.
michaelplatings added reviewers: rnk, amccarth, dsanders, miyuki.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
michaelplatings edited the summary of this revision.

This fixes the following failure:

  C:\[...]\llvm\tools\clang\test\VFS\external-names.c:34:26: error: 
CHECK-DEBUG-EXTERNAL: expected string not found in input
  // CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: 
"{{[^"]*}}Inputs{{.}}external-names.h"
   ^
  [...]
  :42:54: note: possible intended match here
  !10 = !DIFile(filename: 
"C:/[...]\\llvm\\tools\\clang\\test\\VFS\\Inputs\\external-names.h", directory: 
"")


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71991

Files:
  clang/test/VFS/external-names.c


Index: clang/test/VFS/external-names.c
===
--- clang/test/VFS/external-names.c
+++ clang/test/VFS/external-names.c
@@ -21,7 +21,7 @@
 // Diagnostics:
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -fsyntax-only %s 2>&1 | 
FileCheck -check-prefix=CHECK-DIAG-EXTERNAL %s
-// CHECK-DIAG-EXTERNAL: {{.*}}Inputs{{.}}external-names.h:{{[0-9]*:[0-9]*}}: 
warning: incompatible pointer
+// CHECK-DIAG-EXTERNAL: {{.*}}Inputs{{..?}}external-names.h:{{[0-9]*:[0-9]*}}: 
warning: incompatible pointer
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -fsyntax-only %s 2>&1 | 
FileCheck -check-prefix=CHECK-DIAG %s
 // CHECK-DIAG-NOT: Inputs
@@ -31,7 +31,7 @@
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -triple 
%itanium_abi_triple -debug-info-kind=limited -emit-llvm %s -o - | FileCheck 
-check-prefix=CHECK-DEBUG-EXTERNAL %s
 // CHECK-DEBUG-EXTERNAL: !DISubprogram({{.*}}file: ![[Num:[0-9]+]]
-// CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: 
"{{[^"]*}}Inputs{{.}}external-names.h"
+// CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: 
"{{[^"]*}}Inputs{{..?}}external-names.h"
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -triple %itanium_abi_triple 
-debug-info-kind=limited -emit-llvm %s -o - | FileCheck 
-check-prefix=CHECK-DEBUG %s
 // CHECK-DEBUG-NOT: Inputs
@@ -42,7 +42,7 @@
 // RUN: %clang_cc1 -D REINCLUDE -I %t -ivfsoverlay %t.external.yaml -Eonly %s 
-MTfoo -dependency-file %t.external.dep
 // RUN: echo "EOF" >> %t.external.dep
 // RUN: cat %t.external.dep | FileCheck --check-prefix=CHECK-DEP-EXTERNAL %s
-// CHECK-DEP-EXTERNAL: Inputs{{.}}external-names.h
+// CHECK-DEP-EXTERNAL: Inputs{{..?}}external-names.h
 // CHECK-DEP-EXTERNAL-NEXT: EOF
 
 // RUN: %clang_cc1 -D REINCLUDE -I %t -ivfsoverlay %t.yaml -Eonly %s -MTfoo 
-dependency-file %t.dep


Index: clang/test/VFS/external-names.c
===
--- clang/test/VFS/external-names.c
+++ clang/test/VFS/external-names.c
@@ -21,7 +21,7 @@
 // Diagnostics:
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-DIAG-EXTERNAL %s
-// CHECK-DIAG-EXTERNAL: {{.*}}Inputs{{.}}external-names.h:{{[0-9]*:[0-9]*}}: warning: incompatible pointer
+// CHECK-DIAG-EXTERNAL: {{.*}}Inputs{{..?}}external-names.h:{{[0-9]*:[0-9]*}}: warning: incompatible pointer
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHECK-DIAG %s
 // CHECK-DIAG-NOT: Inputs
@@ -31,7 +31,7 @@
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.external.yaml -triple %itanium_abi_triple -debug-info-kind=limited -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-DEBUG-EXTERNAL %s
 // CHECK-DEBUG-EXTERNAL: !DISubprogram({{.*}}file: ![[Num:[0-9]+]]
-// CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: "{{[^"]*}}Inputs{{.}}external-names.h"
+// CHECK-DEBUG-EXTERNAL: ![[Num]] = !DIFile(filename: "{{[^"]*}}Inputs{{..?}}external-names.h"
 
 // RUN: %clang_cc1 -I %t -ivfsoverlay %t.yaml -triple %itanium_abi_triple -debug-info-kind=limited -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-DEBUG %s
 // CHECK-DEBUG-NOT: Inputs
@@ -42,7 +42,7 @@
 // RUN: %clang_cc1 -D REINCLUDE -I %t -ivfsoverlay %t.external.yaml -Eonly %s -MTfoo -dependency-file %t.external.dep
 // RUN: echo "EOF" >> %t.external.dep
 // RUN: cat %t.external.dep | FileCheck --check-prefix=CHECK-DEP-EXTERNAL %s
-// CHECK-DEP-EXTERNAL: Inputs{{.}}external-names.h
+// CHECK-DEP-EXTERNAL: Inputs{{..?}}external-names.h
 // CHECK-DEP-EXTERNAL-NEXT: EOF
 
 // RUN: %clang_cc1 -D REINCLUDE -I %t -ivfsoverlay %t.yaml -Eonly %s -MTfoo -dependency-file %t.dep
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb47b35ff51b3: [Diagnostic] Add ftabstop to 
-Wmisleading-indentation (authored by Tyker).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71037

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Parser/warn-misleading-indentation.cpp

Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- clang/test/Parser/warn-misleading-indentation.cpp
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -1,7 +1,9 @@
 // RUN: %clang_cc1 -x c -fsyntax-only -verify %s
-// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
-// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
 // RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN -ftabstop 8 -DTAB_SIZE=8 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN  -ftabstop 4 -DTAB_SIZE=4 -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -ftabstop 1 -DTAB_SIZE=1 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wmisleading-indentation -DCXX17 -DWITH_WARN -ftabstop 2 -DTAB_SIZE=2 %s
 
 #ifndef WITH_WARN
 // expected-no-diagnostics
@@ -225,3 +227,80 @@
 // expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
 #endif
 }
+int a4()
+{
+	if (0)
+		return 1;
+ 	return 0;
+#if (TAB_SIZE == 1)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif 
+}
+
+int a5()
+{
+	if (0)
+		return 1;
+		return 0;
+#if WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+int a6()
+{
+	if (0)
+		return 1;
+  		return 0;
+#if (TAB_SIZE == 8)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+#define FOO \
+ goto fail
+
+int main(int argc, char* argv[]) {
+  if (5 != 0)
+goto fail;
+  else
+goto fail;
+
+  if (1) {
+if (1)
+  goto fail;
+else if (1)
+  goto fail;
+else if (1)
+  goto fail;
+else
+  goto fail;
+  } else if (1) {
+if (1)
+  goto fail;
+  }
+
+  if (1) {
+if (1)
+  goto fail;
+  } else if (1)
+goto fail;
+
+
+  if (1) goto fail; goto fail;
+
+if (0)
+goto fail;
+
+goto fail;
+
+if (0)
+FOO;
+
+goto fail;
+
+fail:;
+}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1214,6 +1214,41 @@
 if (Kind == MSK_else && !ShouldSkip)
   P.MisleadingIndentationElseLoc = SL;
   }
+
+  /// Compute the column number will aligning tabs on TabStop (-ftabstop), this
+  /// gives the visual indentation of the SourceLocation.
+  static unsigned getVisualIndentation(SourceManager , SourceLocation Loc) {
+unsigned TabStop = SM.getDiagnostics().getDiagnosticOptions().TabStop;
+
+unsigned ColNo = SM.getSpellingColumnNumber(Loc);
+if (ColNo == 0 || TabStop == 1)
+  return ColNo;
+
+std::pair FIDAndOffset = SM.getDecomposedLoc(Loc);
+
+bool Invalid;
+StringRef BufData = SM.getBufferData(FIDAndOffset.first, );
+if (Invalid)
+  return 0;
+
+const char *EndPos = BufData.data() + FIDAndOffset.second;
+assert(FIDAndOffset.second > ColNo &&
+   "Column number smaller than file offset?");
+
+unsigned VisualColumn = 0; // Stored as 0-based column, here.
+// Loop from beginning of line up to Loc's file position, counting columns,
+// expanding tabs.
+for (const char *CurPos = EndPos - (ColNo - 1); CurPos != EndPos;
+ ++CurPos) {
+  if (*CurPos == '\t')
+// Advance visual column to next tabstop.
+VisualColumn += (TabStop - VisualColumn % TabStop);
+  else
+VisualColumn++;
+}
+return VisualColumn + 1;
+  }
+
   void Check() {
 Token Tok = P.getCurToken();
 if (P.getActions().getDiagnostics().isIgnored(
@@ -1230,9 +1265,9 @@
   P.MisleadingIndentationElseLoc = SourceLocation();
 
 SourceManager  = P.getPreprocessor().getSourceManager();
-unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
-unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
-unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+unsigned PrevColNum = getVisualIndentation(SM, PrevLoc);
+unsigned CurColNum = getVisualIndentation(SM, 

[clang] b47b35f - [Diagnostic] Add ftabstop to -Wmisleading-indentation

[via cfe-commits]

Author: Tyker
Date: 2019-12-30T09:24:34+01:00
New Revision: b47b35ff51b355a446483777155290541ab64fae

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

LOG: [Diagnostic] Add ftabstop to -Wmisleading-indentation

Summary:
this allow much better support of codebases like the linux kernel that mix tabs 
and spaces.

-ftabstop=//Width// allow specifying how large tabs are considered to be.

Reviewers: xbolva00, aaron.ballman, rsmith

Reviewed By: aaron.ballman

Subscribers: jyknight, riccibruno, rsmith, nathanchance

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

Added: 


Modified: 
clang/lib/Parse/ParseStmt.cpp
clang/test/Parser/warn-misleading-indentation.cpp

Removed: 




diff  --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 203f30610ce7..7fc2f2a2b320 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -1214,6 +1214,41 @@ struct MisleadingIndentationChecker {
 if (Kind == MSK_else && !ShouldSkip)
   P.MisleadingIndentationElseLoc = SL;
   }
+
+  /// Compute the column number will aligning tabs on TabStop (-ftabstop), this
+  /// gives the visual indentation of the SourceLocation.
+  static unsigned getVisualIndentation(SourceManager , SourceLocation Loc) {
+unsigned TabStop = SM.getDiagnostics().getDiagnosticOptions().TabStop;
+
+unsigned ColNo = SM.getSpellingColumnNumber(Loc);
+if (ColNo == 0 || TabStop == 1)
+  return ColNo;
+
+std::pair FIDAndOffset = SM.getDecomposedLoc(Loc);
+
+bool Invalid;
+StringRef BufData = SM.getBufferData(FIDAndOffset.first, );
+if (Invalid)
+  return 0;
+
+const char *EndPos = BufData.data() + FIDAndOffset.second;
+assert(FIDAndOffset.second > ColNo &&
+   "Column number smaller than file offset?");
+
+unsigned VisualColumn = 0; // Stored as 0-based column, here.
+// Loop from beginning of line up to Loc's file position, counting columns,
+// expanding tabs.
+for (const char *CurPos = EndPos - (ColNo - 1); CurPos != EndPos;
+ ++CurPos) {
+  if (*CurPos == '\t')
+// Advance visual column to next tabstop.
+VisualColumn += (TabStop - VisualColumn % TabStop);
+  else
+VisualColumn++;
+}
+return VisualColumn + 1;
+  }
+
   void Check() {
 Token Tok = P.getCurToken();
 if (P.getActions().getDiagnostics().isIgnored(
@@ -1230,9 +1265,9 @@ struct MisleadingIndentationChecker {
   P.MisleadingIndentationElseLoc = SourceLocation();
 
 SourceManager  = P.getPreprocessor().getSourceManager();
-unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
-unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
-unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+unsigned PrevColNum = getVisualIndentation(SM, PrevLoc);
+unsigned CurColNum = getVisualIndentation(SM, Tok.getLocation());
+unsigned StmtColNum = getVisualIndentation(SM, StmtLoc);
 
 if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
 ((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||

diff  --git a/clang/test/Parser/warn-misleading-indentation.cpp 
b/clang/test/Parser/warn-misleading-indentation.cpp
index d366db767e67..08d1ee262a3c 100644
--- a/clang/test/Parser/warn-misleading-indentation.cpp
+++ b/clang/test/Parser/warn-misleading-indentation.cpp
@@ -1,7 +1,9 @@
 // RUN: %clang_cc1 -x c -fsyntax-only -verify %s
-// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation 
-DWITH_WARN %s
-// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused 
-DWITH_WARN -DCXX17 %s
 // RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused 
-Wno-misleading-indentation -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation 
-DWITH_WARN -ftabstop 8 -DTAB_SIZE=8 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused 
-DWITH_WARN  -ftabstop 4 -DTAB_SIZE=4 -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN 
-ftabstop 1 -DTAB_SIZE=1 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused 
-Wmisleading-indentation -DCXX17 -DWITH_WARN -ftabstop 2 -DTAB_SIZE=2 %s
 
 #ifndef WITH_WARN
 // expected-no-diagnostics
@@ -225,3 +227,80 @@ void s(int num) {
 // expected-warning@-2 {{misleading indentation; statement is not part of the 
previous 'if'}}
 #endif
 }
+int a4()
+{
+   if (0)
+   return 1;
+   return 0;
+#if (TAB_SIZE == 1)
+// expected-warning@-2 {{misleading indentation; statement is not part of the 
previous 'if'}}
+// expected-note@-5 {{here}}
+#endif 
+}
+
+int a5()
+{
+   if (0)
+   return 1;
+   return 0;
+#if WITH_WARN
+// expected-warning@-2