[clang] 2ec59a0 - Buildbot debugging of 0d0b90105f92f6cd9cc7004d565834f4429183fb (lambda/function_ref lifetime issues)

[David Blaikie via cfe-commits]

Author: David Blaikie
Date: 2020-03-22T22:43:44-07:00
New Revision: 2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9

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

LOG: Buildbot debugging of 0d0b90105f92f6cd9cc7004d565834f4429183fb 
(lambda/function_ref lifetime issues)

This is failing on several buildbots with some inexplicable (to me,
right now) crashes. Let's see if this change is adequate to unblock the
buildbots & further understanding can be gained later.

Added: 


Modified: 
clang/include/clang/AST/OpenMPClause.h

Removed: 




diff  --git a/clang/include/clang/AST/OpenMPClause.h 
b/clang/include/clang/AST/OpenMPClause.h
index 548328d36a79..a781797c2d56 100644
--- a/clang/include/clang/AST/OpenMPClause.h
+++ b/clang/include/clang/AST/OpenMPClause.h
@@ -7061,9 +7061,9 @@ struct OMPTraitInfo {
 
   bool anyScoreOrCondition(
   llvm::function_ref Cond) {
-return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet ) {
+return llvm::any_of(Sets, [&](OMPTraitInfo::OMPTraitSet ) {
   return llvm::any_of(
-  Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector ) {
+  Set.Selectors, [&](OMPTraitInfo::OMPTraitSelector ) {
 return Cond(Selector.ScoreOrCondition,
 /* IsScore */ Selector.Kind !=
 llvm::omp::TraitSelector::user_condition);



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

Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[David Blaikie via cfe-commits]

Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what
happens with the buildbots.

On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert 
wrote:

>
> Apologies for the confusion.
>
> I wrote the commit message after looking into this and I though the
> issue was related to the capture by copy in the inner llvm::any_of and
> the reuse in the outer. Looking back at the code I cannot say anymore
> how I got that impression.
>
> If you think the reference is problematic, I'm totally happy to remove
> it. If the windows bots (or any other ones) don't like it we need to
> investigate why. As mentioned, I had a problem recreating the problem
> locally before.
>
> Cheers,
>Johannes
>
>
> On 3/22/20 1:37 PM, Arthur O'Dwyer wrote:
>  > On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <
>  > cfe-commits@lists.llvm.org> wrote:
>  >
>  >> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert
> 
>  >> wrote:
>  >>
>  >>> Some buildbots, I think only Windows buildbots for some reason,
> crashed
>  >>> in this function.
>  >>>
>  >>> The reason, as described, is that an `llvm::function_ref` cannot be
>  >>> copied and then reused. It just happened to work on almost all
>  >>> configurations.
>  >>>
>  >>
>  >> That doesn't seem to be accurate, or if it is there's a lot of
> mistakes in
>  >> the codebase - basically every function_ref parameter I can see in
> LLVM is
>  >> passing by value, not by const ref. The implementation of
>  >> llvm::function_ref looks quite copyable so long as it doesn't
> outlive the
>  >> functor it is pointing to.
>  >>
>  >
>  > David is correct. llvm::function_ref, like std::reference_wrapper, is a
>  > trivially copyable type, and it's designed to be copied.
>  > Like string_view and reference_wrapper, function_ref is designed to be
>  > passed by value. Redundantly passing function_ref *again by
> reference* is a
>  > code smell.
>  >
>  > I've also checked the code here, and it looks like there are only two
>  > callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and
> they
>  > are both fine. FWIW, I would recommend reverting Johannes' change and
>  > seeing if those Windows buildbots are still unhappy (and if so, why).
>  >
>  >
>  > Btw, one failure mode I'm aware of, but which I believe is NOT
> relevant in
>  > Johannes' case, is that `function_ref`'s converting constructor behaves
>  > differently from its copy constructor.
>  >
>  > int main()
>  >
>  > {
>  >
>  > auto lamb = [](){ return 42; };
>  >
>  > auto sheep = [](){ return 43; };
>  >
>  > llvm::function_ref one = lamb;
>  >
>  >
>  > llvm::function_ref twoA = one;// twoA refers to lamb
>  >
>  > llvm::function_ref twoB = one;  // twoB refers to one which
>  > refers to lamb
>  >
>  >
>  > one = sheep;
>  >
>  >
>  > assert(twoA() == 42);  // twoA refers to lamb
>  >
>  > assert(twoB() == 43);  // twoB refers to one which refers to sheep
>  >
>  > }
>  >
>  > That is, if you have a function that takes a parameter of type
>  > function_ref, and someone passes it an argument of type
> function_ref,
>  > then inside the function your parameter will be referring to that
> argument
>  > itself instead of to its referent.
>  > However, in Johannes' particular case, we have no function_refs
> referring
>  > to other function_refs. We just make a lambda and take a function_ref to
>  > it, the end. So I'm pretty sure this pitfall is irrelevant.
>  >
>  > –Arthur
>  >
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 0d0b901 - Revert "[FIX] Do not copy an llvm::function_ref if it has to be reused"

[David Blaikie via cfe-commits]

Author: David Blaikie
Date: 2020-03-22T18:43:39-07:00
New Revision: 0d0b90105f92f6cd9cc7004d565834f4429183fb

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

LOG: Revert "[FIX] Do not copy an llvm::function_ref if it has to be reused"

This fix doesn't seem to be right (function_ref can/should be passed by
value) so I'm reverted it to see if the buildbots decide to explain
what's wrong.

This reverts commit 857bf5da35af8e1f9425e1865dab5f5fce5e38f2.

Added: 


Modified: 
clang/include/clang/AST/OpenMPClause.h

Removed: 




diff  --git a/clang/include/clang/AST/OpenMPClause.h 
b/clang/include/clang/AST/OpenMPClause.h
index 38485cb1ad7e..548328d36a79 100644
--- a/clang/include/clang/AST/OpenMPClause.h
+++ b/clang/include/clang/AST/OpenMPClause.h
@@ -7060,10 +7060,10 @@ struct OMPTraitInfo {
   llvm::SmallVector Sets;
 
   bool anyScoreOrCondition(
-  const llvm::function_ref ) {
-return llvm::any_of(Sets, [](OMPTraitInfo::OMPTraitSet ) {
+  llvm::function_ref Cond) {
+return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet ) {
   return llvm::any_of(
-  Set.Selectors, [](OMPTraitInfo::OMPTraitSelector ) {
+  Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector ) {
 return Cond(Selector.ScoreOrCondition,
 /* IsScore */ Selector.Kind !=
 llvm::omp::TraitSelector::user_condition);



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

[PATCH] D70555: [coroutines] Don't build promise init with no args

[JunMa via Phabricator via cfe-commits]

junparser added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70555



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

[PATCH] D76582: [Hexagon] Don't clear libpath when target is linux-musl

[Sid Manning via Phabricator via cfe-commits]

sidneym created this revision.
sidneym added reviewers: kparzysz, bcain, adasgupt.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Use the default libpath when the target is linux-musl


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76582

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


Index: clang/lib/Driver/ToolChains/Hexagon.cpp
===
--- clang/lib/Driver/ToolChains/Hexagon.cpp
+++ clang/lib/Driver/ToolChains/Hexagon.cpp
@@ -499,11 +499,10 @@
 
   ToolChain::path_list  = getFilePaths();
 
-  // Remove paths added by Linux toolchain. Currently Hexagon_TC really targets
-  // 'elf' OS type, so the Linux paths are not appropriate. When we actually
-  // support 'linux' we'll need to fix this up
-  LibPaths.clear();
-  getHexagonLibraryPaths(Args, LibPaths);
+  if (!Triple.isMusl()) {
+LibPaths.clear();
+getHexagonLibraryPaths(Args, LibPaths);
+  }
 }
 
 HexagonToolChain::~HexagonToolChain() {}


Index: clang/lib/Driver/ToolChains/Hexagon.cpp
===
--- clang/lib/Driver/ToolChains/Hexagon.cpp
+++ clang/lib/Driver/ToolChains/Hexagon.cpp
@@ -499,11 +499,10 @@
 
   ToolChain::path_list  = getFilePaths();
 
-  // Remove paths added by Linux toolchain. Currently Hexagon_TC really targets
-  // 'elf' OS type, so the Linux paths are not appropriate. When we actually
-  // support 'linux' we'll need to fix this up
-  LibPaths.clear();
-  getHexagonLibraryPaths(Args, LibPaths);
+  if (!Triple.isMusl()) {
+LibPaths.clear();
+getHexagonLibraryPaths(Args, LibPaths);
+  }
 }
 
 HexagonToolChain::~HexagonToolChain() {}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D76054#1935533 , @ymandel wrote:

> Thanks for expanding the description, including the helpful example.  I'm not 
> sure, though, that this is the "right" behavior, at least not always. Worse, 
> I'm not sure there is a single "right" behavior. I can easily imagine a tidy 
> that matches multiple times in the same TU and tries, therefore, to apply the 
> same fix multiple times, even though it wants at most one (and possibly an 
> error).  The major problem is that character-level (versus AST level) edits 
> simply don't compose. So, multiple edits on a file are always suspect.  Could 
> you also give an example (in terms of code changes, not YAML) of why this 
> comes up? As in, are we sure that the problem lies in the algorithm here, 
> rather than in the phrasing of the transformation itself?
>
> That said, based on some of your linked bugs, it sounds like your change 
> would make the behavior here consistent with the behavior in another 
> situation (when clang-tidy applies the edits directly rather than outputting 
> to YAML?).  Consistency could be a strong argument in favor of this change.


clang-tidy does a good job of filtering out conflicting fix-its so duplicated 
insertions are going to be intentional.

The example yaml is generated from running is running the 
readability-braces-around-statements check on this code(offsets dont match due 
to formatting)

  void f() {
if (1)
  if (2) return;
  }

Both if statements have the same end location which is where the check will 
insert a `}`. 
This leads to 2 insertion fix its that are the same location and same text. 
When clang-tidy applies the fix-it there is no issue, but currently 
clang-apply-replacements thinks they should be deduplicated. 
This ultimately leads to malformed code as 2 `{` are inserted after the if 
conditions, but only one `}` is inserted at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[libunwind] 72fd103 - Doc: Links should use https

[Sylvestre Ledru via cfe-commits]

Author: Sylvestre Ledru
Date: 2020-03-22T22:49:33+01:00
New Revision: 72fd1033ea577a769cc855fde6b5576b82380715

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

LOG: Doc: Links should use https

Added: 


Modified: 
clang/docs/LibASTImporter.rst
clang/docs/analyzer/checkers.rst
libcxx/docs/TestingLibcxx.rst
libcxx/docs/UsingLibcxx.rst
libcxx/docs/index.rst
libunwind/docs/BuildingLibunwind.rst
libunwind/docs/index.rst
lld/docs/AtomLLD.rst
lld/docs/NewLLD.rst
lld/docs/design.rst
lld/docs/development.rst
lld/docs/getting_started.rst
lld/docs/index.rst
llvm/docs/AMDGPUUsage.rst
llvm/docs/AliasAnalysis.rst
llvm/docs/CMake.rst
llvm/docs/CommandGuide/llvm-lipo.rst
llvm/docs/CommandGuide/llvm-objcopy.rst
llvm/docs/CommandGuide/llvm-objdump.rst
llvm/docs/CommandGuide/llvm-size.rst
llvm/docs/CommandGuide/llvm-strings.rst
llvm/docs/CommandGuide/llvm-strip.rst
llvm/docs/CompileCudaWithLLVM.rst
llvm/docs/FAQ.rst
llvm/docs/GettingStarted.rst
llvm/docs/GettingStartedVS.rst
llvm/docs/GlobalISel/GMIR.rst
llvm/docs/GlobalISel/IRTranslator.rst
llvm/docs/GlobalISel/KnownBits.rst
llvm/docs/HistoricalNotes/2007-OriginalClangReadme.txt
llvm/docs/HowToCrossCompileLLVM.rst
llvm/docs/HowToSetUpLLVMStyleRTTI.rst
llvm/docs/LLVMBuild.txt
llvm/docs/LangRef.rst
llvm/docs/Lexicon.rst
llvm/docs/LibFuzzer.rst
llvm/docs/LoopTerminology.rst
llvm/docs/MarkdownQuickstartTemplate.md
llvm/docs/MergeFunctions.rst
llvm/docs/ProgrammersManual.rst
llvm/docs/Proposals/GitHubMove.rst
llvm/docs/README.txt
llvm/docs/Reference.rst
llvm/docs/ReleaseProcess.rst
llvm/docs/SphinxQuickstartTemplate.rst
llvm/docs/TableGen/index.rst
llvm/docs/TestSuiteGuide.md
llvm/docs/TestingGuide.rst
llvm/docs/TypeMetadata.rst
llvm/docs/UserGuides.rst
llvm/docs/Vectorizers.rst
llvm/docs/WritingAnLLVMPass.rst
llvm/docs/index.rst
llvm/docs/tutorial/BuildingAJIT1.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl02.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl03.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl04.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl05.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl08.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl09.rst
llvm/docs/tutorial/OCamlLangImpl3.rst
llvm/docs/tutorial/OCamlLangImpl5.rst
llvm/docs/tutorial/index.rst
openmp/README.rst
polly/docs/TipsAndTricks.rst

Removed: 




diff  --git a/clang/docs/LibASTImporter.rst b/clang/docs/LibASTImporter.rst
index 9c02b6ae76e9..bedaf527f5e9 100644
--- a/clang/docs/LibASTImporter.rst
+++ b/clang/docs/LibASTImporter.rst
@@ -119,7 +119,7 @@ Now we create the Importer and do the import:
   llvm::Expected ImportedOrErr = Importer.Import(From);
 
 The ``Import`` call returns with ``llvm::Expected``, so, we must check for any 
error.
-Please refer to the `error handling 
`_ 
documentation for details.
+Please refer to the `error handling 
`_ 
documentation for details.
 
 .. code-block:: cpp
 

diff  --git a/clang/docs/analyzer/checkers.rst 
b/clang/docs/analyzer/checkers.rst
index bac1181cced1..0bfb6456dc82 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2199,9 +2199,9 @@ lck_rw_try_lock_exclusive, lck_rw_try_lock_shared, 
pthread_mutex_unlock, pthread
 alpha.unix.SimpleStream (C)
 """
 Check for misuses of stream APIs. Check for misuses of stream APIs: ``fopen, 
fclose``
-(demo checker, the subject of the demo (`Slides 
`_ ,
+(demo checker, the subject of the demo (`Slides 
`_ ,
 `Video `_) by Anna Zaks and Jordan Rose 
presented at the
-`2012 LLVM Developers' Meeting `_).
+`2012 LLVM Developers' Meeting `_).
 
 .. code-block:: c
 

diff  --git a/libcxx/docs/TestingLibcxx.rst b/libcxx/docs/TestingLibcxx.rst
index eaba214390da..d295d13d26f3 100644
--- a/libcxx/docs/TestingLibcxx.rst
+++ b/libcxx/docs/TestingLibcxx.rst
@@ -19,7 +19,7 @@ test libc++.
 
 Please see the `Lit Command Guide`_ for more information about LIT.
 
-.. _LIT Command Guide: http://llvm.org/docs/CommandGuide/lit.html
+.. _LIT Command Guide: https://llvm.org/docs/CommandGuide/lit.html
 
 Setting up the Environment
 --

diff  --git a/libcxx/docs/UsingLibcxx.rst 

[clang] 72fd103 - Doc: Links should use https

[Sylvestre Ledru via cfe-commits]

Author: Sylvestre Ledru
Date: 2020-03-22T22:49:33+01:00
New Revision: 72fd1033ea577a769cc855fde6b5576b82380715

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

LOG: Doc: Links should use https

Added: 


Modified: 
clang/docs/LibASTImporter.rst
clang/docs/analyzer/checkers.rst
libcxx/docs/TestingLibcxx.rst
libcxx/docs/UsingLibcxx.rst
libcxx/docs/index.rst
libunwind/docs/BuildingLibunwind.rst
libunwind/docs/index.rst
lld/docs/AtomLLD.rst
lld/docs/NewLLD.rst
lld/docs/design.rst
lld/docs/development.rst
lld/docs/getting_started.rst
lld/docs/index.rst
llvm/docs/AMDGPUUsage.rst
llvm/docs/AliasAnalysis.rst
llvm/docs/CMake.rst
llvm/docs/CommandGuide/llvm-lipo.rst
llvm/docs/CommandGuide/llvm-objcopy.rst
llvm/docs/CommandGuide/llvm-objdump.rst
llvm/docs/CommandGuide/llvm-size.rst
llvm/docs/CommandGuide/llvm-strings.rst
llvm/docs/CommandGuide/llvm-strip.rst
llvm/docs/CompileCudaWithLLVM.rst
llvm/docs/FAQ.rst
llvm/docs/GettingStarted.rst
llvm/docs/GettingStartedVS.rst
llvm/docs/GlobalISel/GMIR.rst
llvm/docs/GlobalISel/IRTranslator.rst
llvm/docs/GlobalISel/KnownBits.rst
llvm/docs/HistoricalNotes/2007-OriginalClangReadme.txt
llvm/docs/HowToCrossCompileLLVM.rst
llvm/docs/HowToSetUpLLVMStyleRTTI.rst
llvm/docs/LLVMBuild.txt
llvm/docs/LangRef.rst
llvm/docs/Lexicon.rst
llvm/docs/LibFuzzer.rst
llvm/docs/LoopTerminology.rst
llvm/docs/MarkdownQuickstartTemplate.md
llvm/docs/MergeFunctions.rst
llvm/docs/ProgrammersManual.rst
llvm/docs/Proposals/GitHubMove.rst
llvm/docs/README.txt
llvm/docs/Reference.rst
llvm/docs/ReleaseProcess.rst
llvm/docs/SphinxQuickstartTemplate.rst
llvm/docs/TableGen/index.rst
llvm/docs/TestSuiteGuide.md
llvm/docs/TestingGuide.rst
llvm/docs/TypeMetadata.rst
llvm/docs/UserGuides.rst
llvm/docs/Vectorizers.rst
llvm/docs/WritingAnLLVMPass.rst
llvm/docs/index.rst
llvm/docs/tutorial/BuildingAJIT1.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl02.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl03.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl04.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl05.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl08.rst
llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl09.rst
llvm/docs/tutorial/OCamlLangImpl3.rst
llvm/docs/tutorial/OCamlLangImpl5.rst
llvm/docs/tutorial/index.rst
openmp/README.rst
polly/docs/TipsAndTricks.rst

Removed: 




diff  --git a/clang/docs/LibASTImporter.rst b/clang/docs/LibASTImporter.rst
index 9c02b6ae76e9..bedaf527f5e9 100644
--- a/clang/docs/LibASTImporter.rst
+++ b/clang/docs/LibASTImporter.rst
@@ -119,7 +119,7 @@ Now we create the Importer and do the import:
   llvm::Expected ImportedOrErr = Importer.Import(From);
 
 The ``Import`` call returns with ``llvm::Expected``, so, we must check for any 
error.
-Please refer to the `error handling 
`_ 
documentation for details.
+Please refer to the `error handling 
`_ 
documentation for details.
 
 .. code-block:: cpp
 

diff  --git a/clang/docs/analyzer/checkers.rst 
b/clang/docs/analyzer/checkers.rst
index bac1181cced1..0bfb6456dc82 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2199,9 +2199,9 @@ lck_rw_try_lock_exclusive, lck_rw_try_lock_shared, 
pthread_mutex_unlock, pthread
 alpha.unix.SimpleStream (C)
 """
 Check for misuses of stream APIs. Check for misuses of stream APIs: ``fopen, 
fclose``
-(demo checker, the subject of the demo (`Slides 
`_ ,
+(demo checker, the subject of the demo (`Slides 
`_ ,
 `Video `_) by Anna Zaks and Jordan Rose 
presented at the
-`2012 LLVM Developers' Meeting `_).
+`2012 LLVM Developers' Meeting `_).
 
 .. code-block:: c
 

diff  --git a/libcxx/docs/TestingLibcxx.rst b/libcxx/docs/TestingLibcxx.rst
index eaba214390da..d295d13d26f3 100644
--- a/libcxx/docs/TestingLibcxx.rst
+++ b/libcxx/docs/TestingLibcxx.rst
@@ -19,7 +19,7 @@ test libc++.
 
 Please see the `Lit Command Guide`_ for more information about LIT.
 
-.. _LIT Command Guide: http://llvm.org/docs/CommandGuide/lit.html
+.. _LIT Command Guide: https://llvm.org/docs/CommandGuide/lit.html
 
 Setting up the Environment
 --

diff  --git a/libcxx/docs/UsingLibcxx.rst 

[clang-tools-extra] 9860517 - doc: use the right url to bugzilla

[Sylvestre Ledru via cfe-commits]

Author: Sylvestre Ledru
Date: 2020-03-22T22:49:40+01:00
New Revision: 986051749cb12992461626e8e37d7c0923816763

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

LOG: doc: use the right url to bugzilla

Added: 


Modified: 
clang-tools-extra/docs/clang-rename.rst
lld/docs/getting_started.rst
llvm/docs/CommandGuide/llvm-lipo.rst
llvm/docs/CommandGuide/llvm-objcopy.rst
llvm/docs/CommandGuide/llvm-objdump.rst
llvm/docs/CommandGuide/llvm-size.rst
llvm/docs/CommandGuide/llvm-strings.rst
llvm/docs/CommandGuide/llvm-strip.rst
llvm/docs/CompileCudaWithLLVM.rst
llvm/docs/LibFuzzer.rst

Removed: 




diff  --git a/clang-tools-extra/docs/clang-rename.rst 
b/clang-tools-extra/docs/clang-rename.rst
index ef6ed9cc0814..b45ba01c06a2 100644
--- a/clang-tools-extra/docs/clang-rename.rst
+++ b/clang-tools-extra/docs/clang-rename.rst
@@ -16,7 +16,7 @@ functions, variables, arguments, namespaces etc.
 
 The tool is in a very early development stage, so you might encounter bugs and
 crashes. Submitting reports with information about how to reproduce the issue
-to `the LLVM bugtracker `_ will definitely help the
+to `the LLVM bugtracker `_ will definitely help the
 project. If you have any ideas or suggestions, you might want to put a feature
 request there.
 

diff  --git a/lld/docs/getting_started.rst b/lld/docs/getting_started.rst
index 5cf7acdb6a06..506cb24dde84 100644
--- a/lld/docs/getting_started.rst
+++ b/lld/docs/getting_started.rst
@@ -6,7 +6,7 @@ Getting Started: Building and Running lld
 This page gives you the shortest path to checking out and building lld. If you
 run into problems, please file bugs in the `LLVM Bugzilla`__
 
-__ https://llvm.org/bugs/
+__ https://bugs.llvm.org/
 
 Building lld
 

diff  --git a/llvm/docs/CommandGuide/llvm-lipo.rst 
b/llvm/docs/CommandGuide/llvm-lipo.rst
index 666b48116e1a..20b2984fc9b2 100644
--- a/llvm/docs/CommandGuide/llvm-lipo.rst
+++ b/llvm/docs/CommandGuide/llvm-lipo.rst
@@ -70,4 +70,4 @@ COMMANDS
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .

diff  --git a/llvm/docs/CommandGuide/llvm-objcopy.rst 
b/llvm/docs/CommandGuide/llvm-objcopy.rst
index 468d9ae73fb0..14e08d7641d5 100644
--- a/llvm/docs/CommandGuide/llvm-objcopy.rst
+++ b/llvm/docs/CommandGuide/llvm-objcopy.rst
@@ -545,7 +545,7 @@ Otherwise, it exits with code 0.
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .
 
 There is a known issue with :option:`--input-target` and :option:`--target`
 causing only ``binary`` and ``ihex`` formats to have any effect. Other values

diff  --git a/llvm/docs/CommandGuide/llvm-objdump.rst 
b/llvm/docs/CommandGuide/llvm-objdump.rst
index d3ae1c2b7090..04c76491101d 100644
--- a/llvm/docs/CommandGuide/llvm-objdump.rst
+++ b/llvm/docs/CommandGuide/llvm-objdump.rst
@@ -324,7 +324,7 @@ MACH-O ONLY OPTIONS AND COMMANDS
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .
 
 SEE ALSO
 

diff  --git a/llvm/docs/CommandGuide/llvm-size.rst 
b/llvm/docs/CommandGuide/llvm-size.rst
index 856d2f9ac972..b229bc63d40a 100644
--- a/llvm/docs/CommandGuide/llvm-size.rst
+++ b/llvm/docs/CommandGuide/llvm-size.rst
@@ -195,4 +195,4 @@ Otherwise, it exits with code 0.
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .

diff  --git a/llvm/docs/CommandGuide/llvm-strings.rst 
b/llvm/docs/CommandGuide/llvm-strings.rst
index a393dabd8476..e2fe4cb88c9a 100644
--- a/llvm/docs/CommandGuide/llvm-strings.rst
+++ b/llvm/docs/CommandGuide/llvm-strings.rst
@@ -127,4 +127,4 @@ Otherwise, it exits with code 0.
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .

diff  --git a/llvm/docs/CommandGuide/llvm-strip.rst 
b/llvm/docs/CommandGuide/llvm-strip.rst
index b962a1f96e8a..455dc07e9c5c 100644
--- a/llvm/docs/CommandGuide/llvm-strip.rst
+++ b/llvm/docs/CommandGuide/llvm-strip.rst
@@ -190,7 +190,7 @@ Otherwise, it exits with code 0.
 BUGS
 
 
-To report bugs, please visit .
+To report bugs, please visit .
 
 SEE ALSO
 

diff  --git a/llvm/docs/CompileCudaWithLLVM.rst 
b/llvm/docs/CompileCudaWithLLVM.rst
index c471c48c2d97..a2d7fd0b7453 100644
--- a/llvm/docs/CompileCudaWithLLVM.rst
+++ b/llvm/docs/CompileCudaWithLLVM.rst
@@ -30,7 +30,7 @@ Before you build CUDA code, you'll need to have installed the 
CUDA SDK.  See
 `NVIDIA's CUDA installation guide
 

[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

[Ties Stuij via Phabricator via cfe-commits]

stuij added inline comments.



Comment at: clang/test/Driver/aarch64-cpus.c:622
+
+// The BFloat16 extension is a mandatory component of the Armv8.6-A 
extensions, but is permitted as an
+// optional feature for any implementation of Armv8.2-A to Armv8.5-A 
(inclusive)

SjoerdMeijer wrote:
> Do we need 2 additional tests here?
> - one for v8.6,
> - and another with SVE?
> 
> 
I think we're ok here.
- for v8.6 the driver doesn't pass bfloat as an argument
- SVE: from a cmdline perspective, there's no special interaction between 
bfloat and SVE. either can be active without the other.



Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7867
+V128, asm, ".4s",
+  []> {
+  let AsmString = !strconcat(asm, "{\t$Rd", ".4s", ", $Rn", ".8h",

SjoerdMeijer wrote:
> and perhaps this one. But looks intentional, perhaps it's fine then, I don't 
> know.
Yes, the square brackets will be filled in in a next patch. I'll just leave 
them as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76062



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

[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

[Ties Stuij via Phabricator via cfe-commits]

stuij updated this revision to Diff 251916.
stuij marked 9 inline comments as done.
stuij added a comment.

reindenting a few lines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76062

Files:
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Basic/Targets/ARM.cpp
  clang/test/Driver/aarch64-cpus.c
  clang/test/Driver/arm-cortex-cpus.c
  clang/test/Preprocessor/arm-target-features.c
  llvm/include/llvm/ADT/Triple.h
  llvm/include/llvm/Support/AArch64TargetParser.def
  llvm/include/llvm/Support/AArch64TargetParser.h
  llvm/include/llvm/Support/ARMTargetParser.def
  llvm/include/llvm/Support/ARMTargetParser.h
  llvm/lib/Support/AArch64TargetParser.cpp
  llvm/lib/Support/ARMTargetParser.cpp
  llvm/lib/Support/Triple.cpp
  llvm/lib/Target/AArch64/AArch64.td
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/lib/Target/AArch64/AArch64InstrInfo.td
  llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
  llvm/lib/Target/AArch64/AArch64Subtarget.h
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/AArch64/SVEInstrFormats.td
  llvm/lib/Target/ARM/ARM.td
  llvm/lib/Target/ARM/ARMInstrNEON.td
  llvm/lib/Target/ARM/ARMInstrVFP.td
  llvm/lib/Target/ARM/ARMPredicates.td
  llvm/lib/Target/ARM/ARMSubtarget.h
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/test/MC/AArch64/SVE/bfcvt-diagnostics.s
  llvm/test/MC/AArch64/SVE/bfcvt.s
  llvm/test/MC/AArch64/SVE/bfcvtnt-diagnostics.s
  llvm/test/MC/AArch64/SVE/bfcvtnt.s
  llvm/test/MC/AArch64/SVE/bfdot-diagnostics.s
  llvm/test/MC/AArch64/SVE/bfdot.s
  llvm/test/MC/AArch64/SVE/bfmlal-diagnostics.s
  llvm/test/MC/AArch64/SVE/bfmlal.s
  llvm/test/MC/AArch64/SVE/bfmmla-diagnostics.s
  llvm/test/MC/AArch64/SVE/bfmmla.s
  llvm/test/MC/AArch64/armv8.6a-bf16.s
  llvm/test/MC/ARM/bfloat16-a32-errors.s
  llvm/test/MC/ARM/bfloat16-a32-errors2.s
  llvm/test/MC/ARM/bfloat16-a32.s
  llvm/test/MC/ARM/bfloat16-t32-errors.s
  llvm/test/MC/ARM/bfloat16-t32.s
  llvm/test/MC/Disassembler/AArch64/armv8.6a-bf16.txt
  llvm/test/MC/Disassembler/ARM/bfloat16-a32_1.txt
  llvm/test/MC/Disassembler/ARM/bfloat16-a32_2.txt
  llvm/test/MC/Disassembler/ARM/bfloat16-t32.txt
  llvm/test/MC/Disassembler/ARM/bfloat16-t32_errors.txt
  llvm/unittests/Support/TargetParserTest.cpp

Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -26,9 +26,9 @@
 "armv7e-m","armv7em",  "armv8-a", "armv8","armv8a",
 "armv8l",  "armv8.1-a","armv8.1a","armv8.2-a","armv8.2a",
 "armv8.3-a",   "armv8.3a", "armv8.4-a",   "armv8.4a", "armv8.5-a",
-"armv8.5a", "armv8-r", "armv8r",  "armv8-m.base", "armv8m.base",
-"armv8-m.main", "armv8m.main", "iwmmxt",  "iwmmxt2",  "xscale",
-"armv8.1-m.main",
+"armv8.5a", "armv8.6-a",   "armv8.6a", "armv8-r", "armv8r",
+"armv8-m.base", "armv8m.base", "armv8-m.main", "armv8m.main", "iwmmxt",
+"iwmmxt2",  "xscale",  "armv8.1-m.main",
 };
 
 bool testARMCPU(StringRef CPUName, StringRef ExpectedArch,
@@ -411,6 +411,9 @@
   testARMArch("armv8.5-a", "generic", "v8.5a",
   ARMBuildAttrs::CPUArch::v8_A));
   EXPECT_TRUE(
+  testARMArch("armv8.6-a", "generic", "v8.6a",
+  ARMBuildAttrs::CPUArch::v8_A));
+  EXPECT_TRUE(
   testARMArch("armv8-r", "cortex-r52", "v8r",
   ARMBuildAttrs::CPUArch::v8_R));
   EXPECT_TRUE(
@@ -678,7 +681,7 @@
   "v7",   "v7a","v7ve",  "v7hl",   "v7l",   "v7-r",   "v7r",   "v7-m",
   "v7m",  "v7k","v7s",   "v7e-m",  "v7em",  "v8-a",   "v8","v8a",
   "v8l",  "v8.1-a", "v8.1a", "v8.2-a", "v8.2a", "v8.3-a", "v8.3a", "v8.4-a",
-  "v8.4a", "v8.5-a","v8.5a", "v8-r",   "v8m.base", "v8m.main", "v8.1m.main"
+  "v8.4a", "v8.5-a","v8.5a", "v8.6-a", "v8.6a", "v8-r",   "v8m.base", "v8m.main", "v8.1m.main"
   };
 
   for (unsigned i = 0; i < array_lengthof(Arch); i++) {
@@ -743,6 +746,7 @@
 case ARM::ArchKind::ARMV8_3A:
 case ARM::ArchKind::ARMV8_4A:
 case ARM::ArchKind::ARMV8_5A:
+case ARM::ArchKind::ARMV8_6A:
   EXPECT_EQ(ARM::ProfileKind::A, ARM::parseArchProfile(ARMArch[i]));
   break;
 default:
@@ -1008,6 +1012,8 @@
   ARMBuildAttrs::CPUArch::v8_A));
   EXPECT_TRUE(testAArch64Arch("armv8.5-a", "generic", "v8.5a",
   ARMBuildAttrs::CPUArch::v8_A));
+  EXPECT_TRUE(testAArch64Arch("armv8.6-a", "generic", "v8.6a",
+  ARMBuildAttrs::CPUArch::v8_A));
 }
 
 bool testAArch64Extension(StringRef CPUName, AArch64::ArchKind AK,
Index: llvm/test/MC/Disassembler/ARM/bfloat16-t32_errors.txt

[clang] 7cfd5de - clang/release notes: s/Subversion/git/

[Sylvestre Ledru via cfe-commits]

Author: Sylvestre Ledru
Date: 2020-03-22T22:18:40+01:00
New Revision: 7cfd5de61b8191bbd5a405a05163b71802e8fb8d

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

LOG: clang/release notes: s/Subversion/git/

Added: 


Modified: 
clang/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 710f005985da..9f20c271b50e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -304,7 +304,7 @@ Additional Information
 
 A wide variety of additional information is available on the `Clang web
 page `_. The web page contains versions of the
-API documentation which are up-to-date with the Subversion version of
+API documentation which are up-to-date with the Git version of
 the source code. You can access versions of these documents specific to
 this release by going into the "``clang/docs/``" directory in the Clang
 tree.



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

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Passing-by remark:

> I wrote a Clang warning [not pictured] to diagnose any use of `T(x)` which 
> was not equivalent to `static_cast(x)`.

I'm not sure whether or not this will pass the bar for a clang diagnostic,
but that does sound like a good clang-tidy readability check.

I also have a bit of pet peeve with `T(x)` (i.e. including those that should be 
`static_cast<>`),
so if you do go with clang-tidy direction, it may be good to generalize it to 
handle all `T(x)`,
but diagnose only those cast types that are specified in the config.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76572



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

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone created this revision.
Quuxplusone added reviewers: rsmith, espindola, DiggerLin, ahatanak, reames.
Herald added subscribers: cfe-commits, rupprecht, dexonsmith, MaskRay, 
hiraditya.
Herald added a reviewer: jhenderson.
Herald added a project: clang.

I wrote a Clang warning [not pictured] to diagnose any use of `T(x)` which was 
not equivalent to `static_cast(x)`. Then I built Clang with that warning 
turned on. Here are all of the instances of "unsafe" `T(x)` that it identified 
in the codebase. Every single one is a cast from some pointer type to 
`uintptr_t`.

N.B.: The code in `llvm/IR/SymbolTableListTraits.h` looks absolutely crazy, and 
this will be the first time it's been touched in over a decade.

If this refactor is acceptable, I'll need someone to land it for me, as I don't 
have permissions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76572

Files:
  clang/lib/CodeGen/CGCall.h
  llvm/include/llvm/IR/SymbolTableListTraits.h
  llvm/include/llvm/Object/Binary.h
  llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp
  llvm/lib/Object/COFFObjectFile.cpp
  llvm/lib/Object/ELFObjectFile.cpp
  llvm/lib/Object/XCOFFObjectFile.cpp
  llvm/lib/Target/Hexagon/HexagonCommonGEP.cpp
  llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
  llvm/tools/llvm-readobj/ELFDumper.cpp

Index: llvm/tools/llvm-readobj/ELFDumper.cpp
===
--- llvm/tools/llvm-readobj/ELFDumper.cpp
+++ llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -443,7 +443,7 @@
   const ELFFile *Obj = ObjF->getELFFile();
   unsigned SecNdx = Sec - (Obj->sections()).front();
 
-  if (uintptr_t(Obj->base() + Sec->sh_offset) % sizeof(uint16_t) != 0)
+  if (reinterpret_cast(Obj->base() + Sec->sh_offset) % sizeof(uint16_t) != 0)
 return createError("the SHT_GNU_versym section with index " +
Twine(SecNdx) + " is misaligned");
 
@@ -522,7 +522,7 @@
  Twine(SecNdx) + ": version definition " + Twine(I) +
  " goes past the end of the section");
 
-if (uintptr_t(VerdefBuf) % sizeof(uint32_t) != 0)
+if (reinterpret_cast(VerdefBuf) % sizeof(uint32_t) != 0)
   return createError(
   "invalid SHT_GNU_verdef section with index " + Twine(SecNdx) +
   ": found a misaligned version definition entry at offset 0x" +
@@ -545,7 +545,7 @@
 
 const uint8_t *VerdauxBuf = VerdefBuf + D->vd_aux;
 for (unsigned J = 0; J < D->vd_cnt; ++J) {
-  if (uintptr_t(VerdauxBuf) % sizeof(uint32_t) != 0)
+  if (reinterpret_cast(VerdauxBuf) % sizeof(uint32_t) != 0)
 return createError("invalid SHT_GNU_verdef section with index " +
Twine(SecNdx) +
": found a misaligned auxiliary entry at offset 0x" +
@@ -597,7 +597,7 @@
  Twine(SecNdx) + ": version dependency " + Twine(I) +
  " goes past the end of the section");
 
-if (uintptr_t(VerneedBuf) % sizeof(uint32_t) != 0)
+if (reinterpret_cast(VerneedBuf) % sizeof(uint32_t) != 0)
   return createError(
   "invalid SHT_GNU_verneed section with index " + Twine(SecNdx) +
   ": found a misaligned version dependency entry at offset 0x" +
@@ -624,7 +624,7 @@
 
 const uint8_t *VernauxBuf = VerneedBuf + Verneed->vn_aux;
 for (unsigned J = 0; J < Verneed->vn_cnt; ++J) {
-  if (uintptr_t(VernauxBuf) % sizeof(uint32_t) != 0)
+  if (reinterpret_cast(VernauxBuf) % sizeof(uint32_t) != 0)
 return createError("invalid SHT_GNU_verneed section with index " +
Twine(SecNdx) +
": found a misaligned auxiliary entry at offset 0x" +
Index: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
@@ -619,14 +619,14 @@
 ArgTLS = nullptr;
 GetArgTLSTy = FunctionType::get(PointerType::getUnqual(ArgTLSTy), false);
 GetArgTLS = ConstantExpr::getIntToPtr(
-ConstantInt::get(IntptrTy, uintptr_t(GetArgTLSPtr)),
+ConstantInt::get(IntptrTy, reinterpret_cast(GetArgTLSPtr)),
 PointerType::getUnqual(GetArgTLSTy));
   }
   if (GetRetvalTLSPtr) {
 RetvalTLS = nullptr;
 GetRetvalTLSTy = FunctionType::get(PointerType::getUnqual(ShadowTy), false);
 GetRetvalTLS = ConstantExpr::getIntToPtr(
-ConstantInt::get(IntptrTy, uintptr_t(GetRetvalTLSPtr)),
+ConstantInt::get(IntptrTy, reinterpret_cast(GetRetvalTLSPtr)),
 PointerType::getUnqual(GetRetvalTLSTy));
   }
 
Index: llvm/lib/Target/Hexagon/HexagonCommonGEP.cpp
===
--- llvm/lib/Target/Hexagon/HexagonCommonGEP.cpp
+++ llvm/lib/Target/Hexagon/HexagonCommonGEP.cpp
@@ -482,7 +482,8 @@
   // determining 

Re: [clang] 4a0267e - Convert a reachable llvm_unreachable into an assert.

[David Blaikie via cfe-commits]

On Sun, Mar 22, 2020 at 9:34 AM Aaron Ballman 
wrote:

> On Sun, Mar 22, 2020 at 12:19 PM David Blaikie  wrote:
> >
> > On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman 
> wrote:
> >>
> >> On Sat, Mar 21, 2020 at 11:31 PM David Blaikie 
> wrote:
> >> >
> >> > Why the change? this seems counter to LLVM's style which pretty
> consistently uses unreachable rather than assert(false), so far as I know?
> >>
> >> We're not super consistent (at least within Clang), but the rules as
> >> I've generally understood them are to use llvm_unreachable only for
> >> truly unreachable code and to use assert(false) when the code is
> >> technically reachable but is a programmer mistake to have gotten
> >> there.
> >
> >
> > I don't see those as two different things personally - llvm_unreachable
> is used when the programmer believes it to be unreachable (not that it must
> be proven to be unreachable - we have message text there so it's
> informative if the assumption turns out not to hold)
>
> The message text doesn't help when the code is reached in release
> mode, which was the issue. Asserts + release you still get some
> helpful text saying "you screwed up". llvm_unreachable in release
> mode, you may get lucky or you may not (in this case, I didn't get
> lucky -- there was no text, just a crash).
>

That doesn't seem to be what it's documented to do:

/// Marks that the current location is not supposed to be reachable.
/// In !NDEBUG builds, prints the message and location info to stderr.
/// In NDEBUG builds, becomes an optimizer hint that the current location
/// is not supposed to be reachable.  On compilers that don't support
/// such hints, prints a reduced message instead.

& certainly I think the documentation is what it /should/ be doing.

/maybe/ https://reviews.llvm.org/rG5f4535b974e973d52797945fbf80f19ffba8c4ad
broke
that contract on Windows, but I'm not sure how? (an unreachable at the end
of that function shouldn't cause the whole function to be unreachable -
because abort could have side effects and halt the program before the
unreachable is reached)


> >> In this particular case, the code is very much reachable
> >
> >
> > In what sense? If it is actually reachable - shouldn't it be tested? (&
> in which case the assert(false) will get in the way of that testing)
>
> In the sense that normal code paths reach that code easily. Basically,
> that code is checking to see whether a plugin you've written properly
> sets up its options or not. When you're developing a plugin, it's
> quite reasonable to expect you won't get it just right on the first
> try, so you hit the code path but only as a result of you not writing
> the plugin quite right. So under normal conditions (once the plugin is
> working), the code path should not be reached but under development
> the code path gets reached accidentally.
>
> >> and I
> >> spent a lot more time debugging than I should have because I was using
> >> a release + asserts build and the semantics of llvm_unreachable made
> >> unfortunate codegen (switching to an assert makes the issue
> >> immediately obvious).
> >
> >
> > I think it might be reasonable to say that release+asserts to have
> unreachable behave the same way unreachable behaves at -O0 (which is to
> say, much like assert(false)). Clearly release+asserts effects code
> generation, so there's nothing like the "debug info invariance" goal that
> this would be tainting, etc.
> >
> > Certainly opinions vary here, but here are some commits that show the
> sort of general preference:
> > http://llvm.org/viewvc/llvm-project?view=revision=259302
> > http://llvm.org/viewvc/llvm-project?view=revision=253005
> > http://llvm.org/viewvc/llvm-project?view=revision=251266
> >
> > And some counter examples:
> > http://llvm.org/viewvc/llvm-project?view=revision=225043
> > Including this thread where Chandler originally (not sure what his take
> on it is these days) expressed some ideas more along your lines:
> >
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583
> >
> > But I'm always pretty concerned about the idea that assertions should be
> used in places where the behavior of the program has any constraints when
> the assertion is false...
>
> I'm opposed to using unreachability hints on control flow paths you
> expect to reach -- the semantics are just plain wrong, even if you can
> get the same end result of controlled crash + message. In this
> particular case, the code is reachable but erroneous to do so -- and
> that's what assertions are intended to be used for. My preference is
> to use llvm_unreachable because the code is unreachable, not because
> the code should not be reachable only if everything works out right.
>
> It may be that we're not in agreement about the definition of "expects
> to reach" here. To me, this code clearly expects to reach that path:
> it's a search over a finite collection where it's possible the thing
> being searched for is not found. The "we didn't 

[PATCH] D76549: [clang-tidy] Fix RenamerClangTidy handling qualified TypeLocs

[Nathan James via Phabricator via cfe-commits]

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

- Address `auto` and add test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76549

Files:
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -532,3 +532,16 @@
 
 using namespace FOO_NS::InlineNamespace;
 // CHECK-FIXES: {{^}}using namespace foo_ns::inline_namespace;
+
+void QualifiedTypeLocTest(THIS___Structure);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(this_structure);{{$}}
+void QualifiedTypeLocTest(THIS___Structure &);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(this_structure &);{{$}}
+void QualifiedTypeLocTest(THIS___Structure &&);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(this_structure &&);{{$}}
+void QualifiedTypeLocTest(const THIS___Structure);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(const this_structure);{{$}}
+void QualifiedTypeLocTest(const THIS___Structure &);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(const this_structure &);{{$}}
+void QualifiedTypeLocTest(volatile THIS___Structure &);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(volatile this_structure &);{{$}}
Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
@@ -57,7 +57,7 @@
 inline reference_wrapper
 cref(const Up ) noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: declaration uses identifier 
'u', which is not a reserved identifier [bugprone-reserved-identifier]
-  // CHECK-FIXES: {{^}}cref(const Up &__u) noexcept {{{$}}
+  // CHECK-FIXES: {{^}}cref(const _Up &__u) noexcept {{{$}}
   return reference_wrapper(u);
 }
 
Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
===
--- clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
@@ -203,14 +203,15 @@
   }
 
   if (const auto *Loc = Result.Nodes.getNodeAs("typeLoc")) {
+UnqualTypeLoc Unqual = Loc->getUnqualifiedLoc();
 NamedDecl *Decl = nullptr;
-if (const auto  = Loc->getAs())
+if (const auto  = Unqual.getAs())
   Decl = Ref.getDecl();
-else if (const auto  = Loc->getAs())
+else if (const auto  = Unqual.getAs())
   Decl = Ref.getDecl();
-else if (const auto  = Loc->getAs())
+else if (const auto  = Unqual.getAs())
   Decl = Ref.getDecl();
-else if (const auto  = Loc->getAs())
+else if (const auto  = Unqual.getAs())
   Decl = Ref.getDecl();
 // further TypeLocs handled below
 


Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -532,3 +532,16 @@
 
 using namespace FOO_NS::InlineNamespace;
 // CHECK-FIXES: {{^}}using namespace foo_ns::inline_namespace;
+
+void QualifiedTypeLocTest(THIS___Structure);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(this_structure);{{$}}
+void QualifiedTypeLocTest(THIS___Structure &);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(this_structure &);{{$}}
+void QualifiedTypeLocTest(THIS___Structure &&);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(this_structure &&);{{$}}
+void QualifiedTypeLocTest(const THIS___Structure);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(const this_structure);{{$}}
+void QualifiedTypeLocTest(const THIS___Structure &);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(const this_structure &);{{$}}
+void QualifiedTypeLocTest(volatile THIS___Structure &);
+// CHECK-FIXES: {{^}}void QualifiedTypeLocTest(volatile this_structure &);{{$}}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
@@ -57,7 +57,7 @@
 inline reference_wrapper
 cref(const Up ) noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 

[PATCH] D76455: [NFC] Refactor handling of Xarch option

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG78957bab5515: [NFC] Refactor handling of Xarch option 
(authored by yaxunl).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76455

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/HIP.cpp

Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -382,31 +382,7 @@
   // Skip this argument unless the architecture matches BoundArch.
   if (BoundArch.empty() || A->getValue(0) != BoundArch)
 continue;
-
-  unsigned Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
-  unsigned Prev = Index;
-  std::unique_ptr XarchArg(Opts.ParseOneArg(Args, Index));
-
-  // If the argument parsing failed or more than one argument was
-  // consumed, the -Xarch_ argument's parameter tried to consume
-  // extra arguments. Emit an error and ignore.
-  //
-  // We also want to disallow any options which would alter the
-  // driver behavior; that isn't going to work in our model. We
-  // use isDriverOption() as an approximation, although things
-  // like -O4 are going to slip through.
-  if (!XarchArg || Index > Prev + 1) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_with_args)
-<< A->getAsString(Args);
-continue;
-  } else if (XarchArg->getOption().hasFlag(options::DriverOption)) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_isdriver)
-<< A->getAsString(Args);
-continue;
-  }
-  XarchArg->setBaseArg(A);
-  A = XarchArg.release();
-  DAL->AddSynthesizedArg(A);
+  TranslateXarchArgs(Args, A, DAL);
 }
 DAL->append(A);
   }
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2132,32 +2132,7 @@
 continue;
 
   Arg *OriginalArg = A;
-  unsigned Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
-  unsigned Prev = Index;
-  std::unique_ptr XarchArg(Opts.ParseOneArg(Args, Index));
-
-  // If the argument parsing failed or more than one argument was
-  // consumed, the -Xarch_ argument's parameter tried to consume
-  // extra arguments. Emit an error and ignore.
-  //
-  // We also want to disallow any options which would alter the
-  // driver behavior; that isn't going to work in our model. We
-  // use isDriverOption() as an approximation, although things
-  // like -O4 are going to slip through.
-  if (!XarchArg || Index > Prev + 1) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_with_args)
-<< A->getAsString(Args);
-continue;
-  } else if (XarchArg->getOption().hasFlag(options::DriverOption)) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_isdriver)
-<< A->getAsString(Args);
-continue;
-  }
-
-  XarchArg->setBaseArg(A);
-
-  A = XarchArg.release();
-  DAL->AddSynthesizedArg(A);
+  TranslateXarchArgs(Args, A, DAL);
 
   // Linker input arguments require custom handling. The problem is that we
   // have already constructed the phase actions, so we can not treat them as
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -804,31 +804,7 @@
   // Skip this argument unless the architecture matches BoundArch
   if (BoundArch.empty() || A->getValue(0) != BoundArch)
 continue;
-
-  unsigned Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
-  unsigned Prev = Index;
-  std::unique_ptr XarchArg(Opts.ParseOneArg(Args, Index));
-
-  // If the argument parsing failed or more than one argument was
-  // consumed, the -Xarch_ argument's parameter tried to consume
-  // extra arguments. Emit an error and ignore.
-  //
-  // We also want to disallow any options which would alter the
-  // driver behavior; that isn't going to work in our model. We
-  // use isDriverOption() as an approximation, although things
-  // like -O4 are going to slip through.
-  if (!XarchArg || Index > Prev + 1) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_with_args)
-<< A->getAsString(Args);
-continue;
-  } else if (XarchArg->getOption().hasFlag(options::DriverOption)) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_isdriver)
- 

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



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

anna wrote:
> lebedev.ri wrote:
> > anna wrote:
> > > Noticed while adding couple more tests, there are 2 bugs here:
> > > 1. the `isGuaranteedToTransferExecutionToSuccessor` check should be 
> > > inverted
> > > 2.  make_range should be until the return instruction - so we do not want 
> > > `std::prev` on the returnInstruction. what's needed is: 
> > > `make_range(RVal->getIterator(), RInst->getIterator())`
> > > This means that from the callsite until (and excluding) the return 
> > > instruction should be guaranteed to transfer execution to successor - 
> > > only then we can backward propagate the attribute to that callsite.
> > Are you aware of `llvm::isValidAssumeForContext()`?
> > All this (including pitfalls) sound awfully close to that function.
> as stated in a previous comment (https://reviews.llvm.org/D76140#1922292), 
> adding `Assumes` here for simple cases seems like an overkill. It has 
> significant IR churn and it also adds a use for something which can be easily 
> inferred. 
> Consider optimizations that depend on facts such as `hasOneUse` or a limited 
> number of uses. We will now be inhibiting those optimizations. 
While i venomously disagree with the avoidance of the usage of one versatile 
interface
and hope things will change once there's more progress on attributor & assume 
bundles,
in this case, as it can be seen even from the signature of the 
`isValidAssumeForContext()` function,
it implies/forces nothing about using assumes, but only performs a validity 
checking,
similar to the `MayContainThrowingOrExitingCall()`

https://github.com/llvm/llvm-project/blob/ca04d0c8fd269978be1c13fe1241172cdfe6a6ea/llvm/lib/Analysis/ValueTracking.cpp#L603

That being said, i haven't reviewed this code so maybe there's some differences 
here
that make that function unapplicable here.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[clang] 78957ba - [NFC] Refactor handling of Xarch option

[Yaxun Liu via cfe-commits]

Author: Yaxun (Sam) Liu
Date: 2020-03-22T14:42:09-04:00
New Revision: 78957bab5515d044caa4ec611b30c952ed63de2d

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

LOG: [NFC] Refactor handling of Xarch option

Extract common code to a function. To prepare for
adding an option for CUDA/HIP host and device only
option.

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

Added: 


Modified: 
clang/include/clang/Driver/ToolChain.h
clang/lib/Driver/ToolChain.cpp
clang/lib/Driver/ToolChains/Cuda.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/lib/Driver/ToolChains/HIP.cpp

Removed: 




diff  --git a/clang/include/clang/Driver/ToolChain.h 
b/clang/include/clang/Driver/ToolChain.h
index 88ce205228fd..115d5ff20f07 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/clang/include/clang/Driver/ToolChain.h
@@ -295,6 +295,12 @@ class ToolChain {
   const llvm::opt::DerivedArgList , bool SameTripleAsHost,
   SmallVectorImpl ) const;
 
+  /// Append the argument following \p A to \p DAL assuming \p A is an Xarch
+  /// argument.
+  virtual void TranslateXarchArgs(const llvm::opt::DerivedArgList ,
+  llvm::opt::Arg *,
+  llvm::opt::DerivedArgList *DAL) const;
+
   /// Choose a tool to use to handle the action \p JA.
   ///
   /// This can be overridden when a particular ToolChain needs to use

diff  --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index dad6a1ac08fb..b1626900318f 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1102,3 +1102,33 @@ llvm::opt::DerivedArgList 
*ToolChain::TranslateOpenMPTargetArgs(
   delete DAL;
   return nullptr;
 }
+
+void ToolChain::TranslateXarchArgs(const llvm::opt::DerivedArgList ,
+   llvm::opt::Arg *,
+   llvm::opt::DerivedArgList *DAL) const {
+  const OptTable  = getDriver().getOpts();
+  unsigned Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
+  unsigned Prev = Index;
+  std::unique_ptr XarchArg(Opts.ParseOneArg(Args, Index));
+
+  // If the argument parsing failed or more than one argument was
+  // consumed, the -Xarch_ argument's parameter tried to consume
+  // extra arguments. Emit an error and ignore.
+  //
+  // We also want to disallow any options which would alter the
+  // driver behavior; that isn't going to work in our model. We
+  // use isDriverOption() as an approximation, although things
+  // like -O4 are going to slip through.
+  if (!XarchArg || Index > Prev + 1) {
+getDriver().Diag(diag::err_drv_invalid_Xarch_argument_with_args)
+<< A->getAsString(Args);
+return;
+  } else if (XarchArg->getOption().hasFlag(options::DriverOption)) {
+getDriver().Diag(diag::err_drv_invalid_Xarch_argument_isdriver)
+<< A->getAsString(Args);
+return;
+  }
+  XarchArg->setBaseArg(A);
+  A = XarchArg.release();
+  DAL->AddSynthesizedArg(A);
+}

diff  --git a/clang/lib/Driver/ToolChains/Cuda.cpp 
b/clang/lib/Driver/ToolChains/Cuda.cpp
index 78139add8a82..9ecb247d80d4 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -804,31 +804,7 @@ CudaToolChain::TranslateArgs(const 
llvm::opt::DerivedArgList ,
   // Skip this argument unless the architecture matches BoundArch
   if (BoundArch.empty() || A->getValue(0) != BoundArch)
 continue;
-
-  unsigned Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
-  unsigned Prev = Index;
-  std::unique_ptr XarchArg(Opts.ParseOneArg(Args, Index));
-
-  // If the argument parsing failed or more than one argument was
-  // consumed, the -Xarch_ argument's parameter tried to consume
-  // extra arguments. Emit an error and ignore.
-  //
-  // We also want to disallow any options which would alter the
-  // driver behavior; that isn't going to work in our model. We
-  // use isDriverOption() as an approximation, although things
-  // like -O4 are going to slip through.
-  if (!XarchArg || Index > Prev + 1) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_with_args)
-<< A->getAsString(Args);
-continue;
-  } else if (XarchArg->getOption().hasFlag(options::DriverOption)) {
-getDriver().Diag(diag::err_drv_invalid_Xarch_argument_isdriver)
-<< A->getAsString(Args);
-continue;
-  }
-  XarchArg->setBaseArg(A);
-  A = XarchArg.release();
-  DAL->AddSynthesizedArg(A);
+  TranslateXarchArgs(Args, A, DAL);
 }
 DAL->append(A);
   }

diff  --git a/clang/lib/Driver/ToolChains/Darwin.cpp 
b/clang/lib/Driver/ToolChains/Darwin.cpp
index a5c08b405914..d1aec649cdc7 100644
--- 

Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[Arthur O'Dwyer via cfe-commits]

On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert 
> wrote:
>
>> Some buildbots, I think only Windows buildbots for some reason, crashed
>> in this function.
>>
>> The reason, as described, is that an `llvm::function_ref` cannot be
>> copied and then reused. It just happened to work on almost all
>> configurations.
>>
>
> That doesn't seem to be accurate, or if it is there's a lot of mistakes in
> the codebase - basically every function_ref parameter I can see in LLVM is
> passing by value, not by const ref. The implementation of
> llvm::function_ref looks quite copyable so long as it doesn't outlive the
> functor it is pointing to.
>

David is correct. llvm::function_ref, like std::reference_wrapper, is a
trivially copyable type, and it's designed to be copied.
Like string_view and reference_wrapper, function_ref is designed to be
passed by value. Redundantly passing function_ref *again by reference* is a
code smell.

I've also checked the code here, and it looks like there are only two
callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and they
are both fine. FWIW, I would recommend reverting Johannes' change and
seeing if those Windows buildbots are still unhappy (and if so, why).


Btw, one failure mode I'm aware of, but which I believe is NOT relevant in
Johannes' case, is that `function_ref`'s converting constructor behaves
differently from its copy constructor.

int main()

{

auto lamb = [](){ return 42; };

auto sheep = [](){ return 43; };

llvm::function_ref one = lamb;


llvm::function_ref twoA = one;// twoA refers to lamb

llvm::function_ref twoB = one;  // twoB refers to one which
refers to lamb


one = sheep;


assert(twoA() == 42);  // twoA refers to lamb

assert(twoB() == 43);  // twoB refers to one which refers to sheep

}

That is, if you have a function that takes a parameter of type
function_ref, and someone passes it an argument of type function_ref,
then inside the function your parameter will be referring to that argument
itself instead of to its referent.
However, in Johannes' particular case, we have no function_refs referring
to other function_refs. We just make a lambda and take a function_ref to
it, the end. So I'm pretty sure this pitfall is irrelevant.

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

Re: [clang] 4a0267e - Convert a reachable llvm_unreachable into an assert.

[Lang Hames via cfe-commits]

Hi Dave, Aaron,

and I spent a lot more time debugging than I should have because I was using
> a release + asserts build and the semantics of llvm_unreachable made
> unfortunate codegen (switching to an assert makes the issue
> immediately obvious).


Huh. I think we should be using llvm_unreachable here, but I would have
expected it to behave similarly to assert(false && ...) in +Asserts builds.
If it isn't that might be a bug?

-- Lang.

On Sun, Mar 22, 2020 at 9:19 AM David Blaikie  wrote:

> On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman 
> wrote:
>
>> On Sat, Mar 21, 2020 at 11:31 PM David Blaikie 
>> wrote:
>> >
>> > Why the change? this seems counter to LLVM's style which pretty
>> consistently uses unreachable rather than assert(false), so far as I know?
>>
>> We're not super consistent (at least within Clang), but the rules as
>> I've generally understood them are to use llvm_unreachable only for
>> truly unreachable code and to use assert(false) when the code is
>> technically reachable but is a programmer mistake to have gotten
>> there.
>
>
> I don't see those as two different things personally - llvm_unreachable is
> used when the programmer believes it to be unreachable (not that it must be
> proven to be unreachable - we have message text there so it's informative
> if the assumption turns out not to hold)
>
>
>> In this particular case, the code is very much reachable
>
>
> In what sense? If it is actually reachable - shouldn't it be tested? (& in
> which case the assert(false) will get in the way of that testing)
>
>
>> and I
>> spent a lot more time debugging than I should have because I was using
>> a release + asserts build and the semantics of llvm_unreachable made
>> unfortunate codegen (switching to an assert makes the issue
>> immediately obvious).
>>
>
> I think it might be reasonable to say that release+asserts to have
> unreachable behave the same way unreachable behaves at -O0 (which is to
> say, much like assert(false)). Clearly release+asserts effects code
> generation, so there's nothing like the "debug info invariance" goal that
> this would be tainting, etc.
>
> Certainly opinions vary here, but here are some commits that show the sort
> of general preference:
> http://llvm.org/viewvc/llvm-project?view=revision=259302
> http://llvm.org/viewvc/llvm-project?view=revision=253005
> http://llvm.org/viewvc/llvm-project?view=revision=251266
>
> And some counter examples:
> http://llvm.org/viewvc/llvm-project?view=revision=225043
> Including this thread where Chandler originally (not sure what his take on
> it is these days) expressed some ideas more along your lines:
>
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583
>
> But I'm always pretty concerned about the idea that assertions should be
> used in places where the behavior of the program has any constraints when
> the assertion is false...
>
> - Dave
>
>
>>
>> >
>> > On Tue, Mar 10, 2020 at 11:22 AM Aaron Ballman via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>> >>
>> >>
>> >> Author: Aaron Ballman
>> >> Date: 2020-03-10T14:22:21-04:00
>> >> New Revision: 4a0267e3ad8c4d47f267d7d960f127e099fb4818
>> >>
>> >> URL:
>> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818
>> >> DIFF:
>> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff
>> >>
>> >> LOG: Convert a reachable llvm_unreachable into an assert.
>> >>
>> >> Added:
>> >>
>> >>
>> >> Modified:
>> >> clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> >>
>> >> Removed:
>> >>
>> >>
>> >>
>> >>
>> 
>> >> diff  --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> >> index 01ac2bc83bb6..99e16752b51a 100644
>> >> --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> >> +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> >> @@ -134,9 +134,9 @@ StringRef
>> AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,
>> >>  CheckerName = CheckerName.substr(0, Pos);
>> >>} while (!CheckerName.empty() && SearchInParents);
>> >>
>> >> -  llvm_unreachable("Unknown checker option! Did you call
>> getChecker*Option "
>> >> -   "with incorrect parameters? User input must've
>> been "
>> >> -   "verified by CheckerRegistry.");
>> >> +  assert(false && "Unknown checker option! Did you call
>> getChecker*Option "
>> >> +  "with incorrect parameters? User input must've been
>> "
>> >> +  "verified by CheckerRegistry.");
>> >>
>> >>return "";
>> >>  }
>> >>
>> >>
>> >>
>> >> ___
>> >> cfe-commits mailing list
>> >> cfe-commits@lists.llvm.org
>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list

Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[Johannes Doerfert via cfe-commits]

Some buildbots, I think only Windows buildbots for some reason, crashed 
in this function.


The reason, as described, is that an `llvm::function_ref` cannot be 
copied and then reused. It just happened to work on almost all 
configurations.


The change was not a "shot in the dark" but identified as proper fix by 
examining the `llvm::function_ref` implementation.


I could not reproduce the error on any machine I had immediately access 
to so maybe you can call that part "shot in the dark".



I'm not sure what else you want me to add here as a explanation but 
please let me know if there is anything I can do.



On 3/22/20 12:21 AM, David Blaikie wrote:

"a problem" seems a smidge vague - was there a specific explanation for the
kind of problem that was signalled? Or was this fix a bit of a shot in the
dark?

On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <
cfe-commits@lists.llvm.org> wrote:


Author: Johannes Doerfert
Date: 2020-02-16T00:51:11-06:00
New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2

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

LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused

Some buildbots signaled a problem in this method when the
llvm::function_ref was copied and reused after 1228d42ddab8. To
eliminate the problem we avoid copying the llvm::function_ref and
instead we pass it as a const reference.

Added:


Modified:
 clang/include/clang/AST/OpenMPClause.h

Removed:





diff  --git a/clang/include/clang/AST/OpenMPClause.h
b/clang/include/clang/AST/OpenMPClause.h
index a3831fd5950f..453c068bbeb0 100644
--- a/clang/include/clang/AST/OpenMPClause.h
+++ b/clang/include/clang/AST/OpenMPClause.h
@@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
llvm::SmallVector Sets;

bool anyScoreOrCondition(
-  llvm::function_ref Cond) {
-return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet ) {
+  const llvm::function_ref ) {
+return llvm::any_of(Sets, [](OMPTraitInfo::OMPTraitSet ) {
return llvm::any_of(
-  Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector )
{
+  Set.Selectors, [](OMPTraitInfo::OMPTraitSelector
) {
  return Cond(Selector.ScoreOrCondition,
  /* IsScore */ Selector.Kind !=
  llvm::omp::TraitSelector::user_condition);



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


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

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

[Anna Thomas via Phabricator via cfe-commits]

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



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

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

[Anna Thomas via Phabricator via cfe-commits]

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

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

1 the isGuaranteedToTransferExecutionToSuccessor check should be inverted

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140

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

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

Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[David Blaikie via cfe-commits]

On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert 
wrote:

> Some buildbots, I think only Windows buildbots for some reason, crashed
> in this function.
>
> The reason, as described, is that an `llvm::function_ref` cannot be
> copied and then reused. It just happened to work on almost all
> configurations.
>

That doesn't seem to be accurate, or if it is there's a lot of mistakes in
the codebase - basically every function_ref parameter I can see in LLVM is
passing by value, not by const ref. The implementation of
llvm::function_ref looks quite copyable so long as it doesn't outlive the
functor it is pointing to.


> The change was not a "shot in the dark" but identified as proper fix by
> examining the `llvm::function_ref` implementation.
>

What aspect of the implementation of llvm::function_ref makes it not safe
to copy in the way the code original did so? My reading of it makes it look
like it's OK to copy so long as it doesn't outlive the functor it is
pointing to (& the code as originall written doesn't look like it's
outliving the caller's functor)


> I could not reproduce the error on any machine I had immediately access
> to so maybe you can call that part "shot in the dark".
>

Sounds like this sort of thing could/should've shown up with msan, maybe?
Were you able to test with any sanitizers?


>
>
> I'm not sure what else you want me to add here as a explanation but
> please let me know if there is anything I can do.
>
>
> On 3/22/20 12:21 AM, David Blaikie wrote:
> > "a problem" seems a smidge vague - was there a specific explanation for
> the
> > kind of problem that was signalled? Or was this fix a bit of a shot in
> the
> > dark?
> >
> > On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <
> > cfe-commits@lists.llvm.org> wrote:
> >
> >> Author: Johannes Doerfert
> >> Date: 2020-02-16T00:51:11-06:00
> >> New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> >>
> >> URL:
> >>
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> >> DIFF:
> >>
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff
> >>
> >> LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused
> >>
> >> Some buildbots signaled a problem in this method when the
> >> llvm::function_ref was copied and reused after 1228d42ddab8. To
> >> eliminate the problem we avoid copying the llvm::function_ref and
> >> instead we pass it as a const reference.
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >>  clang/include/clang/AST/OpenMPClause.h
> >>
> >> Removed:
> >>
> >>
> >>
> >>
> >>
> 
> >> diff  --git a/clang/include/clang/AST/OpenMPClause.h
> >> b/clang/include/clang/AST/OpenMPClause.h
> >> index a3831fd5950f..453c068bbeb0 100644
> >> --- a/clang/include/clang/AST/OpenMPClause.h
> >> +++ b/clang/include/clang/AST/OpenMPClause.h
> >> @@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
> >> llvm::SmallVector Sets;
> >>
> >> bool anyScoreOrCondition(
> >> -  llvm::function_ref Cond) {
> >> -return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet ) {
> >> +  const llvm::function_ref
> ) {
> >> +return llvm::any_of(Sets, [](OMPTraitInfo::OMPTraitSet ) {
> >> return llvm::any_of(
> >> -  Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector
> )
> >> {
> >> +  Set.Selectors, [](OMPTraitInfo::OMPTraitSelector
> >> ) {
> >>   return Cond(Selector.ScoreOrCondition,
> >>   /* IsScore */ Selector.Kind !=
> >>   llvm::omp::TraitSelector::user_condition);
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

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

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:7727
 
+// Move function type attribute to the declarator.
+case ParsedAttr::AT_LifetimeContract:

aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > This is not an okay thing to do for C++ attributes. They have a specific 
> > > meaning as to what they appertain to and do not move around to better 
> > > subjects like GNU-style attributes.
> > Yeah, I know that. But this is problematic in the standard. See the 
> > contracts proposal which also kind of violates this rule by adding an 
> > exception. Basically, this is the poor man's version of emulating the 
> > contracts proposal.  In the long term we plan to piggyback on contracts 
> > rather than hacking the C++ attributes.
> The contracts proposal was not adopted and I am opposed to adding this change 
> for any standard attribute. We've done a lot of work to ensure that those 
> attributes attach to the correct thing based on lexical placement and I don't 
> want to start to undo that effort.
I am not sure how to proceed in this case. As far as I understand this wasn't 
the main reason why didn't we adopt the contracts proposal.

While I do understand why is it good to make the lexical placement matter, but 
the current placement hinders non-trivial use-cases. Currently, we either have 
a type attribute or we cannot mention any of the formal parameters. This is why 
the contracts cannot work without changing attributes in the standard, and the 
same reason why this patch will never work without doing this. Or do you have 
an alternative in mind?

Do you think introducing a frontend flag for this attribute would help? This 
would make it clear that we are currently using a non-standard feature that 
needs to be enabled explicitly. Again, this is a temporary solution, that will 
go away as soon as we have contracts. Since contracts need to solve the same 
problem, whatever the solution will be, this patch can piggyback on that.

What do you think?


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

https://reviews.llvm.org/D72810



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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



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

anna wrote:
> Noticed while adding couple more tests, there are 2 bugs here:
> 1. the `isGuaranteedToTransferExecutionToSuccessor` check should be inverted
> 2.  make_range should be until the return instruction - so we do not want 
> `std::prev` on the returnInstruction. what's needed is: 
> `make_range(RVal->getIterator(), RInst->getIterator())`
> This means that from the callsite until (and excluding) the return 
> instruction should be guaranteed to transfer execution to successor - only 
> then we can backward propagate the attribute to that callsite.
Are you aware of `llvm::isValidAssumeForContext()`?
All this (including pitfalls) sound awfully close to that function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76546: [Hexagon] MaxAtomicPromoteWidth, MaxAtomicInlineWidth are not getting set.

[Sid Manning via Phabricator via cfe-commits]

sidneym updated this revision to Diff 251896.
sidneym added a comment.

remove extra line break.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76546

Files:
  clang/lib/Basic/Targets/Hexagon.h
  clang/test/Preprocessor/hexagon-predefines.c


Index: clang/test/Preprocessor/hexagon-predefines.c
===
--- clang/test/Preprocessor/hexagon-predefines.c
+++ clang/test/Preprocessor/hexagon-predefines.c
@@ -113,3 +113,19 @@
 // CHECK-LINUX: #define __unix__ 1
 // CHECK-LINUX: #define linux 1
 // CHECK-LINUX: #define unix 1
+
+// RUN: %clang_cc1 -E -dM -triple hexagon-unknown-linux-musl \
+// RUN: -target-cpu hexagonv67 -target-feature +hvxv67 \
+// RUN: -target-feature +hvx-length128b %s | FileCheck \
+// RUN: %s -check-prefix CHECK-ATOMIC
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_BOOL_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_CHAR16_T_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_CHAR32_T_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_CHAR_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_INT_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_LLONG_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_LONG_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_POINTER_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_SHORT_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_WCHAR_T_LOCK_FREE 2
+
Index: clang/lib/Basic/Targets/Hexagon.h
===
--- clang/lib/Basic/Targets/Hexagon.h
+++ clang/lib/Basic/Targets/Hexagon.h
@@ -57,6 +57,7 @@
 LargeArrayAlign = 64;
 UseBitFieldTypeAlignment = true;
 ZeroLengthBitfieldBoundary = 32;
+MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
 
 // These are the default values anyway, but explicitly make sure
 // that the size of the boolean type is 8 bits. Bool vectors are used


Index: clang/test/Preprocessor/hexagon-predefines.c
===
--- clang/test/Preprocessor/hexagon-predefines.c
+++ clang/test/Preprocessor/hexagon-predefines.c
@@ -113,3 +113,19 @@
 // CHECK-LINUX: #define __unix__ 1
 // CHECK-LINUX: #define linux 1
 // CHECK-LINUX: #define unix 1
+
+// RUN: %clang_cc1 -E -dM -triple hexagon-unknown-linux-musl \
+// RUN: -target-cpu hexagonv67 -target-feature +hvxv67 \
+// RUN: -target-feature +hvx-length128b %s | FileCheck \
+// RUN: %s -check-prefix CHECK-ATOMIC
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_BOOL_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_CHAR16_T_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_CHAR32_T_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_CHAR_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_INT_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_LLONG_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_LONG_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_POINTER_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_SHORT_LOCK_FREE 2
+// CHECK-ATOMIC: #define __CLANG_ATOMIC_WCHAR_T_LOCK_FREE 2
+
Index: clang/lib/Basic/Targets/Hexagon.h
===
--- clang/lib/Basic/Targets/Hexagon.h
+++ clang/lib/Basic/Targets/Hexagon.h
@@ -57,6 +57,7 @@
 LargeArrayAlign = 64;
 UseBitFieldTypeAlignment = true;
 ZeroLengthBitfieldBoundary = 32;
+MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
 
 // These are the default values anyway, but explicitly make sure
 // that the size of the boolean type is 8 bits. Bool vectors are used
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anna Thomas via Phabricator via cfe-commits]

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



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

Re: [clang] 4a0267e - Convert a reachable llvm_unreachable into an assert.

[Aaron Ballman via cfe-commits]

On Sun, Mar 22, 2020 at 12:19 PM David Blaikie  wrote:
>
> On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman  wrote:
>>
>> On Sat, Mar 21, 2020 at 11:31 PM David Blaikie  wrote:
>> >
>> > Why the change? this seems counter to LLVM's style which pretty 
>> > consistently uses unreachable rather than assert(false), so far as I know?
>>
>> We're not super consistent (at least within Clang), but the rules as
>> I've generally understood them are to use llvm_unreachable only for
>> truly unreachable code and to use assert(false) when the code is
>> technically reachable but is a programmer mistake to have gotten
>> there.
>
>
> I don't see those as two different things personally - llvm_unreachable is 
> used when the programmer believes it to be unreachable (not that it must be 
> proven to be unreachable - we have message text there so it's informative if 
> the assumption turns out not to hold)

The message text doesn't help when the code is reached in release
mode, which was the issue. Asserts + release you still get some
helpful text saying "you screwed up". llvm_unreachable in release
mode, you may get lucky or you may not (in this case, I didn't get
lucky -- there was no text, just a crash).

>> In this particular case, the code is very much reachable
>
>
> In what sense? If it is actually reachable - shouldn't it be tested? (& in 
> which case the assert(false) will get in the way of that testing)

In the sense that normal code paths reach that code easily. Basically,
that code is checking to see whether a plugin you've written properly
sets up its options or not. When you're developing a plugin, it's
quite reasonable to expect you won't get it just right on the first
try, so you hit the code path but only as a result of you not writing
the plugin quite right. So under normal conditions (once the plugin is
working), the code path should not be reached but under development
the code path gets reached accidentally.

>> and I
>> spent a lot more time debugging than I should have because I was using
>> a release + asserts build and the semantics of llvm_unreachable made
>> unfortunate codegen (switching to an assert makes the issue
>> immediately obvious).
>
>
> I think it might be reasonable to say that release+asserts to have 
> unreachable behave the same way unreachable behaves at -O0 (which is to say, 
> much like assert(false)). Clearly release+asserts effects code generation, so 
> there's nothing like the "debug info invariance" goal that this would be 
> tainting, etc.
>
> Certainly opinions vary here, but here are some commits that show the sort of 
> general preference:
> http://llvm.org/viewvc/llvm-project?view=revision=259302
> http://llvm.org/viewvc/llvm-project?view=revision=253005
> http://llvm.org/viewvc/llvm-project?view=revision=251266
>
> And some counter examples:
> http://llvm.org/viewvc/llvm-project?view=revision=225043
> Including this thread where Chandler originally (not sure what his take on it 
> is these days) expressed some ideas more along your lines:
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583
>
> But I'm always pretty concerned about the idea that assertions should be used 
> in places where the behavior of the program has any constraints when the 
> assertion is false...

I'm opposed to using unreachability hints on control flow paths you
expect to reach -- the semantics are just plain wrong, even if you can
get the same end result of controlled crash + message. In this
particular case, the code is reachable but erroneous to do so -- and
that's what assertions are intended to be used for. My preference is
to use llvm_unreachable because the code is unreachable, not because
the code should not be reachable only if everything works out right.

It may be that we're not in agreement about the definition of "expects
to reach" here. To me, this code clearly expects to reach that path:
it's a search over a finite collection where it's possible the thing
being searched for is not found. The "we didn't find the thing we were
expecting to find" assertion is reasonable because this isn't the
result of end-user error (then we'd fire a diagnostic instead) but is
the result of a plugin author's error. If the collection and the input
to the search were both fully under control of the analyzer (so the
search cannot fail without exceptional circumstances), then I think
llvm_unreachable could be reasonable.

~Aaron

>
> - Dave
>
>>
>>
>> >
>> > On Tue, Mar 10, 2020 at 11:22 AM Aaron Ballman via cfe-commits 
>> >  wrote:
>> >>
>> >>
>> >> Author: Aaron Ballman
>> >> Date: 2020-03-10T14:22:21-04:00
>> >> New Revision: 4a0267e3ad8c4d47f267d7d960f127e099fb4818
>> >>
>> >> URL: 
>> >> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818
>> >> DIFF: 
>> >> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff
>> >>
>> >> LOG: Convert a reachable llvm_unreachable into an assert.
>> >>
>> >> 

Re: [clang] 4a0267e - Convert a reachable llvm_unreachable into an assert.

[David Blaikie via cfe-commits]

On Sun, Mar 22, 2020 at 6:34 AM Aaron Ballman 
wrote:

> On Sat, Mar 21, 2020 at 11:31 PM David Blaikie  wrote:
> >
> > Why the change? this seems counter to LLVM's style which pretty
> consistently uses unreachable rather than assert(false), so far as I know?
>
> We're not super consistent (at least within Clang), but the rules as
> I've generally understood them are to use llvm_unreachable only for
> truly unreachable code and to use assert(false) when the code is
> technically reachable but is a programmer mistake to have gotten
> there.


I don't see those as two different things personally - llvm_unreachable is
used when the programmer believes it to be unreachable (not that it must be
proven to be unreachable - we have message text there so it's informative
if the assumption turns out not to hold)


> In this particular case, the code is very much reachable


In what sense? If it is actually reachable - shouldn't it be tested? (& in
which case the assert(false) will get in the way of that testing)


> and I
> spent a lot more time debugging than I should have because I was using
> a release + asserts build and the semantics of llvm_unreachable made
> unfortunate codegen (switching to an assert makes the issue
> immediately obvious).
>

I think it might be reasonable to say that release+asserts to have
unreachable behave the same way unreachable behaves at -O0 (which is to
say, much like assert(false)). Clearly release+asserts effects code
generation, so there's nothing like the "debug info invariance" goal that
this would be tainting, etc.

Certainly opinions vary here, but here are some commits that show the sort
of general preference:
http://llvm.org/viewvc/llvm-project?view=revision=259302
http://llvm.org/viewvc/llvm-project?view=revision=253005
http://llvm.org/viewvc/llvm-project?view=revision=251266

And some counter examples:
http://llvm.org/viewvc/llvm-project?view=revision=225043
Including this thread where Chandler originally (not sure what his take on
it is these days) expressed some ideas more along your lines:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110919/thread.html#46583

But I'm always pretty concerned about the idea that assertions should be
used in places where the behavior of the program has any constraints when
the assertion is false...

- Dave


>
> >
> > On Tue, Mar 10, 2020 at 11:22 AM Aaron Ballman via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> >>
> >>
> >> Author: Aaron Ballman
> >> Date: 2020-03-10T14:22:21-04:00
> >> New Revision: 4a0267e3ad8c4d47f267d7d960f127e099fb4818
> >>
> >> URL:
> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818
> >> DIFF:
> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff
> >>
> >> LOG: Convert a reachable llvm_unreachable into an assert.
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >> clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
> >>
> >> Removed:
> >>
> >>
> >>
> >>
> 
> >> diff  --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
> b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
> >> index 01ac2bc83bb6..99e16752b51a 100644
> >> --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
> >> +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
> >> @@ -134,9 +134,9 @@ StringRef
> AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,
> >>  CheckerName = CheckerName.substr(0, Pos);
> >>} while (!CheckerName.empty() && SearchInParents);
> >>
> >> -  llvm_unreachable("Unknown checker option! Did you call
> getChecker*Option "
> >> -   "with incorrect parameters? User input must've been
> "
> >> -   "verified by CheckerRegistry.");
> >> +  assert(false && "Unknown checker option! Did you call
> getChecker*Option "
> >> +  "with incorrect parameters? User input must've been "
> >> +  "verified by CheckerRegistry.");
> >>
> >>return "";
> >>  }
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76545: [clang-tidy] Add a new check group 'experimental-'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think we want to keep the experimental checks grouped to their parent module 
rather than being in a module of their own. Also, if we want to allow 
experimental checks, I think we should have some rough community consensus on 
1) what are the criteria for something being experimental and included in 
clang-tidy (as opposed to in an out-of-tree repo) and what are the maintenance 
expectations for those checks, 2) what is the criteria for moving something 
*out* of experimental mode, and 3) how long should something be experimental 
without improvement before it's removed from clang-tidy?

I'm not opposed to the idea of experimental checks, but I don't want to see 
"experimental" become a dumping ground for low-quality checks that we then have 
to maintain indefinitely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76545



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

[PATCH] D76549: [clang-tidy] Fix RenamerClangTidy handling qualified TypeLocs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'd appreciate seeing your test cases from the summary added to a test 
somewhere.




Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:206
   if (const auto *Loc = Result.Nodes.getNodeAs("typeLoc")) {
+auto Unqual = Loc->getUnqualifiedLoc();
 NamedDecl *Decl = nullptr;

Don't use `auto` as the type is not spelling out in the initialization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76549



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

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

[Teresa Johnson via Phabricator via cfe-commits]

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

PTAL




Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1999
+WholeProgramDevirtResolution *Res = nullptr;
+if (ExportSummary && isa(S.first.TypeID))
+  // Create the type id summary resolution regardlness of whether we can

tejohnson wrote:
> The fix needed for the Chromium issue was to guard the TypeIdSummary creation 
> here by whether the TypeID exists in the TypeIdMap (which makes it match the 
> comment in fact), as we don't want to create a summary if the type id is not 
> used on a global (in which case it should in fact be Unsat). The equivalent 
> code in the index-only WPD is essentially already guarded by that condition, 
> because of the way the CallSlots are created (and in fact there is an assert 
> in that code that we have a use on a vtable, i.e. that a 
> TypeIdCompatibleVtableSummary is found for the TypeID).
Improved the fix I had made here for Chromium as it wasn't robust enough to 
handle all cases. Specifically, if we came through this code multiple times 
with the same type id, the TypeIdMap entry would no longer be missing since we 
unconditionally created it in the call to tryFindVirtualCallTargets below (via 
the operator[]). Changed it to check for a non-empty set (after more eagerly 
calling operator[]). I beefed up the test I had for this issue 
(llvm/test/ThinLTO/X86/cfi-unsat.ll) to cover this additional case.



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1784
+  ImportSummary->getTypeIdSummary(cast(TypeId)->getString());
+  if (!TidSummary)
+RemoveTypeTestAssumes();

Here is the fix for the issue with the multi-stage clang bootstrap test 
failures described in D75201. I also restructured this code to try to make it 
clearer (rather than an early continue when we don't need to remove type test 
assumes, make it an explicit removal when needed).

Added new test llvm/test/ThinLTO/X86/type_test_noindircall.ll for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson updated this revision to Diff 251893.
tejohnson added a comment.

Includes fixe for 2-stage clang bootstrap test failures and an expanded
fix for Chromium issue, plus new tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242

Files:
  llvm/include/llvm/IR/ModuleSummaryIndex.h
  llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/IR/AsmWriter.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/Bitcode/summary_version.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/ThinLTO/X86/Inputs/cfi-unsat.ll
  llvm/test/ThinLTO/X86/cfi-unsat.ll
  llvm/test/ThinLTO/X86/nodevirt-nonpromoted-typeid.ll
  llvm/test/ThinLTO/X86/type_test_noindircall.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl2.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll

Index: llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
===
--- llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
+++ llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
@@ -25,7 +25,7 @@
   %fptr = load i8*, i8** %fptrptr
   %fptr_casted = bitcast i8* %fptr to i32 (i8*)*
   %result = call i32 %fptr_casted(i8* %obj)
-  ; CHECK-NOT: call
+  ; CHECK-NOT: call i32 %
   ; CHECK: ret i32 123
   ret i32 %result
 }
Index: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
===
--- llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
+++ llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
@@ -32,7 +32,7 @@
 ; SUMMARY-NEXT: TypeIdMap:
 ; SUMMARY-NEXT:   typeid1:
 ; SUMMARY-NEXT: TTRes:
-; SUMMARY-NEXT:   Kind:Unsat
+; SUMMARY-NEXT:   Kind:Unknown
 ; SUMMARY-NEXT:   SizeM1BitWidth:  0
 ; SUMMARY-NEXT:   AlignLog2:   0
 ; SUMMARY-NEXT:   SizeM1:  0
Index: llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
===
--- llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
+++ llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
@@ -9,7 +9,7 @@
 ; SUMMARY:  TypeIdMap:
 ; SUMMARY-NEXT:   typeid3:
 ; SUMMARY-NEXT: TTRes:
-; SUMMARY-NEXT:   Kind:Unsat
+; SUMMARY-NEXT:   Kind:Unknown
 ; SUMMARY-NEXT:   SizeM1BitWidth:  0
 ; SUMMARY-NEXT:   AlignLog2:   0
 ; SUMMARY-NEXT:   SizeM1:  0
@@ -29,7 +29,7 @@
 ; SUMMARY-ARM-NEXT: Bit: 1
 ; SUMMARY-NEXT:   typeid4:
 ; SUMMARY-NEXT: TTRes:
-; SUMMARY-NEXT:   Kind:Unsat
+; SUMMARY-NEXT:   Kind:Unknown
 ; SUMMARY-NEXT:   SizeM1BitWidth:  0
 ; SUMMARY-NEXT:   AlignLog2:   0
 ; SUMMARY-NEXT:   SizeM1:  0
Index: llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
===
--- llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
+++ llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
@@ -6,7 +6,7 @@
 ; SUMMARY:  TypeIdMap:
 ; SUMMARY-NEXT:   typeid3:
 ; SUMMARY-NEXT: TTRes:
-; SUMMARY-NEXT:   Kind:Unsat
+; SUMMARY-NEXT:   Kind:Unknown
 ; SUMMARY-NEXT:   SizeM1BitWidth:  0
 ; SUMMARY-NEXT:   AlignLog2:   0
 ; SUMMARY-NEXT:   SizeM1:  0
@@ -24,7 +24,7 @@
 ; SUMMARY-NEXT: Bit: 0
 ; SUMMARY-NEXT:   typeid4:
 ; SUMMARY-NEXT: TTRes:
-; SUMMARY-NEXT:   Kind:Unsat
+; SUMMARY-NEXT:   Kind:Unknown
 ; SUMMARY-NEXT:   SizeM1BitWidth:  0
 ; SUMMARY-NEXT:   AlignLog2:   0
 ; SUMMARY-NEXT:   SizeM1:  0
Index: llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
===
--- llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
+++ llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
@@ -6,7 +6,7 @@
 ; SUMMARY:  TypeIdMap:
 ; SUMMARY-NEXT:   typeid4:
 ; SUMMARY-NEXT: TTRes:
-; SUMMARY-NEXT:   Kind:Unsat
+; SUMMARY-NEXT:   Kind:Unknown
 ; 

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

[Teresa Johnson via Phabricator via cfe-commits]

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

Updating with fixes since this was reverted in 
80bf137fa132ea33204e98bbefa924afe9258a4e 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

Re: [clang] 7a42bab - Reland "[DebugInfo][clang][DWARF5]: Added support for debuginfo generation for defaulted parameters

[David Blaikie via cfe-commits]

In the future, please include more detail in the commit message of a
recommit about how the patch addresses the problems that lead to the
previous revert. This makes it easier for someone reviewing the code to
examine the new changes & ensure they're correct/appropriate.

On Mon, Mar 2, 2020 at 3:16 AM Sourabh Singh Tomar via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

>
> Author: Awanish Pandey
> Date: 2020-03-02T16:45:48+05:30
> New Revision: 7a42babeb83e3927e89e72a0e7e45be9d41b6c23
>
> URL:
> https://github.com/llvm/llvm-project/commit/7a42babeb83e3927e89e72a0e7e45be9d41b6c23
> DIFF:
> https://github.com/llvm/llvm-project/commit/7a42babeb83e3927e89e72a0e7e45be9d41b6c23.diff
>
> LOG: Reland "[DebugInfo][clang][DWARF5]: Added support for debuginfo
> generation for defaulted parameters
> in C++ templates."
>
> This was reverted in 802b22b5c8c30bebc1695a217478be02653c6b53 due to
> missing .bc file and a chromium bot failure.
> https://bugs.chromium.org/p/chromium/issues/detail?id=1057559#c1
> This revision address both of them.
>
> Summary:
> This patch adds support for debuginfo generation for defaulted
> parameters in clang and also extends corresponding DebugMetadata/IR to
> support this feature.
>
> Reviewers: probinson, aprantl, dblaikie
>
> Reviewed By: aprantl, dblaikie
>
> Differential Revision: https://reviews.llvm.org/D73462
>
> Added:
> llvm/test/Assembler/DIDefaultTemplateParam.ll
> llvm/test/Bitcode/DITemplateParameter-5.0.ll
> llvm/test/Bitcode/DITemplateParameter-5.0.ll.bc
>
> Modified:
> clang/lib/CodeGen/CGDebugInfo.cpp
> llvm/include/llvm/IR/DIBuilder.h
> llvm/include/llvm/IR/DebugInfoMetadata.h
> llvm/lib/AsmParser/LLParser.cpp
> llvm/lib/Bitcode/Reader/MetadataLoader.cpp
> llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
> llvm/lib/IR/AsmWriter.cpp
> llvm/lib/IR/DIBuilder.cpp
> llvm/lib/IR/DebugInfoMetadata.cpp
> llvm/lib/IR/LLVMContextImpl.h
> llvm/unittests/IR/MetadataTest.cpp
>
> Removed:
>
>
>
>
> 
> diff  --git a/clang/lib/CodeGen/CGDebugInfo.cpp
> b/clang/lib/CodeGen/CGDebugInfo.cpp
> index e171082942f6..cbf45a5cf748 100644
> --- a/clang/lib/CodeGen/CGDebugInfo.cpp
> +++ b/clang/lib/CodeGen/CGDebugInfo.cpp
> @@ -1787,18 +1787,36 @@ CGDebugInfo::CollectTemplateParams(const
> TemplateParameterList *TPList,
>for (unsigned i = 0, e = TAList.size(); i != e; ++i) {
>  const TemplateArgument  = TAList[i];
>  StringRef Name;
> +bool defaultParameter = false;
>  if (TPList)
>Name = TPList->getParam(i)->getName();
>  switch (TA.getKind()) {
>  case TemplateArgument::Type: {
>llvm::DIType *TTy = getOrCreateType(TA.getAsType(), Unit);
> -  TemplateParams.push_back(
> -  DBuilder.createTemplateTypeParameter(TheCU, Name, TTy));
> +
> +  if (TPList)
> +if (auto *templateType =
> +
> dyn_cast_or_null(TPList->getParam(i)))
> +  if (templateType->hasDefaultArgument())
> +defaultParameter =
> +templateType->getDefaultArgument() == TA.getAsType();
> +
> +  TemplateParams.push_back(DBuilder.createTemplateTypeParameter(
> +  TheCU, Name, TTy, defaultParameter));
> +
>  } break;
>  case TemplateArgument::Integral: {
>llvm::DIType *TTy = getOrCreateType(TA.getIntegralType(), Unit);
> +  if (TPList && CGM.getCodeGenOpts().DwarfVersion >= 5)
> +if (auto *templateType =
> +
> dyn_cast_or_null(TPList->getParam(i)))
> +  if (templateType->hasDefaultArgument())
> +defaultParameter =
> +templateType->getDefaultArgument()->EvaluateKnownConstInt(
> +CGM.getContext()) == TA.getAsIntegral();
> +
>TemplateParams.push_back(DBuilder.createTemplateValueParameter(
> -  TheCU, Name, TTy,
> +  TheCU, Name, TTy, defaultParameter,
>llvm::ConstantInt::get(CGM.getLLVMContext(),
> TA.getAsIntegral(;
>  } break;
>  case TemplateArgument::Declaration: {
> @@ -1837,7 +1855,7 @@ CGDebugInfo::CollectTemplateParams(const
> TemplateParameterList *TPList,
>  V = V->stripPointerCasts();
>}
>TemplateParams.push_back(DBuilder.createTemplateValueParameter(
> -  TheCU, Name, TTy, cast_or_null(V)));
> +  TheCU, Name, TTy, defaultParameter,
> cast_or_null(V)));
>  } break;
>  case TemplateArgument::NullPtr: {
>QualType T = TA.getNullPtrType();
> @@ -1855,8 +1873,8 @@ CGDebugInfo::CollectTemplateParams(const
> TemplateParameterList *TPList,
>V = CGM.getCXXABI().EmitNullMemberPointer(MPT);
>if (!V)
>  V = llvm::ConstantInt::get(CGM.Int8Ty, 0);
> -  TemplateParams.push_back(
> -  DBuilder.createTemplateValueParameter(TheCU, Name, TTy, V));
> +  TemplateParams.push_back(DBuilder.createTemplateValueParameter(
> +  TheCU, Name, TTy, defaultParameter, V));
>  } 

Re: [clang] d9b9621 - Reland D73534: [DebugInfo] Enable the debug entry values feature by default

[David Blaikie via cfe-commits]

Also, this patch was reverted and recommitted several times (more than I
think would be ideal) - please include more details in the commit message
about what went wrong, what was fixed, what testing was missed in the first
place and done in subsequent precommit validation (& is there anything more
broadly we could learn from this patches story to avoid this kind of
revert/recommit repetition in the future?)

On Sat, Mar 21, 2020 at 8:13 PM David Blaikie  wrote:

> Please include the "Differential Revision" line so that Phab picks up
> commits like this and ties them into the review (& also makes it
> conveniently clickable to jump from the commit mail to the review)
>
> On Thu, Mar 19, 2020 at 5:58 AM Djordje Todorovic via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>>
>> Author: Djordje Todorovic
>> Date: 2020-03-19T13:57:30+01:00
>> New Revision: d9b962100942c71a4c26debaa716f7ab0c4ea8a1
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/d9b962100942c71a4c26debaa716f7ab0c4ea8a1
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/d9b962100942c71a4c26debaa716f7ab0c4ea8a1.diff
>>
>> LOG: Reland D73534: [DebugInfo] Enable the debug entry values feature by
>> default
>>
>> The issue that was causing the build failures was fixed with the D76164.
>>
>> Added:
>> llvm/test/DebugInfo/X86/no-entry-values-with-O0.ll
>>
>> Modified:
>> clang/include/clang/Basic/CodeGenOptions.def
>> clang/include/clang/Driver/CC1Options.td
>> clang/lib/CodeGen/BackendUtil.cpp
>> clang/lib/CodeGen/CGDebugInfo.cpp
>> clang/lib/Frontend/CompilerInvocation.cpp
>> clang/test/CodeGen/debug-info-extern-call.c
>> clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp
>> lldb/packages/Python/lldbsuite/test/decorators.py
>>
>> lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/Makefile
>> llvm/include/llvm/Target/TargetMachine.h
>> llvm/include/llvm/Target/TargetOptions.h
>> llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
>> llvm/lib/CodeGen/CommandFlags.cpp
>> llvm/lib/CodeGen/LiveDebugValues.cpp
>> llvm/lib/CodeGen/TargetOptionsImpl.cpp
>> llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
>> llvm/lib/Target/ARM/ARMTargetMachine.cpp
>> llvm/lib/Target/X86/X86TargetMachine.cpp
>> llvm/test/CodeGen/MIR/Hexagon/bundled-call-site-info.mir
>> llvm/test/CodeGen/MIR/X86/call-site-info-error4.mir
>> llvm/test/CodeGen/X86/call-site-info-output.ll
>> llvm/test/DebugInfo/AArch64/dbgcall-site-float-entry-value.ll
>> llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-orr-moves.mir
>> llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovd.mir
>> llvm/test/DebugInfo/MIR/ARM/call-site-info-vmovs.mir
>> llvm/test/DebugInfo/MIR/ARM/dbgcall-site-propagated-value.mir
>>
>> llvm/test/DebugInfo/MIR/Hexagon/dbgcall-site-instr-before-bundled-call.mir
>>
>> llvm/test/DebugInfo/MIR/Hexagon/live-debug-values-bundled-entry-values.mir
>> llvm/test/DebugInfo/MIR/SystemZ/call-site-lzer.mir
>> llvm/test/DebugInfo/MIR/X86/DW_OP_entry_value.mir
>> llvm/test/DebugInfo/MIR/X86/call-site-gnu-vs-dwarf5-attrs.mir
>> llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-copy-super-sub.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-lea-interpretation.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-partial-describe.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-reference.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-reg-shuffle.mir
>> llvm/test/DebugInfo/MIR/X86/dbgcall-site-two-fwd-reg-defs.mir
>> llvm/test/DebugInfo/MIR/X86/dbginfo-entryvals.mir
>> llvm/test/DebugInfo/MIR/X86/debug-call-site-param.mir
>> llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir
>> llvm/test/DebugInfo/MIR/X86/entry-values-diamond-bbs.mir
>> llvm/test/DebugInfo/MIR/X86/propagate-entry-value-cross-bbs.mir
>> llvm/test/DebugInfo/MIR/X86/unreachable-block-call-site.mir
>> llvm/test/DebugInfo/X86/dbg-value-range.ll
>> llvm/test/DebugInfo/X86/dbg-value-regmask-clobber.ll
>> llvm/test/DebugInfo/X86/dbgcall-site-64-bit-imms.ll
>> llvm/test/DebugInfo/X86/dbgcall-site-zero-valued-imms.ll
>> llvm/test/DebugInfo/X86/loclists-dwp.ll
>> llvm/test/tools/llvm-locstats/locstats.ll
>>
>> Removed:
>>
>>
>>
>>
>> 
>> diff  --git a/clang/include/clang/Basic/CodeGenOptions.def
>> b/clang/include/clang/Basic/CodeGenOptions.def
>> index 3c8b0eeb47a5..e047054447f3 100644
>> --- a/clang/include/clang/Basic/CodeGenOptions.def
>> +++ b/clang/include/clang/Basic/CodeGenOptions.def
>> @@ -63,7 +63,6 @@ CODEGENOPT(ExperimentalNewPassManager, 1, 0) ///<
>> Enables the new, experimental
>>  CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the
>> new
>>   

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for expanding the description, including the helpful example.  I'm not 
sure, though, that this is the "right" behavior, at least not always. Worse, 
I'm not sure there is a single "right" behavior. I can easily imagine a tidy 
that matches multiple times in the same TU and tries, therefore, to apply the 
same fix multiple times, even though it wants at most one (and possibly an 
error).  The major problem is that character-level (versus AST level) edits 
simply don't compose. So, multiple edits on a file are always suspect.  Could 
you also give an example (in terms of code changes, not YAML) of why this comes 
up? As in, are we sure that the problem lies in the algorithm here, rather than 
in the phrasing of the transformation itself?

That said, based on some of your linked bugs, it sounds like your change would 
make the behavior here consistent with the behavior in another situation (when 
clang-tidy applies the edits directly rather than outputting to YAML?).  
Consistency could be a strong argument in favor of this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

Re: [clang] 4a0267e - Convert a reachable llvm_unreachable into an assert.

[Aaron Ballman via cfe-commits]

On Sat, Mar 21, 2020 at 11:31 PM David Blaikie  wrote:
>
> Why the change? this seems counter to LLVM's style which pretty consistently 
> uses unreachable rather than assert(false), so far as I know?

We're not super consistent (at least within Clang), but the rules as
I've generally understood them are to use llvm_unreachable only for
truly unreachable code and to use assert(false) when the code is
technically reachable but is a programmer mistake to have gotten
there. In this particular case, the code is very much reachable and I
spent a lot more time debugging than I should have because I was using
a release + asserts build and the semantics of llvm_unreachable made
unfortunate codegen (switching to an assert makes the issue
immediately obvious).

>
> On Tue, Mar 10, 2020 at 11:22 AM Aaron Ballman via cfe-commits 
>  wrote:
>>
>>
>> Author: Aaron Ballman
>> Date: 2020-03-10T14:22:21-04:00
>> New Revision: 4a0267e3ad8c4d47f267d7d960f127e099fb4818
>>
>> URL: 
>> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818
>> DIFF: 
>> https://github.com/llvm/llvm-project/commit/4a0267e3ad8c4d47f267d7d960f127e099fb4818.diff
>>
>> LOG: Convert a reachable llvm_unreachable into an assert.
>>
>> Added:
>>
>>
>> Modified:
>> clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>>
>> Removed:
>>
>>
>>
>> 
>> diff  --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp 
>> b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> index 01ac2bc83bb6..99e16752b51a 100644
>> --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> @@ -134,9 +134,9 @@ StringRef 
>> AnalyzerOptions::getCheckerStringOption(StringRef CheckerName,
>>  CheckerName = CheckerName.substr(0, Pos);
>>} while (!CheckerName.empty() && SearchInParents);
>>
>> -  llvm_unreachable("Unknown checker option! Did you call getChecker*Option "
>> -   "with incorrect parameters? User input must've been "
>> -   "verified by CheckerRegistry.");
>> +  assert(false && "Unknown checker option! Did you call getChecker*Option "
>> +  "with incorrect parameters? User input must've been "
>> +  "verified by CheckerRegistry.");
>>
>>return "";
>>  }
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Ping @MyDeveloperDay  , any clue about what's going on here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326



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

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Elvis Stansvik via Phabricator via cfe-commits]

estan added a comment.

It would be great to get something like this in.

In our project we're currently using a kludge of overriding the 
`RULE_LAUNCH_COMPILE` CMake variable to insert our own shell script as the 
compiler. The script strips the clang-tidy invocation inserted by CMake if the 
source path matches any of a set of regexes we have in a .clang-tidy-exclude 
file.

The original suggested solution with an exclude regex would be sufficient for 
us, but the source-centric approach described by @alexfh would also work. Just 
something would be good.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654



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