[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D144006#4656383 , @dzhidzhoev 
wrote:

> In D144006#4656371 , @aeubanks 
> wrote:
>
>> Is there any issue with mixing IR built with 
>> `LLVMContext::enableDebugTypeODRUniquing()` and without? I'm suspecting that 
>> ThinLTO is mixing Rust and Clang IR which set that differently.
>
> Apparently, it's an issue with ThinLTO that might be caused by a change in 
> MetadataLoader https://github.com/llvm/llvm-project/pull/68986.
> Is there a way to get a repro like this one 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1487910#c3 if it's not 
> too complicated?

I've shared the repro tar with your commit email address, sorry it's so big. I 
tried linking again with the revert and now it looks like the debug info 
verifier is firing instead. Do you also need a repro tar with everything built 
with the revert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

F30438826: i.ll 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

another observation, `opt` complained about some debug info due to setting 
`LLVMContext::enableDebugTypeODRUniquing()` and stripped it away, but `llc` 
happily accepted the debug info and crashed with the assertion above when 
emitting assembly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Is there any issue with mixing IR built with 
`LLVMContext::enableDebugTypeODRUniquing()` and without? I'm suspecting that 
ThinLTO is mixing Rust and Clang IR which set that differently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

`../../llvm/lib/CodeGen/AsmPrinter/DwarfFile.cpp:110: void 
llvm::DwarfFile::addScopeVariable(LexicalScope *, DbgVariable *): Assertion 
'Ret.second' failed.`

is the crash btw


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Heads up, we're seeing issues from this in Chrome again, will try to post a 
reduced repro (https://crbug.com/1500022)

The reduced repro I found crashed very far back, so I'm guessing this patch 
caused bad debug info which caused an existing issue that doesn't handle bad 
debug info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D151479: [clang] Use IgnoreParensSingleStep in more places

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe32cde6f41cd: [clang] Use IgnoreParensSingleStep in more 
places (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151479

Files:
  clang/lib/Sema/SemaInit.cpp


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
@@ -170,22 +171,9 @@
   while (true) {
 E->setType(Ty);
 E->setValueKind(VK_PRValue);
-if (isa(E) || isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else if (PredefinedExpr *PE = dyn_cast(E)) {
-  E = PE->getFunctionName();
-} else {
-  llvm_unreachable("unexpected expr in string literal init");
-}
+if (isa(E) || isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 
@@ -194,20 +182,9 @@
 static void updateGNUCompoundLiteralRValue(Expr *E) {
   while (true) {
 E->setValueKind(VK_PRValue);
-if (isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else {
-  llvm_unreachable("unexpected expr in array compound literal init");
-}
+if (isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
@@ -170,22 +171,9 @@
   while (true) {
 E->setType(Ty);
 E->setValueKind(VK_PRValue);
-if (isa(E) || isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else if (PredefinedExpr *PE = dyn_cast(E)) {
-  E = PE->getFunctionName();
-} else {
-  llvm_unreachable("unexpected expr in string literal init");
-}
+if (isa(E) || isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 
@@ -194,20 +182,9 @@
 static void updateGNUCompoundLiteralRValue(Expr *E) {
   while (true) {
 E->setValueKind(VK_PRValue);
-if (isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else {
-  llvm_unreachable("unexpected expr in array compound literal init");
-}
+if (isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks abandoned this revision.
aeubanks added a comment.

reworked into https://github.com/llvm/llvm-project/pull/69030


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149800

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

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

If people are passing `-Wthread-safety-reference`, there was clearly some value 
in the previous checks and it would be unfortunate to turn them off while 
fixing the codebase.
I'm not super familiar with flag families and if what I'm proposing is easily 
doable, but I think ideally we would keep this new functionality turned on by 
default under `-Wthread-safety-reference` and make just this new functionality 
toggleable under a subflag `-W[no-]thread-safety-reference-return`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153131

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

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D137149#4653381 , @vchuravy wrote:

> In D137149#4653308 , @aeubanks 
> wrote:
>
>> we don't currently support reusing a pipeline so I'm surprised that you're 
>> able to share/reuse pipelines without running into any issues
>
> In addition to @pchintalapudi's comment. Reuse of pipelines is something that 
> we (JuliaLang) had come to expect from old PM. This is why opt had a 
> `-run-twice` option to help flush
> out bugs that arose out of the idea that passes would only be used once. 
> @loladiro might remember those discussions.
>
> So when we ported Julia to NewPM I didn't think twice and @pchintalapudi 
> implemented our NewPM usage such that only the AnalysisManager
> would be created fresh.

Passes can store state that might break between runs (especially module passes 
since they typically only run once). I'm not saying it's impossible to reuse a 
pipeline, just that it's not tested, e.g. `-run-twice` isn't hooked up to the 
new PM. There was a discussion about this before somewhere, can't remember 
where. We'd need a lot more testing before we can claim to support reusing 
pipelines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

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

[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

This is finding lots of real issues in code, which is awesome, but could I 
request that this be put under a separate warning flag so we can toggle off 
just the new functionality and turn it on as we clean our codebase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153131

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

[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

we don't currently support reusing a pipeline so I'm surprised that you're able 
to share/reuse pipelines without running into any issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149

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

[PATCH] D154093: [clang-format] Break long string literals in C#, etc.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D154093#4648986 , @aeubanks wrote:

> In D154093#4648931 , @eaeltsin 
> wrote:
>
>> This might have another issue with Verilog -
>>
>>   < import "AAA-BBB" foo bar baz
>>   ---
>>   > import {"AAA-",
>>   > "BBB"} foo bar baz
>>
>> I wonder if Verilog allows breaking strings in `import`?
>
> From the spec:
> `dpi_spec_string ::= "DPI-C" | "DPI"`
> Seems like it can't.

Sent out https://github.com/llvm/llvm-project/pull/66951


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154093

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

[PATCH] D154093: [clang-format] Break long string literals in C#, etc.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D154093#4648931 , @eaeltsin wrote:

> This might have another issue with Verilog -
>
>   < import "AAA-BBB" foo bar baz
>   ---
>   > import {"AAA-",
>   > "BBB"} foo bar baz
>
> I wonder if Verilog allows breaking strings in `import`?

From the spec:
`dpi_spec_string ::= "DPI-C" | "DPI"`
Seems like it can't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154093

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

[PATCH] D130531: [IR] Use Min behavior for module flag "PIC Level"

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

I'm not understanding why this doesn't also apply to "PIE Level", doesn't it 
also follow the same reasoning? pic -> PIC is the same as pie -> PIE

e.g. if you merge a small PIC and large PIC file, the resulting file would only 
be guaranteed to work with a "large PIC executable" (unsure what the right term 
is) and not a "small PIC executable", so if we say it's a large PIC file, 
that's wrong since it wouldn't link into a "large PIC executable", so we have 
to conservatively say it's a small PIC file.
and s/PIC/PIE for the same argument


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130531

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

[PATCH] D127221: [Clang] Enable -print-pipeline-passes in clang.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGen/print-pipeline-passes.c:8
+// CHECK: always-inline
+// CHECK-SAME: verify
+void Foo(void) {}

jcranmer-intel wrote:
> aeubanks wrote:
> > aeubanks wrote:
> > > I believe this will fail in non-assert builds
> > since we don't run the verifier in non-assert builds
> Apparently verify is added by clang unless `-disable-llvm-verifier` is 
> present on the command line, but I've switched it to annotation-remarks 
> nevertheless.
`-disable-llvm-verifier` is added by clang in non-assert builds


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127221

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

[PATCH] D153924: [OpenMP] Allow exceptions in target regions when offloading to GPUs

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/OpenMP/amdgpu_throw_trap.cpp:4
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device %s -emit-llvm -S -Wno-openmp-target-exception -o - | 
FileCheck -check-prefix=DEVICE %s
+// RUN: %clang_cc1 -fopenmp -triple x86_64-pc-linux-gnu 
-fopenmp-is-target-device -fcxx-exceptions %s -emit-llvm -S 
-Wno-openmp-target-exception -o - | FileCheck -check-prefix=HOST %s
+// DEVICE: s_trap

thakis wrote:
> aeubanks wrote:
> > thakis wrote:
> > > This test fails if X86 isn't in `LLVM_TARGETS_TO_BUILD` and the host 
> > > system is some non-x86 system (e.g. arm64).
> > > 
> > > (This is the only test in check-clang that fails then.)
> > > 
> > > Should this test grow a `REQUIRES: x86-registered-target`? Should it use 
> > > `%itanium_abi_triple` instead of `x86_64-pc-linux-gnu`? (It seems to pass 
> > > when replacing `x86_64-pc-linux-gnu` with `%itanium_abi_triple` on my arm 
> > > mac.)
> > added x86-registered-target in 238a1ef44f4f2361205e538b3cb7ebc5ec70894d
> Is that better than `%itanium_abi_triple`?
I was worried about LLVM failing if the calculated `%itanium_abi_triple` wasn't 
supported in that build of LLVM, but TIL that clang/LLVM can handle triples it 
doesn't recognize all the way until the codegen phase. But IIUC optimizations 
can change depending on whether or not LLVM recognizes the triple so it's still 
a little inconsistent.

so yeah `%itanium_abi_triple` would probably work, but it seems susceptible to 
configuration differences


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153924

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

[PATCH] D153924: [OpenMP] Allow exceptions in target regions when offloading to GPUs

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/OpenMP/amdgpu_throw_trap.cpp:4
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device %s -emit-llvm -S -Wno-openmp-target-exception -o - | 
FileCheck -check-prefix=DEVICE %s
+// RUN: %clang_cc1 -fopenmp -triple x86_64-pc-linux-gnu 
-fopenmp-is-target-device -fcxx-exceptions %s -emit-llvm -S 
-Wno-openmp-target-exception -o - | FileCheck -check-prefix=HOST %s
+// DEVICE: s_trap

thakis wrote:
> This test fails if X86 isn't in `LLVM_TARGETS_TO_BUILD` and the host system 
> is some non-x86 system (e.g. arm64).
> 
> (This is the only test in check-clang that fails then.)
> 
> Should this test grow a `REQUIRES: x86-registered-target`? Should it use 
> `%itanium_abi_triple` instead of `x86_64-pc-linux-gnu`? (It seems to pass 
> when replacing `x86_64-pc-linux-gnu` with `%itanium_abi_triple` on my arm 
> mac.)
added x86-registered-target in 238a1ef44f4f2361205e538b3cb7ebc5ec70894d


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153924

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

[PATCH] D127221: [Clang] Enable -print-pipeline-passes in clang.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGen/print-pipeline-passes.c:8
+// CHECK: always-inline
+// CHECK-SAME: verify
+void Foo(void) {}

aeubanks wrote:
> I believe this will fail in non-assert builds
since we don't run the verifier in non-assert builds


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127221

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

[PATCH] D127221: [Clang] Enable -print-pipeline-passes in clang.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks accepted this revision.
aeubanks added a comment.

lg with one test comment




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1102
+outs() << "\n";
+return;
+  }

jcranmer-intel wrote:
> aeubanks wrote:
> > I wouldn't return here, doesn't seem right that we'll skip running the opt 
> > pipeline but continue with compilation. we should either bail out entirely  
> > of producing any output files (which would probably require code changes 
> > elsewhere), or run everything as normal, not do something weird where we 
> > don't run the optimization pipeline but still output files
> This is basically doing the same thing that `opt -print-pipeline-passes` is 
> doing: 
> https://github.com/llvm/llvm-project/blob/d1b418f55263ec48d14f220ad020d55f126cfcab/llvm/tools/opt/NewPMDriver.cpp#L500-L524.
> 
> In the case of emitting LLVM IR or bitcode, this logic is sufficient to not 
> emit any output. In the case of .s or .o files, it looks like I have to also 
> bail out of running `RunCodegenPipeline`
if skipping `RunCodegenPipeline` doesn't produce any output file, sgtm



Comment at: clang/test/CodeGen/print-pipeline-passes.c:8
+// CHECK: always-inline
+// CHECK-SAME: verify
+void Foo(void) {}

I believe this will fail in non-assert builds


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127221

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

[PATCH] D127221: [Clang] Enable -print-pipeline-passes in clang.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1102
+outs() << "\n";
+return;
+  }

I wouldn't return here, doesn't seem right that we'll skip running the opt 
pipeline but continue with compilation. we should either bail out entirely  of 
producing any output files (which would probably require code changes 
elsewhere), or run everything as normal, not do something weird where we don't 
run the optimization pipeline but still output files



Comment at: clang/test/CodeGen/print-pipeline-passes.c:8
+// CHECK: always-inline
+// CHECK-SAME: BitcodeWriterPass
+void Foo(void) {}

I wouldn't test BitcodeWriterPass, we may have a proper textual name for it at 
some point


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127221

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

[PATCH] D158474: [clang][ExtractAPI] Fix bool spelling coming from the macro definition.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/ExtractAPI/bool.c:14
+//--- input.h
+#include 
+bool Foo;

aeubanks wrote:
> clang tests should not be including system headers since that makes the tests 
> non-hermetic (this test fails in our internal builds that provide more 
> isolated test environments), could you make this not include ?
ignore my previous comment, stdbool.h is provided by clang, sorry for the 
trouble


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158474

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

[PATCH] D158474: [clang][ExtractAPI] Fix bool spelling coming from the macro definition.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/ExtractAPI/bool.c:14
+//--- input.h
+#include 
+bool Foo;

clang tests should not be including system headers since that makes the tests 
non-hermetic (this test fails in our internal builds that provide more isolated 
test environments), could you make this not include ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158474

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

[PATCH] D157518: Avoid running optimization passes in frontend test

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp:147-148
 
-// CHECK: ![[PROF_LIKELY]] = !{!"branch_weights", i32 [[UNLIKELY]], i32 
[[LIKELY]]}
-// CHECK: ![[PROF_UNLIKELY]] = !{!"branch_weights", i32 [[LIKELY]], i32 
[[UNLIKELY]]}

wenlei wrote:
> I thought this is the purpose of the test, which is no longer checked after 
> change? 
those should be covered by LowerExpectIntrinsic tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157518

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aaron.ballman wrote:
> aeubanks wrote:
> > rnk wrote:
> > > aaron.ballman wrote:
> > > > aeubanks wrote:
> > > > > rnk wrote:
> > > > > > erichkeane wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > aeubanks wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > aeubanks wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > aeubanks wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > What happens for tentative definitions where the 
> > > > > > > > > > > > > > > value isn't known? e.g.,
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > What happens if the types are similar but not the 
> > > > > > > > > > > > > > > same?
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > > > > > > > > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Should we diagnose taking the address of such an 
> > > > > > > > > > > > > > > attributed variable so users have some hope of 
> > > > > > > > > > > > > > > spotting the non-conforming situations?
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Does this attribute have impacts across 
> > > > > > > > > > > > > > > translation unit boundaries (perhaps only when 
> > > > > > > > > > > > > > > doing LTO) or only within a single TU?
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > What does this attribute do in C++ in the 
> > > > > > > > > > > > > > > presence of constructors and destructors? e.g.,
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > struct S {
> > > > > > > > > > > > > > >   S();
> > > > > > > > > > > > > > >   ~S();
> > > > > > > > > > > > > > > };
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > [[clang::unnamed_addr]] S s1, s2; // Are these 
> > > > > > > > > > > > > > > merged and there's only one ctor/dtor call?
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > globals are only mergeable if they're known to be 
> > > > > > > > > > > > > > constant and have the same value/size. this can be 
> > > > > > > > > > > > > > done at compile time only if the optimizer can see 
> > > > > > > > > > > > > > the constant values, or at link time
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > so nothing would happen in any of the cases you've 
> > > > > > > > > > > > > > given.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > but yeah that does imply that we should warn when 
> > > > > > > > > > > > > > the attribute is used on non const, non-POD 
> > > > > > > > > > > > > > globals. I'll update this patch to do that
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > as mentioned in the description, we actually do 
> > > > > > > > > > > > > > want to take the address of these globals for 
> > > > > > > > > > > > > > table-driven parsing, but we don't care about 
> > > > > > > > > > > > > > identity equality
> > > > > > > > > > > > > > globals are only mergeable if they're known to be 
> > > > > > > > > > > > > > constant and have the same value/size. this can be 
> > > > > > > > > > > > > > done at compile time only if the optimizer can see 
> > > > > > > > > > > > > > the constant values, or at link time
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > so nothing would happen in any of the cases you've 
> > > > > > > > > > > > > > given.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > A that's good to know. So I assume we *will* 
> > > > > > > > > > > > > merge these?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > struct S {
> > > > > > > > > > > > >   int i, j;
> > > > > > > > > > > > >   float f;
> > > > > > > > > > > > > };
> > > > > > > > > > > > > 
> > > > > > > > > > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > > > > > > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > > > > > > > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > but yeah that does imply that we should warn when 
> > > > > > > > > > > > > > the attribute is used on non const, non-POD 
> > > > > > > > > > > > > > globals. I'll update this patch to do that
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Thank you, I think that will be more user-friendly
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > as mentioned in 

[PATCH] D153924: [OpenMP] Allow exceptions in target regions when offloading to GPUs

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/OpenMP/amdgpu_exceptions.cpp:10
+
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device -fcxx-exceptions -fexceptions %s -emit-llvm -S 
-verify=with -Wopenmp-target-exception -analyze
+// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa 
-fopenmp-is-target-device -fcxx-exceptions -fexceptions %s -emit-llvm -S 
-verify=with -Wopenmp-target-exception -analyze

I believe tests using `-analyze` need `REQUIRES: staticanalyzer`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153924

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

rnk wrote:
> aaron.ballman wrote:
> > aeubanks wrote:
> > > rnk wrote:
> > > > erichkeane wrote:
> > > > > aeubanks wrote:
> > > > > > erichkeane wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > aeubanks wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > aeubanks wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > What happens for tentative definitions where the 
> > > > > > > > > > > > > value isn't known? e.g.,
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > 
> > > > > > > > > > > > > What happens if the types are similar but not the 
> > > > > > > > > > > > > same?
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > > > > > > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Should we diagnose taking the address of such an 
> > > > > > > > > > > > > attributed variable so users have some hope of 
> > > > > > > > > > > > > spotting the non-conforming situations?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Does this attribute have impacts across translation 
> > > > > > > > > > > > > unit boundaries (perhaps only when doing LTO) or only 
> > > > > > > > > > > > > within a single TU?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > What does this attribute do in C++ in the presence of 
> > > > > > > > > > > > > constructors and destructors? e.g.,
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > struct S {
> > > > > > > > > > > > >   S();
> > > > > > > > > > > > >   ~S();
> > > > > > > > > > > > > };
> > > > > > > > > > > > > 
> > > > > > > > > > > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged 
> > > > > > > > > > > > > and there's only one ctor/dtor call?
> > > > > > > > > > > > > ```
> > > > > > > > > > > > globals are only mergeable if they're known to be 
> > > > > > > > > > > > constant and have the same value/size. this can be done 
> > > > > > > > > > > > at compile time only if the optimizer can see the 
> > > > > > > > > > > > constant values, or at link time
> > > > > > > > > > > > 
> > > > > > > > > > > > so nothing would happen in any of the cases you've 
> > > > > > > > > > > > given.
> > > > > > > > > > > > 
> > > > > > > > > > > > but yeah that does imply that we should warn when the 
> > > > > > > > > > > > attribute is used on non const, non-POD globals. I'll 
> > > > > > > > > > > > update this patch to do that
> > > > > > > > > > > > 
> > > > > > > > > > > > as mentioned in the description, we actually do want to 
> > > > > > > > > > > > take the address of these globals for table-driven 
> > > > > > > > > > > > parsing, but we don't care about identity equality
> > > > > > > > > > > > globals are only mergeable if they're known to be 
> > > > > > > > > > > > constant and have the same value/size. this can be done 
> > > > > > > > > > > > at compile time only if the optimizer can see the 
> > > > > > > > > > > > constant values, or at link time
> > > > > > > > > > > >
> > > > > > > > > > > > so nothing would happen in any of the cases you've 
> > > > > > > > > > > > given.
> > > > > > > > > > > 
> > > > > > > > > > > A that's good to know. So I assume we *will* merge 
> > > > > > > > > > > these?
> > > > > > > > > > > 
> > > > > > > > > > > ```
> > > > > > > > > > > struct S {
> > > > > > > > > > >   int i, j;
> > > > > > > > > > >   float f;
> > > > > > > > > > > };
> > > > > > > > > > > 
> > > > > > > > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > > > > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > > > > > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > > > > > > > > ```
> > > > > > > > > > > 
> > > > > > > > > > > > but yeah that does imply that we should warn when the 
> > > > > > > > > > > > attribute is used on non const, non-POD globals. I'll 
> > > > > > > > > > > > update this patch to do that
> > > > > > > > > > > 
> > > > > > > > > > > Thank you, I think that will be more user-friendly
> > > > > > > > > > > 
> > > > > > > > > > > > as mentioned in the description, we actually do want to 
> > > > > > > > > > > > take the address of these globals for table-driven 
> > > > > > > > > > > > parsing, but we don't care about identity equality
> > > > > > > > > > > 
> > > > > > > > > > > Yeah, I still wonder if we want to diagnose just the same 
> > > > > > > > > > > -- if the address is never taken, there's not really a 
> > > > > > > > > > > way to notice the 

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

I don't have all the context here, but seems fine once the commit description 
is updated with the new spelling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D159000: Reland "[Profile] Allow online merging with debug info correlation."

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

looks fine to me but I'll let ellis lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159000

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

rnk wrote:
> erichkeane wrote:
> > aeubanks wrote:
> > > erichkeane wrote:
> > > > aeubanks wrote:
> > > > > aaron.ballman wrote:
> > > > > > aeubanks wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > aeubanks wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > What happens for tentative definitions where the value 
> > > > > > > > > > isn't known? e.g.,
> > > > > > > > > > ```
> > > > > > > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > What happens if the types are similar but not the same?
> > > > > > > > > > ```
> > > > > > > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > > > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > Should we diagnose taking the address of such an attributed 
> > > > > > > > > > variable so users have some hope of spotting the 
> > > > > > > > > > non-conforming situations?
> > > > > > > > > > 
> > > > > > > > > > Does this attribute have impacts across translation unit 
> > > > > > > > > > boundaries (perhaps only when doing LTO) or only within a 
> > > > > > > > > > single TU?
> > > > > > > > > > 
> > > > > > > > > > What does this attribute do in C++ in the presence of 
> > > > > > > > > > constructors and destructors? e.g.,
> > > > > > > > > > ```
> > > > > > > > > > struct S {
> > > > > > > > > >   S();
> > > > > > > > > >   ~S();
> > > > > > > > > > };
> > > > > > > > > > 
> > > > > > > > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and 
> > > > > > > > > > there's only one ctor/dtor call?
> > > > > > > > > > ```
> > > > > > > > > globals are only mergeable if they're known to be constant 
> > > > > > > > > and have the same value/size. this can be done at compile 
> > > > > > > > > time only if the optimizer can see the constant values, or at 
> > > > > > > > > link time
> > > > > > > > > 
> > > > > > > > > so nothing would happen in any of the cases you've given.
> > > > > > > > > 
> > > > > > > > > but yeah that does imply that we should warn when the 
> > > > > > > > > attribute is used on non const, non-POD globals. I'll update 
> > > > > > > > > this patch to do that
> > > > > > > > > 
> > > > > > > > > as mentioned in the description, we actually do want to take 
> > > > > > > > > the address of these globals for table-driven parsing, but we 
> > > > > > > > > don't care about identity equality
> > > > > > > > > globals are only mergeable if they're known to be constant 
> > > > > > > > > and have the same value/size. this can be done at compile 
> > > > > > > > > time only if the optimizer can see the constant values, or at 
> > > > > > > > > link time
> > > > > > > > >
> > > > > > > > > so nothing would happen in any of the cases you've given.
> > > > > > > > 
> > > > > > > > A that's good to know. So I assume we *will* merge these?
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > struct S {
> > > > > > > >   int i, j;
> > > > > > > >   float f;
> > > > > > > > };
> > > > > > > > 
> > > > > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > > but yeah that does imply that we should warn when the 
> > > > > > > > > attribute is used on non const, non-POD globals. I'll update 
> > > > > > > > > this patch to do that
> > > > > > > > 
> > > > > > > > Thank you, I think that will be more user-friendly
> > > > > > > > 
> > > > > > > > > as mentioned in the description, we actually do want to take 
> > > > > > > > > the address of these globals for table-driven parsing, but we 
> > > > > > > > > don't care about identity equality
> > > > > > > > 
> > > > > > > > Yeah, I still wonder if we want to diagnose just the same -- if 
> > > > > > > > the address is never taken, there's not really a way to notice 
> > > > > > > > the optimization, but if the address is taken, you basically 
> > > > > > > > get UB (and I think we should explicitly document it as such). 
> > > > > > > > Given how easy it is to accidentally take the address of 
> > > > > > > > something (like via a reference in C++), I think we should warn 
> > > > > > > > by default, but still have a warning group for folks who want 
> > > > > > > > to live life dangerously.
> > > > > > > > > globals are only mergeable if they're known to be constant 
> > > > > > > > > and have the same value/size. this can be done at compile 
> > > > > > > > > time only if the optimizer can see the constant values, or at 
> > > > > > > > > link time
> > > > > > > > 

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D61670#4607313 , @mstorsjo wrote:

>> I expected the answer would be "yes", so I said "lgtm" and then phrased my 
>> question very awkwardly.
>
> Ah, thanks for the clarification!
>
> Any opinion on the name, `-fno-autoimport` vs `-fno-auto-import`, given the 
> existing linker option `--disable-auto-import`?

Is there an official name, "auto-import"/"autoimport"/"auto import"? Since cc1 
flags are changeable and this hasn't landed yet, I think it'd be nice to 
standardize before we commit on a stable name.
If there isn't already a name, I like the dash in "auto-import".

In D61670#4604554 , @rnk wrote:

> In D61670#4604486 , @mstorsjo wrote:
>
>> In D61670#4604145 , @rnk wrote:
>>
>>> cc +@aeubanks @jyknight to consider using the code model for this purpose
>>
>> Hmm, I don't quite understand this comment; do you suggest that we after all 
>> should use the code model for controlling the use of `.refptr` stubs too - 
>> didn't we conclude earlier that it isn't a great fit for that?
>
> Sorry, let me elaborate. Since the first round of review, Arthur and James 
> have been looking at x86 code models, in particular the medium code model. I 
> guess what I really want to ask is, Arthur and James, do you agree with our 
> decision that the code model should not control the formation of these COFF 
> refptr stubs?
>
> I expected the answer would be "yes", so I said "lgtm" and then phrased my 
> question very awkwardly.

Yeah, at least with my understanding code models shouldn't change whether or 
not things are dso_local or not. They should only affect the instruction 
sequence we use to access globals/functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61670

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

erichkeane wrote:
> aeubanks wrote:
> > aaron.ballman wrote:
> > > aeubanks wrote:
> > > > aaron.ballman wrote:
> > > > > aeubanks wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > What happens for tentative definitions where the value isn't 
> > > > > > > known? e.g.,
> > > > > > > ```
> > > > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > > > ```
> > > > > > > 
> > > > > > > What happens if the types are similar but not the same?
> > > > > > > ```
> > > > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > > > ```
> > > > > > > 
> > > > > > > Should we diagnose taking the address of such an attributed 
> > > > > > > variable so users have some hope of spotting the non-conforming 
> > > > > > > situations?
> > > > > > > 
> > > > > > > Does this attribute have impacts across translation unit 
> > > > > > > boundaries (perhaps only when doing LTO) or only within a single 
> > > > > > > TU?
> > > > > > > 
> > > > > > > What does this attribute do in C++ in the presence of 
> > > > > > > constructors and destructors? e.g.,
> > > > > > > ```
> > > > > > > struct S {
> > > > > > >   S();
> > > > > > >   ~S();
> > > > > > > };
> > > > > > > 
> > > > > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's 
> > > > > > > only one ctor/dtor call?
> > > > > > > ```
> > > > > > globals are only mergeable if they're known to be constant and have 
> > > > > > the same value/size. this can be done at compile time only if the 
> > > > > > optimizer can see the constant values, or at link time
> > > > > > 
> > > > > > so nothing would happen in any of the cases you've given.
> > > > > > 
> > > > > > but yeah that does imply that we should warn when the attribute is 
> > > > > > used on non const, non-POD globals. I'll update this patch to do 
> > > > > > that
> > > > > > 
> > > > > > as mentioned in the description, we actually do want to take the 
> > > > > > address of these globals for table-driven parsing, but we don't 
> > > > > > care about identity equality
> > > > > > globals are only mergeable if they're known to be constant and have 
> > > > > > the same value/size. this can be done at compile time only if the 
> > > > > > optimizer can see the constant values, or at link time
> > > > > >
> > > > > > so nothing would happen in any of the cases you've given.
> > > > > 
> > > > > A that's good to know. So I assume we *will* merge these?
> > > > > 
> > > > > ```
> > > > > struct S {
> > > > >   int i, j;
> > > > >   float f;
> > > > > };
> > > > > 
> > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > > ```
> > > > > 
> > > > > > but yeah that does imply that we should warn when the attribute is 
> > > > > > used on non const, non-POD globals. I'll update this patch to do 
> > > > > > that
> > > > > 
> > > > > Thank you, I think that will be more user-friendly
> > > > > 
> > > > > > as mentioned in the description, we actually do want to take the 
> > > > > > address of these globals for table-driven parsing, but we don't 
> > > > > > care about identity equality
> > > > > 
> > > > > Yeah, I still wonder if we want to diagnose just the same -- if the 
> > > > > address is never taken, there's not really a way to notice the 
> > > > > optimization, but if the address is taken, you basically get UB (and 
> > > > > I think we should explicitly document it as such). Given how easy it 
> > > > > is to accidentally take the address of something (like via a 
> > > > > reference in C++), I think we should warn by default, but still have 
> > > > > a warning group for folks who want to live life dangerously.
> > > > > > globals are only mergeable if they're known to be constant and have 
> > > > > > the same value/size. this can be done at compile time only if the 
> > > > > > optimizer can see the constant values, or at link time
> > > > > >
> > > > > > so nothing would happen in any of the cases you've given.
> > > > > 
> > > > > A that's good to know. So I assume we *will* merge these?
> > > > > 
> > > > > ```
> > > > > struct S {
> > > > >   int i, j;
> > > > >   float f;
> > > > > };
> > > > > 
> > > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > > > [[clang::unnamed_addr]] const S s3 = s2;
> > > > > ```
> > > > yeah those are merged even just by clang
> > > > 
> > > > ```
> > > > struct S {
> > > >   int i, j;
> > > >   float f;
> > > > };
> > > > 
> > > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > 

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aaron.ballman wrote:
> aeubanks wrote:
> > aaron.ballman wrote:
> > > aeubanks wrote:
> > > > aaron.ballman wrote:
> > > > > What happens for tentative definitions where the value isn't known? 
> > > > > e.g.,
> > > > > ```
> > > > > [[clang::unnamed_addr]] int i1, i2;
> > > > > ```
> > > > > 
> > > > > What happens if the types are similar but not the same?
> > > > > ```
> > > > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > > > ```
> > > > > 
> > > > > Should we diagnose taking the address of such an attributed variable 
> > > > > so users have some hope of spotting the non-conforming situations?
> > > > > 
> > > > > Does this attribute have impacts across translation unit boundaries 
> > > > > (perhaps only when doing LTO) or only within a single TU?
> > > > > 
> > > > > What does this attribute do in C++ in the presence of constructors 
> > > > > and destructors? e.g.,
> > > > > ```
> > > > > struct S {
> > > > >   S();
> > > > >   ~S();
> > > > > };
> > > > > 
> > > > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's 
> > > > > only one ctor/dtor call?
> > > > > ```
> > > > globals are only mergeable if they're known to be constant and have the 
> > > > same value/size. this can be done at compile time only if the optimizer 
> > > > can see the constant values, or at link time
> > > > 
> > > > so nothing would happen in any of the cases you've given.
> > > > 
> > > > but yeah that does imply that we should warn when the attribute is used 
> > > > on non const, non-POD globals. I'll update this patch to do that
> > > > 
> > > > as mentioned in the description, we actually do want to take the 
> > > > address of these globals for table-driven parsing, but we don't care 
> > > > about identity equality
> > > > globals are only mergeable if they're known to be constant and have the 
> > > > same value/size. this can be done at compile time only if the optimizer 
> > > > can see the constant values, or at link time
> > > >
> > > > so nothing would happen in any of the cases you've given.
> > > 
> > > A that's good to know. So I assume we *will* merge these?
> > > 
> > > ```
> > > struct S {
> > >   int i, j;
> > >   float f;
> > > };
> > > 
> > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > [[clang::unnamed_addr]] const S s3 = s2;
> > > ```
> > > 
> > > > but yeah that does imply that we should warn when the attribute is used 
> > > > on non const, non-POD globals. I'll update this patch to do that
> > > 
> > > Thank you, I think that will be more user-friendly
> > > 
> > > > as mentioned in the description, we actually do want to take the 
> > > > address of these globals for table-driven parsing, but we don't care 
> > > > about identity equality
> > > 
> > > Yeah, I still wonder if we want to diagnose just the same -- if the 
> > > address is never taken, there's not really a way to notice the 
> > > optimization, but if the address is taken, you basically get UB (and I 
> > > think we should explicitly document it as such). Given how easy it is to 
> > > accidentally take the address of something (like via a reference in C++), 
> > > I think we should warn by default, but still have a warning group for 
> > > folks who want to live life dangerously.
> > > > globals are only mergeable if they're known to be constant and have the 
> > > > same value/size. this can be done at compile time only if the optimizer 
> > > > can see the constant values, or at link time
> > > >
> > > > so nothing would happen in any of the cases you've given.
> > > 
> > > A that's good to know. So I assume we *will* merge these?
> > > 
> > > ```
> > > struct S {
> > >   int i, j;
> > >   float f;
> > > };
> > > 
> > > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > > [[clang::unnamed_addr]] const S s3 = s2;
> > > ```
> > yeah those are merged even just by clang
> > 
> > ```
> > struct S {
> >   int i, j;
> >   float f;
> > };
> > 
> > [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> > [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> > [[clang::unnamed_addr]] const S s3 = s2;
> > 
> > const void * f1() {
> >   return 
> > }
> > 
> > const void * f2() {
> >   return 
> > }
> > 
> > const void * f3() {
> >   return 
> > }
> > 
> > $ ./build/rel/bin/clang++ -S -emit-llvm -o - -O2 /tmp/a.cc
> > ```
> > > 
> > > > but yeah that does imply that we should warn when the attribute is used 
> > > > on non const, non-POD globals. I'll update this patch to do that
> > > 
> > > Thank you, I think that will 

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 552889.
aeubanks edited the summary of this revision.
aeubanks added a comment.

add warning


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/unnamed-addr.c
  clang/test/SemaCXX/unnamed-addr.cpp

Index: clang/test/SemaCXX/unnamed-addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/unnamed-addr.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Foo {
+  int a;
+};
+
+struct Bar {
+  int a;
+  Bar();
+};
+
+[[clang::unnamed_addr]] const int i = 8;
+
+[[clang::unnamed_addr]] int i2 = 8; // expected-warning{{unnamed_addr should only be used on const POD (plain old data) globals}}
+
+[[clang::unnamed_addr]] const Foo j = {2};
+
+[[clang::unnamed_addr]] Foo j2 = {2}; // expected-warning{{unnamed_addr should only be used on const POD (plain old data) globals}}
+
+[[clang::unnamed_addr]] const Bar k; // expected-warning{{unnamed_addr should only be used on const POD (plain old data) globals}}
Index: clang/test/CodeGen/unnamed-addr.c
===
--- /dev/null
+++ clang/test/CodeGen/unnamed-addr.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O1 -emit-llvm -o - -disable-llvm-passes %s | FileCheck %s
+
+// CHECK: @i = unnamed_addr constant i32 8
+
+[[clang::unnamed_addr]] const int i = 8;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14519,6 +14519,13 @@
   PragmaClangRelroSection.PragmaLocation));
   }
 
+  if (VD->hasGlobalStorage() && VD->isThisDeclarationADefinition() &&
+  VD->hasAttr() &&
+  (!VD->getType().isPODType(getASTContext()) ||
+   !VD->getType().isConstQualified())) {
+Diag(VD->getLocation(), diag::warn_attribute_unnamed_addr);
+  }
+
   if (auto *DD = dyn_cast(ThisDecl)) {
 for (auto *BD : DD->bindings()) {
   FinalizeDeclaration(BD);
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -5255,6 +5255,9 @@
   GV->setConstant(true);
   }
 
+  if (D->hasAttr())
+GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
   CharUnits AlignVal = getContext().getDeclAlign(D);
   // Check for alignment specifed in an 'omp allocate' directive.
   if (std::optional AlignValFromAllocate =
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3587,6 +3587,9 @@
   "weakref declaration of %0 must be in a global context">;
 def err_attribute_weakref_without_alias : Error<
   "weakref declaration of %0 must also have an alias attribute">;
+def warn_attribute_unnamed_addr : Warning<
+  "unnamed_addr should only be used on const POD (plain old data) globals">,
+  InGroup>;
 def err_alias_not_supported_on_darwin : Error <
   "aliases are not supported on darwin">;
 def warn_attribute_wrong_decl_type_str : Warning<
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -1408,6 +1408,24 @@
   }];
 }
 
+def UnnamedAddrDocs : Documentation {
+  let Category = DocCatField;
+  let Content = [{
+The ``clang::unnamed_addr`` attribute specifies that the address of a global is
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+
+Example usage:
+
+.. code-block:: c
+
+  // i and j may have the same address.
+  [[clang::unnamed_addr]] const int i = 42;
+  [[clang::unnamed_addr]] const int j = 42;
+  }];
+}
+
 def ObjCRequiresSuperDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -1794,6 +1794,13 @@
   let SimpleHandler = 1;
 }
 
+def UnnamedAddr : InheritableAttr {
+  let Spellings = [Clang<"unnamed_addr">];
+  let Subjects = SubjectList<[GlobalVar], ErrorDiag>;
+  let Documentation = [UnnamedAddrDocs];
+  let SimpleHandler = 1;
+}
+
 def ReturnsTwice : InheritableAttr {
   let Spellings = [GCC<"returns_twice">];
   let Subjects = SubjectList<[Function]>;

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

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

reverted in 9b6b6bbc9127d604881cdc9dcea0e7c74ef821fd 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

aeubanks wrote:
> ellis wrote:
> > aeubanks wrote:
> > > aeubanks wrote:
> > > > we're running into this error message even though we don't have debug 
> > > > info correlation turned on, is that expected in some configurations? 
> > > > I'm still trying to understand how we could end up in this code path 
> > > > without debug info correlation
> > > > 
> > > > https://crbug.com/1473935
> > > also it seems like this code path isn't tested?
> > It's very strange that you are hitting this code path because I didn't 
> > think it was possible. Were you ever able to reproduce locally?
> no, it's only showed up on some bots (both mac and linux), I'm still trying 
> to understand what's going on
managed to repro having no `DataSize` (`NumData` now).

```
$ cat /tmp/a.cc
int main() {}
$ cat /tmp/funlist
[clang]
default:skip
$ clang++ -O1 -fprofile-instr-generate -fcoverage-mapping -o /tmp/a 
-fuse-ld=lld /tmp/z.cc -fprofile-list=/tmp/funlist
$ export LLVM_PROFILE_FILE='a%m.profraw'
$ /tmp/a
$ /tmp/a
LLVM Profile Error: Missing profile data section.
LLVM Profile Error: Invalid profile data to merge
LLVM Profile Error: Profile Merging of file a18405209413954990729_0.profraw 
failed: Success
LLVM Profile Error: Failed to write file "a18405209413954990729_0.profraw": 
Success
```

basically if nothing is instrumented we'll hit this

will revert


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aaron.ballman wrote:
> aeubanks wrote:
> > aaron.ballman wrote:
> > > What happens for tentative definitions where the value isn't known? e.g.,
> > > ```
> > > [[clang::unnamed_addr]] int i1, i2;
> > > ```
> > > 
> > > What happens if the types are similar but not the same?
> > > ```
> > > [[clang::unnamed_addr]] signed int i1 = 32;
> > > [[clang::unnamed_addr]] unsigned int i2 = 32;
> > > ```
> > > 
> > > Should we diagnose taking the address of such an attributed variable so 
> > > users have some hope of spotting the non-conforming situations?
> > > 
> > > Does this attribute have impacts across translation unit boundaries 
> > > (perhaps only when doing LTO) or only within a single TU?
> > > 
> > > What does this attribute do in C++ in the presence of constructors and 
> > > destructors? e.g.,
> > > ```
> > > struct S {
> > >   S();
> > >   ~S();
> > > };
> > > 
> > > [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's only 
> > > one ctor/dtor call?
> > > ```
> > globals are only mergeable if they're known to be constant and have the 
> > same value/size. this can be done at compile time only if the optimizer can 
> > see the constant values, or at link time
> > 
> > so nothing would happen in any of the cases you've given.
> > 
> > but yeah that does imply that we should warn when the attribute is used on 
> > non const, non-POD globals. I'll update this patch to do that
> > 
> > as mentioned in the description, we actually do want to take the address of 
> > these globals for table-driven parsing, but we don't care about identity 
> > equality
> > globals are only mergeable if they're known to be constant and have the 
> > same value/size. this can be done at compile time only if the optimizer can 
> > see the constant values, or at link time
> >
> > so nothing would happen in any of the cases you've given.
> 
> A that's good to know. So I assume we *will* merge these?
> 
> ```
> struct S {
>   int i, j;
>   float f;
> };
> 
> [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s3 = s2;
> ```
> 
> > but yeah that does imply that we should warn when the attribute is used on 
> > non const, non-POD globals. I'll update this patch to do that
> 
> Thank you, I think that will be more user-friendly
> 
> > as mentioned in the description, we actually do want to take the address of 
> > these globals for table-driven parsing, but we don't care about identity 
> > equality
> 
> Yeah, I still wonder if we want to diagnose just the same -- if the address 
> is never taken, there's not really a way to notice the optimization, but if 
> the address is taken, you basically get UB (and I think we should explicitly 
> document it as such). Given how easy it is to accidentally take the address 
> of something (like via a reference in C++), I think we should warn by 
> default, but still have a warning group for folks who want to live life 
> dangerously.
> > globals are only mergeable if they're known to be constant and have the 
> > same value/size. this can be done at compile time only if the optimizer can 
> > see the constant values, or at link time
> >
> > so nothing would happen in any of the cases you've given.
> 
> A that's good to know. So I assume we *will* merge these?
> 
> ```
> struct S {
>   int i, j;
>   float f;
> };
> 
> [[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
> [[clang::unnamed_addr]] const S s3 = s2;
> ```
yeah those are merged even just by clang

```
struct S {
  int i, j;
  float f;
};

[[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s3 = s2;

const void * f1() {
  return 
}

const void * f2() {
  return 
}

const void * f3() {
  return 
}

$ ./build/rel/bin/clang++ -S -emit-llvm -o - -O2 /tmp/a.cc
```
> 
> > but yeah that does imply that we should warn when the attribute is used on 
> > non const, non-POD globals. I'll update this patch to do that
> 
> Thank you, I think that will be more user-friendly
> 
> > as mentioned in the description, we actually do want to take the address of 
> > these globals for table-driven parsing, but we don't care about identity 
> > equality
> 
> Yeah, I still wonder if we want to diagnose just the same -- if the address 
> is never taken, there's not really a way to notice the optimization, but if 
> the address is taken, you basically get UB (and I think we should explicitly 
> document it as such). Given how easy it is to accidentally take the address 
> of something (like via a reference in 

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

ellis wrote:
> aeubanks wrote:
> > aeubanks wrote:
> > > we're running into this error message even though we don't have debug 
> > > info correlation turned on, is that expected in some configurations? I'm 
> > > still trying to understand how we could end up in this code path without 
> > > debug info correlation
> > > 
> > > https://crbug.com/1473935
> > also it seems like this code path isn't tested?
> It's very strange that you are hitting this code path because I didn't think 
> it was possible. Were you ever able to reproduce locally?
no, it's only showed up on some bots (both mac and linux), I'm still trying to 
understand what's going on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D157879: [clang] Report missing designated initializers in C++

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D157879#4606531 , @aaron.ballman 
wrote:

> In D157879#4604288 , @phosek wrote:
>
>> Note that there's an ongoing discussion on the GCC bug 
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868 whether 
>> `-Wmissing-field-initializers` should apply to both C and C++ or just C.
>
> Thank you for posting this! I'm with @jwakely on this: 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868#c3 -- the warning is for 
> people who want to be told they've not explicitly specified all of the 
> initializers, it is not for telling you "this hasn't been initialized". I 
> think the desire makes as much sense in C++ as it does in C, but this is more 
> of a style warning than a correctness warning.
>
> In D157879#4604471 , @ianloic wrote:
>
>> In D157879#4604233 , @aeubanks 
>> wrote:
>>
>>> ah I thought this was in `-Wall` but it's not
>>>
>>> the struct is
>>>
>>>   struct Foo {
>>> const void* buffer;
>>> uint32_t capacity;
>>> uint32_t reserved;
>>>   };
>>>
>>> where `reserved` isn't explicitly initialized. that seems like reasonable 
>>> code, but I suppose we can just explicitly initialize `reserved` here
>>
>> FWIW, if this is the specific piece of code that led me to this 
>> conversation, that declaration is in a header which at least in theory is 
>> supposed to remain compatible with C. It's inside `extern "C" { ... }` even. 
>> We can't add explicit initialize `reserved` and keep the definition C 
>> comaptible.
>
> Ouch, yeah, that does make it a challenge. But why is 
> `-Wmissing-field-initializers` enabled in the first place for the code? It 
> seems like there are fields you *don't* want explicit initialization for, so 
> perhaps the answer is to disable the diagnostic for those types?

It's an unfortunate configuration. We saw this in Chrome where some projects 
were manually setting `-Wmissing-field-initializers`, but when building for 
Fuchsia, some Fuchsia system headers had this code. We can just suppress this 
for all Fuchsia builds, I think that's fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157879

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+

aaron.ballman wrote:
> What happens for tentative definitions where the value isn't known? e.g.,
> ```
> [[clang::unnamed_addr]] int i1, i2;
> ```
> 
> What happens if the types are similar but not the same?
> ```
> [[clang::unnamed_addr]] signed int i1 = 32;
> [[clang::unnamed_addr]] unsigned int i2 = 32;
> ```
> 
> Should we diagnose taking the address of such an attributed variable so users 
> have some hope of spotting the non-conforming situations?
> 
> Does this attribute have impacts across translation unit boundaries (perhaps 
> only when doing LTO) or only within a single TU?
> 
> What does this attribute do in C++ in the presence of constructors and 
> destructors? e.g.,
> ```
> struct S {
>   S();
>   ~S();
> };
> 
> [[clang::unnamed_addr]] S s1, s2; // Are these merged and there's only one 
> ctor/dtor call?
> ```
globals are only mergeable if they're known to be constant and have the same 
value/size. this can be done at compile time only if the optimizer can see the 
constant values, or at link time

so nothing would happen in any of the cases you've given.

but yeah that does imply that we should warn when the attribute is used on non 
const, non-POD globals. I'll update this patch to do that

as mentioned in the description, we actually do want to take the address of 
these globals for table-driven parsing, but we don't care about identity 
equality


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223

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

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

aeubanks wrote:
> we're running into this error message even though we don't have debug info 
> correlation turned on, is that expected in some configurations? I'm still 
> trying to understand how we could end up in this code path without debug info 
> correlation
> 
> https://crbug.com/1473935
also it seems like this code path isn't tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:146
+if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
+  PROF_ERR("%s\n", "Missing profile data section.");
+  return 1;

we're running into this error message even though we don't have debug info 
correlation turned on, is that expected in some configurations? I'm still 
trying to understand how we could end up in this code path without debug info 
correlation

https://crbug.com/1473935


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632

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

[PATCH] D157879: [clang] Report missing designated initializers in C++

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

ah I thought this was in `-Wall` but it's not

the struct is

  struct Foo {
const void* buffer;
uint32_t capacity;
uint32_t reserved;
  };

where `reserved` isn't explicitly initialized. that seems like reasonable code, 
but I suppose we can just explicitly initialize `reserved` here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157879

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

[PATCH] D157879: [clang] Report missing designated initializers in C++

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

is there any way to suppress this for a specific field? (I believe) I'm seeing 
user code where a field is intentionally not being initialized


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157879

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

[PATCH] D156799: Update generic scheduling to use A510 scheduling model

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/test/CodeGen/AArch64/stack-guard-sysreg.ll:1
-; RUN: split-file %s %t
 ; RUN: cat %t/main.ll %t/a.ll > %t/a2.ll
 ; RUN: cat %t/main.ll %t/b.ll > %t/b2.ll

this test is broken, without the `split-file` the other files won't exist in a 
clean build, please fix or revert if it takes time to fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156799

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

merging doesn't have to be done at link time, the optimizer can also do it. so 
I'd rule out any names with `icf` since that's specifically the name of a 
linker feature

`no_unique_address` popped into my mind but that's already taken :). that's 
exactly what this is though. maybe `no_unique_identity`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

and naming can be bikeshed. `no_addrsig` follows 
https://llvm.org/docs/Extensions.html#sht-llvm-addrsig-section-address-significance-table,
 `unnamed_addr` follows the IR attribute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

unsure if this should have an RFC or not

will add release notes if this is ok to go ahead with


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This simply marks the global with the `unnamed_addr` IR attribute so it
be merged with other globals. This can break C++ pointer identity.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158223

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/unnamed-addr.c


Index: clang/test/CodeGen/unnamed-addr.c
===
--- /dev/null
+++ clang/test/CodeGen/unnamed-addr.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O1 -emit-llvm -o - 
-disable-llvm-passes %s | FileCheck %s
+
+// CHECK: @i = unnamed_addr constant i32 8
+
+[[clang::unnamed_addr]] const int i = 8;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -5254,6 +5254,9 @@
   GV->setConstant(true);
   }
 
+  if (D->hasAttr())
+GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
   CharUnits AlignVal = getContext().getDeclAlign(D);
   // Check for alignment specifed in an 'omp allocate' directive.
   if (std::optional AlignValFromAllocate =
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -1408,6 +1408,24 @@
   }];
 }
 
+def UnnamedAddrDocs : Documentation {
+  let Category = DocCatField;
+  let Content = [{
+The ``clang::unnamed_addr`` attribute specifies that the address of a global is
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+
+Example usage:
+
+.. code-block:: c
+
+  // i and j may have the same address.
+  [[clang::unnamed_addr]] const int i = 42;
+  [[clang::unnamed_addr]] const int j = 42;
+  }];
+}
+
 def ObjCRequiresSuperDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -1794,6 +1794,13 @@
   let SimpleHandler = 1;
 }
 
+def UnnamedAddr : InheritableAttr {
+  let Spellings = [Clang<"unnamed_addr">];
+  let Subjects = SubjectList<[GlobalVar], ErrorDiag>;
+  let Documentation = [UnnamedAddrDocs];
+  let SimpleHandler = 1;
+}
+
 def ReturnsTwice : InheritableAttr {
   let Spellings = [GCC<"returns_twice">];
   let Subjects = SubjectList<[Function]>;


Index: clang/test/CodeGen/unnamed-addr.c
===
--- /dev/null
+++ clang/test/CodeGen/unnamed-addr.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O1 -emit-llvm -o - -disable-llvm-passes %s | FileCheck %s
+
+// CHECK: @i = unnamed_addr constant i32 8
+
+[[clang::unnamed_addr]] const int i = 8;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -5254,6 +5254,9 @@
   GV->setConstant(true);
   }
 
+  if (D->hasAttr())
+GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
   CharUnits AlignVal = getContext().getDeclAlign(D);
   // Check for alignment specifed in an 'omp allocate' directive.
   if (std::optional AlignValFromAllocate =
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -1408,6 +1408,24 @@
   }];
 }
 
+def UnnamedAddrDocs : Documentation {
+  let Category = DocCatField;
+  let Content = [{
+The ``clang::unnamed_addr`` attribute specifies that the address of a global is
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+
+Example usage:
+
+.. code-block:: c
+
+  // i and j may have the same address.
+  [[clang::unnamed_addr]] const int i = 42;
+  [[clang::unnamed_addr]] const int j = 42;
+  }];
+}
+
 def ObjCRequiresSuperDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -1794,6 +1794,13 @@
   let SimpleHandler = 1;
 }
 
+def UnnamedAddr : InheritableAttr {
+  let Spellings = [Clang<"unnamed_addr">];
+  let Subjects = 

[PATCH] D157518: Avoid running optimization passes in frontend test

[Arthur Eubanks via Phabricator via cfe-commits]

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

seems fine if nobody else objects


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157518

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

[PATCH] D155997: [Phase Ordering] Don't speculate in SimplifyCFG before PGO annotation

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

can we try not gating this on PGO as suggested? minimizing differences between 
pipelines is nice, and as mentioned it'll help with other cases




Comment at: llvm/test/Transforms/PhaseOrdering/simplifycfg-speculate-blocks.ll:1
+;; Check that SimplifyCFG does not attempt speculation until after PGO is
+;; annotated in the IR, and then does not perform it when unprofitable.

nikic wrote:
> aeubanks wrote:
> > hmm typically these phase ordering tests use 
> > `llvm/utils/update_test_checks.py`, but that doesn't support the 
> > llvm-profdata RUN line. I think a non-update_test_checks test is probably 
> > fine for this, @nikic does that make sense?
> > 
> > 
> I thought UTC supports unrecognized commands (by just executing them)... If 
> not, a non-UTC test is fine.
`update_test_checks.py` doesn't handle `%t` (maybe that's an improvement we 
could make to the script)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997

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

[PATCH] D155997: [Phase Ordering] Don't speculate in SimplifyCFG before PGO annotation

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/test/Transforms/PhaseOrdering/simplifycfg-speculate-blocks.ll:1
+;; Check that SimplifyCFG does not attempt speculation until after PGO is
+;; annotated in the IR, and then does not perform it when unprofitable.

hmm typically these phase ordering tests use 
`llvm/utils/update_test_checks.py`, but that doesn't support the llvm-profdata 
RUN line. I think a non-update_test_checks test is probably fine for this, 
@nikic does that make sense?





Comment at: 
llvm/test/Transforms/PhaseOrdering/simplifycfg-speculate-blocks.ll:22
+; RUN: opt < %s -passes='thinlto-pre-link' 
-pgo-kind=pgo-sample-use-pipeline 
-profile-file=%S/Inputs/simplifycfg-speculate-blocks.sampleprof -S | FileCheck 
%s --check-prefixes=NO,SAMPLE
+; RUN: opt < %s -passes='thinlto-pre-link' 
-pgo-kind=pgo-sample-use-pipeline 
-profile-file=%S/Inputs/simplifycfg-speculate-blocks.sampleprof -S | FileCheck 
%s --check-prefixes=NO,SAMPLE
+

`lto-pre-link`



Comment at: 
llvm/test/Transforms/PhaseOrdering/simplifycfg-speculate-blocks.ll:83
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}

the debug info is necessary for sample profile to work I presume?

I think some of this can still be simplified, like `PIC Level`, etc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997

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

[PATCH] D155997: [Phase Ordering] Don't speculate in SimplifyCFG before PGO annotation

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/test/Transforms/SimplifyCFG/fold-branch-to-common-dest.ll:3
 ; RUN: opt < %s -S -passes=simplifycfg 
-simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 | 
FileCheck %s
+; RUN: opt < %s -S -passes='simplifycfg' 
-simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 | 
FileCheck %s --check-prefix=NOSPECULATE
 

tejohnson wrote:
> aeubanks wrote:
> > these checks won't work for a `update_test_checks.py` test. should add 
> > tests like the one added in https://reviews.llvm.org/D153391
> Can you clarify - are you saying remove the changes from this test and add a 
> new test for this change like 
> llvm/test/Transforms/SimplifyCFG/speculate-blocks.ll, or change the checks 
> here?
revert the changes here (`update_test_checks.py` tests shouldn't have manually 
added CHECK lines) and add new function(s) to 
`llvm/test/Transforms/SimplifyCFG/speculate-blocks.ll` that show the difference 
between `speculate-blocks` and `no-speculate-blocks`



Comment at: llvm/test/Transforms/SimplifyCFG/pipeline-delay-speculation-pgo.ll:1
+;; Test that the pipelines delay simplify CFG speculation until after
+;; pgo annotation.

tejohnson wrote:
> aeubanks wrote:
> > do you have a phase ordering test instead?
> Can you clarify what you are referring to?
`llvm/test/Transforms/PhaseOrdering` has IR tests that get run through an 
entire pipeline (e.g. `thinlto-pre-link`) to verify some property of the 
entire pipeline. ideally you can get some somewhat-reduced IR that exhibits the 
problem you were seeing before the change and show that it improves with the 
pipeline change. you'll need a profile as part of that test I suppose


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997

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

[PATCH] D155997: [Phase Ordering] Don't speculate in SimplifyCFG before PGO annotation

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

the pipeline change and simplifycfg change should be split into two changes




Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:277
 
+static cl::opt AlwaysAllowSimplifyCFGSpeculation(
+"always-allow-simplify-cfg-speculation", cl::init(false), cl::Hidden,

can we just drop the flag and make this change?



Comment at: llvm/test/Transforms/SimplifyCFG/fold-branch-to-common-dest.ll:3
 ; RUN: opt < %s -S -passes=simplifycfg 
-simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 | 
FileCheck %s
+; RUN: opt < %s -S -passes='simplifycfg' 
-simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 | 
FileCheck %s --check-prefix=NOSPECULATE
 

these checks won't work for a `update_test_checks.py` test. should add tests 
like the one added in https://reviews.llvm.org/D153391



Comment at: llvm/test/Transforms/SimplifyCFG/pipeline-delay-speculation-pgo.ll:1
+;; Test that the pipelines delay simplify CFG speculation until after
+;; pgo annotation.

do you have a phase ordering test instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997

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

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3594
+TrapCall->addFnAttr(A);
+  }
+  TrapCall->setDoesNotReturn();

oskarwirga wrote:
> vitalybuka wrote:
> > wouldn't be you issues solved with
> > TrapCall->setCannotMerge() here?
> ubsantrap gets lowered to an instruction in MIR which then gets merged later. 
> This was the only way I was able to create unique instruction per check. 
isn't that a `nomerge` bug that should get fixed instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

@nikic could you try running this over the rust tests again?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 537871.
aeubanks added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

Files:
  clang/test/Headers/mm_malloc.c
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-pgo-preinline.ll
  llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
  llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll

Index: llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
===
--- llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
+++ llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
@@ -16,10 +16,13 @@
 
 define void @caller1(i1 %c, ptr align 1 %ptr) {
 ; ASSUMPTIONS-OFF-LABEL: @caller1(
-; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-OFF:   false1:
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-OFF:   common.ret:
-; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR:%.*]], align 8
+; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
@@ -27,15 +30,15 @@
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:ret void
-; ASSUMPTIONS-OFF:   false2:
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ;
 ; ASSUMPTIONS-ON-LABEL: @caller1(
-; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-ON:   false1:
+; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-ON:   common.ret:
-; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR:%.*]], i64 8) ]
+; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR]], i64 8) ]
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
@@ -44,9 +47,6 @@
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:ret void
-; ASSUMPTIONS-ON:   false2:
-; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ;
   br i1 %c, label %true1, label %false1
 
Index: llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
===
--- llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
+++ llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
@@ -1,11 +1,28 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -O3 -S < %s | FileCheck %s
 
+; FIXME: see the following issues for background
+; https://github.com/llvm/llvm-project/issues/51631
+; https://github.com/llvm/llvm-project/issues/51744
+; 

[PATCH] D154253: [clang] detect integer overflow through temporary values

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:15215
   Exprs.append(Construct->arg_begin(), Construct->arg_end());
+else if (auto Temporary = dyn_cast(E))
+  Exprs.push_back(Temporary->getSubExpr());

nit: `clang::` isn't necessary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154253

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

[PATCH] D153468: [clang][LTO] Add flag to run verifier after every pass

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGda7f212f4a7a: [clang][LTO] Add flag to run verifier after 
every pass (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153468

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/verify-each.c
  llvm/include/llvm/LTO/Config.h
  llvm/lib/LTO/LTOBackend.cpp


Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -259,7 +259,8 @@
   ModuleAnalysisManager MAM;
 
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager);
+  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager,
+  Conf.VerifyEach);
   SI.registerCallbacks(PIC, );
   PassBuilder PB(TM, Conf.PTO, PGOOpt, );
 
Index: llvm/include/llvm/LTO/Config.h
===
--- llvm/include/llvm/LTO/Config.h
+++ llvm/include/llvm/LTO/Config.h
@@ -57,6 +57,7 @@
   CodeGenOpt::Level CGOptLevel = CodeGenOpt::Default;
   CodeGenFileType CGFileType = CGFT_ObjectFile;
   unsigned OptLevel = 2;
+  bool VerifyEach = false;
   bool DisableVerify = false;
 
   /// Use the standard optimization pipeline.
Index: clang/test/CodeGen/verify-each.c
===
--- /dev/null
+++ clang/test/CodeGen/verify-each.c
@@ -0,0 +1,19 @@
+// Test to ensure -llvm-verify-each works.
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager 2>&1 | FileCheck %s --check-prefix=NO
+// NO-NOT: Verifying
+
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager -llvm-verify-each 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -O2 -o %t.o -flto=thin -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager -llvm-verify-each 2>&1 | FileCheck %s
+// RUN: llvm-lto -thinlto -o %t %t.o
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -o /dev/null 
-x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -llvm-verify-each 
2>&1 | FileCheck %s
+
+// CHECK: Verifying function foo
+// CHECK: Running pass: InstCombinePass
+// CHECK: Verifying function foo
+
+void foo(void) {
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -839,7 +839,7 @@
   StandardInstrumentations SI(
   TheModule->getContext(),
   (CodeGenOpts.DebugPassManager || DebugPassStructure),
-  /*VerifyEach*/ false, PrintPassOpts);
+  CodeGenOpts.VerifyEach, PrintPassOpts);
   SI.registerCallbacks(PIC, );
   PassBuilder PB(TM.get(), PTO, PGOOpt, );
 
@@ -1192,6 +1192,7 @@
 
   Conf.ProfileRemapping = std::move(ProfileRemapping);
   Conf.DebugPassManager = CGOpts.DebugPassManager;
+  Conf.VerifyEach = CGOpts.VerifyEach;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5738,6 +5738,9 @@
 
 let Flags = [CC1Option, NoDriverOption] in {
 
+def llvm_verify_each : Flag<["-"], "llvm-verify-each">,
+  HelpText<"Run the LLVM verifier after every LLVM pass">,
+  MarshallingInfoFlag>;
 def disable_llvm_verifier : Flag<["-"], "disable-llvm-verifier">,
   HelpText<"Don't run the LLVM IR verifier pass">,
   MarshallingInfoNegativeFlag>;
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -322,6 +322,8 @@
 
 CODEGENOPT(VerifyModule  , 1, 1) ///< Control whether the module should be 
run
  ///< through the LLVM Verifier.
+CODEGENOPT(VerifyEach, 1, 1) ///< Control whether the LLVM verifier
+ ///< should run after every pass.
 
 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force stack
  ///< realignment.


Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -259,7 +259,8 @@
   ModuleAnalysisManager MAM;
 
   PassInstrumentationCallbacks PIC;
-  

[PATCH] D153468: [clang][LTO] Add flag to run verifier after every pass

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 533627.
aeubanks marked an inline comment as done.
aeubanks added a comment.

update test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153468

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/verify-each.c
  llvm/include/llvm/LTO/Config.h
  llvm/lib/LTO/LTOBackend.cpp


Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -259,7 +259,8 @@
   ModuleAnalysisManager MAM;
 
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager);
+  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager,
+  Conf.VerifyEach);
   SI.registerCallbacks(PIC, );
   PassBuilder PB(TM, Conf.PTO, PGOOpt, );
 
Index: llvm/include/llvm/LTO/Config.h
===
--- llvm/include/llvm/LTO/Config.h
+++ llvm/include/llvm/LTO/Config.h
@@ -57,6 +57,7 @@
   CodeGenOpt::Level CGOptLevel = CodeGenOpt::Default;
   CodeGenFileType CGFileType = CGFT_ObjectFile;
   unsigned OptLevel = 2;
+  bool VerifyEach = false;
   bool DisableVerify = false;
 
   /// Use the standard optimization pipeline.
Index: clang/test/CodeGen/verify-each.c
===
--- /dev/null
+++ clang/test/CodeGen/verify-each.c
@@ -0,0 +1,19 @@
+// Test to ensure -llvm-verify-each works.
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager 2>&1 | FileCheck %s --check-prefix=NO
+// NO-NOT: Verifying
+
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager -llvm-verify-each 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -O2 -o %t.o -flto=thin -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager -llvm-verify-each 2>&1 | FileCheck %s
+// RUN: llvm-lto -thinlto -o %t %t.o
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -o /dev/null 
-x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -llvm-verify-each 
2>&1 | FileCheck %s
+
+// CHECK: Verifying function foo
+// CHECK: Running pass: InstCombinePass
+// CHECK: Verifying function foo
+
+void foo(void) {
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -839,7 +839,7 @@
   StandardInstrumentations SI(
   TheModule->getContext(),
   (CodeGenOpts.DebugPassManager || DebugPassStructure),
-  /*VerifyEach*/ false, PrintPassOpts);
+  CodeGenOpts.VerifyEach, PrintPassOpts);
   SI.registerCallbacks(PIC, );
   PassBuilder PB(TM.get(), PTO, PGOOpt, );
 
@@ -1192,6 +1192,7 @@
 
   Conf.ProfileRemapping = std::move(ProfileRemapping);
   Conf.DebugPassManager = CGOpts.DebugPassManager;
+  Conf.VerifyEach = CGOpts.VerifyEach;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5737,6 +5737,9 @@
 
 let Flags = [CC1Option, NoDriverOption] in {
 
+def llvm_verify_each : Flag<["-"], "llvm-verify-each">,
+  HelpText<"Run the LLVM verifier after every LLVM pass">,
+  MarshallingInfoFlag>;
 def disable_llvm_verifier : Flag<["-"], "disable-llvm-verifier">,
   HelpText<"Don't run the LLVM IR verifier pass">,
   MarshallingInfoNegativeFlag>;
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -322,6 +322,8 @@
 
 CODEGENOPT(VerifyModule  , 1, 1) ///< Control whether the module should be 
run
  ///< through the LLVM Verifier.
+CODEGENOPT(VerifyEach, 1, 1) ///< Control whether the LLVM verifier
+ ///< should run after every pass.
 
 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force stack
  ///< realignment.


Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -259,7 +259,8 @@
   ModuleAnalysisManager MAM;
 
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager);
+  StandardInstrumentations SI(Mod.getContext(), 

[PATCH] D153468: [clang][LTO] Add flag to run verifier after every pass

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/test/CodeGen/verify-each.c:5
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager 2>&1 | FileCheck %s --check-prefix=NO
+// NO-NOT: Verifying
+

tejohnson wrote:
> Huh, really surprised we are never normally running the verifier through 
> clang! How did we hit the verification error you used this to narrow down in 
> the first place?
there is one, but that's a manual run of the verifier pass that doesn't output 
anything, as opposed to the verifier being run directly by pass instrumentation 
which does output something when debug logging is on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153468

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

[PATCH] D153468: [clang][LTO] Add flag to run verifier after every pass

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
aeubanks added a reviewer: tejohnson.
Herald added subscribers: ormris, steven_wu, hiraditya, inglorion.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Helps with debugging issues caught by the verifier.

Plumbed through both normal clang compile and ThinLTO.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153468

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/verify-each.c
  llvm/include/llvm/LTO/Config.h
  llvm/lib/LTO/LTOBackend.cpp


Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -259,7 +259,8 @@
   ModuleAnalysisManager MAM;
 
   PassInstrumentationCallbacks PIC;
-  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager);
+  StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager,
+  Conf.VerifyEach);
   SI.registerCallbacks(PIC, );
   PassBuilder PB(TM, Conf.PTO, PGOOpt, );
 
Index: llvm/include/llvm/LTO/Config.h
===
--- llvm/include/llvm/LTO/Config.h
+++ llvm/include/llvm/LTO/Config.h
@@ -57,6 +57,7 @@
   CodeGenOpt::Level CGOptLevel = CodeGenOpt::Default;
   CodeGenFileType CGFileType = CGFT_ObjectFile;
   unsigned OptLevel = 2;
+  bool VerifyEach = false;
   bool DisableVerify = false;
 
   /// Use the standard optimization pipeline.
Index: clang/test/CodeGen/verify-each.c
===
--- /dev/null
+++ clang/test/CodeGen/verify-each.c
@@ -0,0 +1,17 @@
+// Test to ensure -llvm-verify-each works.
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager 2>&1 | FileCheck %s --check-prefix=NO
+// NO-NOT: Verifying
+
+// RUN: %clang_cc1 -O2 -o /dev/null -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager -llvm-verify-each 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -O2 -o %t.o -flto=thin -triple x86_64-unknown-linux-gnu 
-emit-llvm-bc %s -fdebug-pass-manager -llvm-verify-each 2>&1 | FileCheck %s
+// RUN: llvm-lto -thinlto -o %t %t.o
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -o /dev/null 
-x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -llvm-verify-each 
2>&1 | FileCheck %s
+
+// CHECK: Verifying
+
+void foo(void) {
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -839,7 +839,7 @@
   StandardInstrumentations SI(
   TheModule->getContext(),
   (CodeGenOpts.DebugPassManager || DebugPassStructure),
-  /*VerifyEach*/ false, PrintPassOpts);
+  CodeGenOpts.VerifyEach, PrintPassOpts);
   SI.registerCallbacks(PIC, );
   PassBuilder PB(TM.get(), PTO, PGOOpt, );
 
@@ -1192,6 +1192,7 @@
 
   Conf.ProfileRemapping = std::move(ProfileRemapping);
   Conf.DebugPassManager = CGOpts.DebugPassManager;
+  Conf.VerifyEach = CGOpts.VerifyEach;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5737,6 +5737,9 @@
 
 let Flags = [CC1Option, NoDriverOption] in {
 
+def llvm_verify_each : Flag<["-"], "llvm-verify-each">,
+  HelpText<"Run the LLVM verifier after every LLVM pass">,
+  MarshallingInfoFlag>;
 def disable_llvm_verifier : Flag<["-"], "disable-llvm-verifier">,
   HelpText<"Don't run the LLVM IR verifier pass">,
   MarshallingInfoNegativeFlag>;
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -322,6 +322,8 @@
 
 CODEGENOPT(VerifyModule  , 1, 1) ///< Control whether the module should be 
run
  ///< through the LLVM Verifier.
+CODEGENOPT(VerifyEach, 1, 1) ///< Control whether the LLVM verifier
+ ///< should run after every pass.
 
 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force stack
  ///< realignment.


Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -259,7 +259,8 @@
   ModuleAnalysisManager MAM;
 
   PassInstrumentationCallbacks PIC;
-  

[PATCH] D151479: [clang] Use IgnoreParensSingleStep in more places

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
aeubanks added a reviewer: rsmith.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Addresses a post-commit comment on D146764 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151479

Files:
  clang/lib/Sema/SemaInit.cpp


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
@@ -163,22 +164,9 @@
   while (true) {
 E->setType(Ty);
 E->setValueKind(VK_PRValue);
-if (isa(E) || isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else if (PredefinedExpr *PE = dyn_cast(E)) {
-  E = PE->getFunctionName();
-} else {
-  llvm_unreachable("unexpected expr in string literal init");
-}
+if (isa(E) || isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 
@@ -187,20 +175,9 @@
 static void updateGNUCompoundLiteralRValue(Expr *E) {
   while (true) {
 E->setValueKind(VK_PRValue);
-if (isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else {
-  llvm_unreachable("unexpected expr in array compound literal init");
-}
+if (isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
@@ -163,22 +164,9 @@
   while (true) {
 E->setType(Ty);
 E->setValueKind(VK_PRValue);
-if (isa(E) || isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else if (PredefinedExpr *PE = dyn_cast(E)) {
-  E = PE->getFunctionName();
-} else {
-  llvm_unreachable("unexpected expr in string literal init");
-}
+if (isa(E) || isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 
@@ -187,20 +175,9 @@
 static void updateGNUCompoundLiteralRValue(Expr *E) {
   while (true) {
 E->setValueKind(VK_PRValue);
-if (isa(E)) {
-  break;
-} else if (ParenExpr *PE = dyn_cast(E)) {
-  E = PE->getSubExpr();
-} else if (UnaryOperator *UO = dyn_cast(E)) {
-  assert(UO->getOpcode() == UO_Extension);
-  E = UO->getSubExpr();
-} else if (GenericSelectionExpr *GSE = dyn_cast(E)) {
-  E = GSE->getResultExpr();
-} else if (ChooseExpr *CE = dyn_cast(E)) {
-  E = CE->getChosenSubExpr();
-} else {
-  llvm_unreachable("unexpected expr in array compound literal init");
-}
+if (isa(E))
+  break;
+E = IgnoreParensSingleStep(E);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145403: [Pipeline] Don't run EarlyFPM in LTO post link

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG13e3d4aa5a4e: [Pipeline] Dont run EarlyFPM in LTO post 
link (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145403

Files:
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Other/new-pm-pgo.ll
  llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll

Index: llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
@@ -29,30 +29,23 @@
 ; CHECK-NOEXT: {{^}}
 
 ; CHECK-EP-PIPELINE-START: Running pass: NoOpModulePass
-; CHECK-O: Running pass: InferFunctionAttrsPass
+; CHECK-O: Running pass: SampleProfileLoaderPass
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O-NEXT: Running pass: CoroEarlyPass
-; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
-; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
-; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
-; CHECK-O-NEXT: Running pass: SROAPass
-; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
-; CHECK-O-NEXT: Running pass: EarlyCSEPass
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O3-NEXT: Running pass: CallSiteSplittingPass
-; CHECK-O-NEXT: Running pass: SampleProfileLoaderPass
 ; CHECK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running pass: PGOIndirectCallPromotion
 ; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: OpenMPOptPass
 ; CHECK-O-NEXT: Running pass: LowerTypeTestsPass
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
+; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
+; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
+; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running pass: CalledValuePropagationPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: PromotePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Running analysis: AAManager on foo
Index: llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
@@ -28,25 +28,18 @@
 ; CHECK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
-; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O-NEXT: Running pass: CoroEarlyPass
-; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
-; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
-; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
-; CHECK-O-NEXT: Running pass: SROAPass
-; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
-; CHECK-O-NEXT: Running pass: EarlyCSEPass
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O3-NEXT: Running pass: CallSiteSplittingPass
 ; CHECK-O-NEXT: Running pass: OpenMPOptPass
 ; CHECK-O-NEXT: Running pass: LowerTypeTestsPass
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
+; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
+; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
+; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running pass: CalledValuePropagationPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: PromotePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running analysis: AAManager
 ; CHECK-O-NEXT: Running analysis: BasicAA
 ; CHECK-O-NEXT: Running analysis: ScopedNoAliasAA
Index: llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
@@ -43,25 +43,18 @@
 ; CHECK-POSTLINK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-POSTLINK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-POSTLINK-O-NEXT: Running analysis: 

[PATCH] D145403: [Pipeline] Don't run EarlyFPM in LTO post link

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 524406.
aeubanks added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145403

Files:
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Other/new-pm-pgo.ll
  llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll

Index: llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
@@ -29,30 +29,23 @@
 ; CHECK-NOEXT: {{^}}
 
 ; CHECK-EP-PIPELINE-START: Running pass: NoOpModulePass
-; CHECK-O: Running pass: InferFunctionAttrsPass
+; CHECK-O: Running pass: SampleProfileLoaderPass
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O-NEXT: Running pass: CoroEarlyPass
-; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
-; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
-; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
-; CHECK-O-NEXT: Running pass: SROAPass
-; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
-; CHECK-O-NEXT: Running pass: EarlyCSEPass
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O3-NEXT: Running pass: CallSiteSplittingPass
-; CHECK-O-NEXT: Running pass: SampleProfileLoaderPass
 ; CHECK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running pass: PGOIndirectCallPromotion
 ; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: OpenMPOptPass
 ; CHECK-O-NEXT: Running pass: LowerTypeTestsPass
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
+; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
+; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
+; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running pass: CalledValuePropagationPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: PromotePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Running analysis: AAManager on foo
Index: llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
@@ -28,25 +28,18 @@
 ; CHECK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
-; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O-NEXT: Running pass: CoroEarlyPass
-; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
-; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
-; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
-; CHECK-O-NEXT: Running pass: SROAPass
-; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
-; CHECK-O-NEXT: Running pass: EarlyCSEPass
-; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
-; CHECK-O3-NEXT: Running pass: CallSiteSplittingPass
 ; CHECK-O-NEXT: Running pass: OpenMPOptPass
 ; CHECK-O-NEXT: Running pass: LowerTypeTestsPass
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
+; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
+; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
+; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running pass: CalledValuePropagationPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: PromotePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running analysis: AAManager
 ; CHECK-O-NEXT: Running analysis: BasicAA
 ; CHECK-O-NEXT: Running analysis: ScopedNoAliasAA
Index: llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
@@ -43,25 +43,18 @@
 ; CHECK-POSTLINK-O-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-POSTLINK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-POSTLINK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
-; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
-; CHECK-O-NEXT: Running analysis: 

[PATCH] D145403: [Pipeline] Don't run EarlyFPM in LTO post link

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

I was trying to get this landed with https://reviews.llvm.org/D145265 so I 
could measure the impact of the two together. But also now there are some merge 
conflicts that I'd rather deal with after submitting D145265 
. But that patch is more likely to get 
reverted anyway so yeah this should go in first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145403

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

[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

`not` will set `LLVM_DISABLE_SYMBOLIZATION` if we use `not --crash`

are these tests supposed to crash or gracefully fail? if they're supposed to 
crash then we should use `not --crash`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148851

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

[PATCH] D150645: [Driver] Support multi /guard: options

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

if you add a test for the repro, relanding lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150645

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

[PATCH] D150645: [Driver] Support multi /guard: options

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

this causes `./build/rel/bin/clang-cl /c /tmp/a.cc /Fo/tmp/a 
/guard:cf,nochecks` to go from no warnings to

  clang-cl: warning: argument unused during compilation: '/guard:cf,nochecks' 
[-Wunused-command-line-argument]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150645

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:476
 
+  FPM.addPass(EarlyCSEPass());
+

nikic wrote:
> Why the extra EarlyCSE pass in the `O1` pipeline?
forgot to update the commit message, see that



Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:547
+  SimplifyCFGPass(SimplifyCFGOptions().convertSwitchRangeToICmp(true)));
+  FPM.addPass(AggressiveInstCombinePass());
 

nikic wrote:
> Any particular reason why InstCombine and AggressiveInstCombine are no longer 
> directly next to each other?
we need InstCombine -> SimplifyCFG, so swapped them (see updated commit 
message). I don't necessarily see why AIC needs to be right after IC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 523182.
aeubanks edited the summary of this revision.
aeubanks added a comment.

update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

Files:
  clang/test/Headers/mm_malloc.c
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-pgo-preinline.ll
  llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
  llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll

Index: llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
===
--- llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
+++ llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
@@ -16,10 +16,13 @@
 
 define void @caller1(i1 %c, ptr align 1 %ptr) {
 ; ASSUMPTIONS-OFF-LABEL: @caller1(
-; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-OFF:   false1:
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-OFF:   common.ret:
-; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR:%.*]], align 8
+; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
@@ -27,15 +30,15 @@
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:ret void
-; ASSUMPTIONS-OFF:   false2:
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ;
 ; ASSUMPTIONS-ON-LABEL: @caller1(
-; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-ON:   false1:
+; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-ON:   common.ret:
-; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR:%.*]], i64 8) ]
+; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR]], i64 8) ]
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
@@ -44,9 +47,6 @@
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:ret void
-; ASSUMPTIONS-ON:   false2:
-; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ;
   br i1 %c, label %true1, label %false1
 
Index: llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
===
--- llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
+++ llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
@@ -1,11 +1,28 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -O3 -S < %s | FileCheck %s
 
+; FIXME: see the following issues for background
+; https://github.com/llvm/llvm-project/issues/51631

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 523175.
aeubanks added a comment.

rebase

keep the existing EarlyCSE/InstCombine ordering


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

Files:
  clang/test/Headers/mm_malloc.c
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Feature/optnone-opt.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-pgo-preinline.ll
  llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
  llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll

Index: llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
===
--- llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
+++ llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
@@ -16,10 +16,13 @@
 
 define void @caller1(i1 %c, ptr align 1 %ptr) {
 ; ASSUMPTIONS-OFF-LABEL: @caller1(
-; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-OFF:   false1:
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-OFF:   common.ret:
-; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR:%.*]], align 8
+; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
@@ -27,15 +30,15 @@
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:ret void
-; ASSUMPTIONS-OFF:   false2:
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ;
 ; ASSUMPTIONS-ON-LABEL: @caller1(
-; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-ON:   false1:
+; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-ON:   common.ret:
-; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR:%.*]], i64 8) ]
+; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR]], i64 8) ]
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
@@ -44,9 +47,6 @@
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:ret void
-; ASSUMPTIONS-ON:   false2:
-; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ;
   br i1 %c, label %true1, label %false1
 
Index: llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
===
--- llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
+++ llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
@@ -1,11 +1,28 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -O3 -S < %s | FileCheck %s
 
+; FIXME: see the following issues for background
+; 

[PATCH] D150326: [WPD] Update llvm.public.type.test after importing functions

[Arthur Eubanks via Phabricator via cfe-commits]

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



Comment at: llvm/test/ThinLTO/X86/public-type-test.ll:31
 ; HIDDEN-NOT: call {{.*}}@llvm.public.type.test
 ; HIDDEN: call {{.*}}@llvm.type.test
 

should this be checked twice for both public type tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150326

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG878e590503df: Reland [clang] Make predefined expressions 
string literals under -fms-extensions (authored by aeubanks).

Changed prior to commit:
  https://reviews.llvm.org/D146764?vs=520204=521039#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Modules/predefined.cpp
  clang/test/Sema/ms_predefined_expr.cpp
  clang/test/SemaCXX/predefined-expr-msvc.cpp

Index: clang/test/SemaCXX/predefined-expr-msvc.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/predefined-expr-msvc.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify -fms-extensions
+
+// expected-no-diagnostics
+
+struct StringRef {
+  StringRef(const char *);
+};
+template 
+StringRef getTypeName() {
+  StringRef s = __func__;
+}
+
Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+
+void f() {
+ const char a[] = __FUNCTION__; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+ const char b[] = __FUNCDNAME__; // expected-warning{{initializing an array from a '__FUNCDNAME__' predefined identifier is a Microsoft extension}}
+ const char c[] = __FUNCSIG__; // expected-warning{{initializing an array from a '__FUNCSIG__' predefined identifier is a Microsoft extension}}
+ const char d[] = __func__; // expected-warning{{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+ const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft extension}}
+}
Index: clang/test/Modules/predefined.cpp
===
--- /dev/null
+++ clang/test/Modules/predefined.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm -fms-extensions -verify
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions -fsyntax-only -verify
+
+//--- a.h
+
+// expected-no-diagnostics
+
+export module A;
+
+export template 
+void f() {
+char a[] = __func__;
+}
+
+//--- a.cpp
+
+// expected-warning@a.h:8 {{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+
+import A;
+
+void g() {
+f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -174,6 +174,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
@@ -8508,6 +8510,15 @@
 << Init->getSourceRange();
   }
 
+  if 

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D149800#4330831 , @davidxl wrote:

> This patch uses a cleaner method than the previous effort. There is some 
> differences:
>
> 1. yamauchi's patch works for both iFDO and autoFDO, while this patch limits 
> to iFDO
> 2. yamauchi's patch also handles size optimization (IIRC) at granularity 
> smaller than function
>
> Longer term, the previous functionality should fold into the new framework.

Could you clarify what "previous functionality" and "new framework" mean here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149800

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

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

the following crashes with this patch:

  struct StringRef {
StringRef(const char *);
  };
  template 
  StringRef getTypeName() {
StringRef s = __func__;
  }

`clang -cc1 -fms-extensions -std=c++17 -x c++ a.cpp -fsyntax-only`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG856f384bf945: [clang] Make predefined expressions string 
literals under -fms-extensions (authored by aeubanks).

Changed prior to commit:
  https://reviews.llvm.org/D146764?vs=519613=520204#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Modules/predefined.cpp
  clang/test/Sema/ms_predefined_expr.cpp

Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+
+void f() {
+ const char a[] = __FUNCTION__; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+ const char b[] = __FUNCDNAME__; // expected-warning{{initializing an array from a '__FUNCDNAME__' predefined identifier is a Microsoft extension}}
+ const char c[] = __FUNCSIG__; // expected-warning{{initializing an array from a '__FUNCSIG__' predefined identifier is a Microsoft extension}}
+ const char d[] = __func__; // expected-warning{{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+ const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft extension}}
+}
Index: clang/test/Modules/predefined.cpp
===
--- /dev/null
+++ clang/test/Modules/predefined.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm -fms-extensions -verify
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions -fsyntax-only -verify
+
+//--- a.h
+
+// expected-no-diagnostics
+
+export module A;
+
+export template 
+void f() {
+char a[] = __func__;
+}
+
+//--- a.cpp
+
+// expected-warning@a.h:8 {{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+
+import A;
+
+void g() {
+f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -174,6 +174,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
@@ -8501,6 +8503,15 @@
 << Init->getSourceRange();
   }
 
+  if (S.getLangOpts().MicrosoftExt && Args.size() == 1 &&
+  isa(Args[0]) && Entity.getType()->isArrayType()) {
+// Produce a Microsoft compatibility warning when initializing from a
+// predefined expression since MSVC treats predefined expressions as string
+// literals.
+Expr *Init = Args[0];
+S.Diag(Init->getBeginLoc(), diag::ext_init_from_predefined) << Init;
+  }
+
   // OpenCL v2.0 s6.13.11.1. atomic variables can be initialized in global scope
   QualType 

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

added a release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 519613.
aeubanks added a comment.

only diagnose if we're initializing an array


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Modules/predefined.cpp
  clang/test/Sema/ms_predefined_expr.cpp

Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+
+void f() {
+ const char a[] = __FUNCTION__; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+ const char b[] = __FUNCDNAME__; // expected-warning{{initializing an array from a '__FUNCDNAME__' predefined identifier is a Microsoft extension}}
+ const char c[] = __FUNCSIG__; // expected-warning{{initializing an array from a '__FUNCSIG__' predefined identifier is a Microsoft extension}}
+ const char d[] = __func__; // expected-warning{{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+ const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft extension}}
+}
Index: clang/test/Modules/predefined.cpp
===
--- /dev/null
+++ clang/test/Modules/predefined.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm -fms-extensions -verify
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions -fsyntax-only -verify
+
+//--- a.h
+
+// expected-no-diagnostics
+
+export module A;
+
+export template 
+void f() {
+char a[] = __func__;
+}
+
+//--- a.cpp
+
+// expected-warning@a.h:8 {{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+
+import A;
+
+void g() {
+f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -174,6 +174,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
@@ -8501,6 +8503,15 @@
 << Init->getSourceRange();
   }
 
+  if (S.getLangOpts().MicrosoftExt && Args.size() == 1 &&
+  isa(Args[0]) && Entity.getType()->isArrayType()) {
+// Produce a Microsoft compatibility warning when initializing from a
+// predefined expression since MSVC treats predefined expressions as string
+// literals.
+Expr *Init = Args[0];
+S.Diag(Init->getBeginLoc(), diag::ext_init_from_predefined) << Init;
+  }
+
   // OpenCL v2.0 s6.13.11.1. atomic variables can be initialized in global scope
   QualType ETy = Entity.getType();
   bool HasGlobalAS = ETy.hasAddressSpace() &&
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -3581,7 

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D149800#4317275 , @mtrofin wrote:

> high level question, why not have it as a pass that runs after profiles (of 
> whatever kind - instrumented or sample-based) are ingested. The pass would 
> attribute functions as described.

that was also my initial thought, but I found it weird that only the ifdo pass 
marked functions as cold, but not the samplepgo pass. I remembered something 
about "precise" profiles and thought maybe that had something to do with not 
marking functions as cold with samplepgo? But maybe 
`PSI->isFunctionColdInCallGraph(F, *BFI)` takes care of all of that

In D149800#4317483 , @tejohnson wrote:

> Previously @yamauchi did a bunch of work related to this called PGSO (Profile 
> Guided Size Optimization). See the functions in 
> https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/SizeOpts.cpp,
>  which are used to guide a number of optimizations that can affect code size. 
> Do you know why those weren't sufficient?

I think there are still a lot of places that just look at optsize/minsize. I'll 
take a look at the inline cost model more to see how it interacts with hotness. 
Separately, it seems good for compile times to calculate once if a function is 
cold or not and save that result as optsize/minsize.

> @davidxl may remember the details, but iirc fully using minsize/optnone for 
> cold funcs might have been too big of a hammer performance-wise.

s/optnone/optsize? But I'm surprised that even minsize would make a perf 
difference just in functions that are supposedly cold. IIUC there aren't too 
many differences at least in the optimization pipeline for minsize functions 
aside from inlining thresholds (maybe there's more drastic changes in the 
backend?). It would be good to know if previous attempts at this sort of thing 
have shown perf regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149800

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

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

looking for feedback if this makes sense at all


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149800

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

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
Herald added subscribers: wlei, Enna1, ormris, wenlei, steven_wu, hiraditya.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

The performance of cold functions shouldn't matter too much, so if we care 
about binary sizes, add an option to mark cold functions as optsize/minsize for 
binary size, or optnone for compile times [1]. Clang patch will be in a future 
patch

Chrome size (ThinLTO + instrumented PGO):
base:371004392
optsize: 366883296
minsize: 342128928

[1] 
https://discourse.llvm.org/t/rfc-new-feature-proposal-de-optimizing-cold-functions-using-pgo-info/56388


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149800

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Support/PGOOptions.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/test/Transforms/PGOProfile/Inputs/cold-func-attr.proftext
  llvm/test/Transforms/PGOProfile/cold-func-attr.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -199,6 +199,18 @@
  cl::desc("Path to the profile remapping file."),
  cl::Hidden);
 
+static cl::opt PGOColdFuncAttr(
+"pgo-cold-func-attr", cl::init(PGOOptions::ColdFuncAttr::None), cl::Hidden,
+cl::desc(
+"Function attribute to apply to cold functions as determined by PGO"),
+cl::values(clEnumValN(PGOOptions::ColdFuncAttr::None, "none", "None"),
+   clEnumValN(PGOOptions::ColdFuncAttr::OptSize, "optsize",
+  "Mark cold functions with optsize."),
+   clEnumValN(PGOOptions::ColdFuncAttr::MinSize, "minsize",
+  "Mark cold functions with minsize."),
+   clEnumValN(PGOOptions::ColdFuncAttr::OptNone, "optnone",
+  "Mark cold functions with optnone.")));
+
 static cl::opt DebugInfoForProfiling(
 "debug-info-for-profiling", cl::init(false), cl::Hidden,
 cl::desc("Emit special debug info to enable PGO profile generation."));
@@ -338,21 +350,23 @@
   std::optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr,
+   PGOOptions::NoCSAction, PGOColdFuncAttr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
-   PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS, PGOOptions::IRUse,
+   PGOOptions::NoCSAction, PGOColdFuncAttr);
 break;
   case SampleUse:
 P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
-   PGOOptions::SampleUse);
+   PGOOptions::SampleUse, PGOOptions::NoCSAction,
+   PGOColdFuncAttr);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
   P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
- PGOOptions::NoCSAction, DebugInfoForProfiling,
- PseudoProbeForProfiling);
+ PGOOptions::NoCSAction, PGOColdFuncAttr,
+ DebugInfoForProfiling, PseudoProbeForProfiling);
 else
   P = std::nullopt;
   }
@@ -372,7 +386,8 @@
 P->CSProfileGenFile = CSProfileGenFile;
   } else
 P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
-   PGOOptions::NoAction, PGOOptions::CSIRInstr);
+   PGOOptions::NoAction, PGOOptions::CSIRInstr,
+   PGOColdFuncAttr);
 } else /* CSPGOKindFlag == CSInstrUse */ {
   if (!P) {
 errs() << "CSInstrUse needs to be together with InstrUse";
Index: llvm/test/Transforms/PGOProfile/cold-func-attr.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/cold-func-attr.ll
@@ -0,0 +1,62 @@
+; RUN: llvm-profdata merge %S/Inputs/cold-func-attr.proftext -o %t.profdata
+; RUN: opt < %s -passes=pgo-instr-use -pgo-kind=pgo-instr-use-pipeline -pgo-test-profile-file=%t.profdata -S -pgo-cold-func-attr=none| FileCheck %s --check-prefixes=NONE
+; RUN: opt < %s -passes=pgo-instr-use -pgo-kind=pgo-instr-use-pipeline -pgo-test-profile-file=%t.profdata -S -pgo-cold-func-attr=optsize | FileCheck %s --check-prefixes=OPTSIZE,CHECK
+; RUN: opt < %s -passes=pgo-instr-use -pgo-kind=pgo-instr-use-pipeline -pgo-test-profile-file=%t.profdata -S -pgo-cold-func-attr=minsize | 

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 519215.
aeubanks added a comment.

add -verify to module test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Modules/predefined.cpp
  clang/test/Sema/ms_predefined_expr.cpp

Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+
+void f() {
+ const char a[] = __FUNCTION__; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+ const char b[] = __FUNCDNAME__; // expected-warning{{initializing an array from a '__FUNCDNAME__' predefined identifier is a Microsoft extension}}
+ const char c[] = __FUNCSIG__; // expected-warning{{initializing an array from a '__FUNCSIG__' predefined identifier is a Microsoft extension}}
+ const char d[] = __func__; // expected-warning{{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+ const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft extension}}
+}
Index: clang/test/Modules/predefined.cpp
===
--- /dev/null
+++ clang/test/Modules/predefined.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm -fms-extensions -verify
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions -fsyntax-only -verify
+
+//--- a.h
+
+// expected-no-diagnostics
+
+export module A;
+
+export template 
+void f() {
+char a[] = __func__;
+}
+
+//--- a.cpp
+
+// expected-warning@a.h:8 {{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+
+import A;
+
+void g() {
+f(); // expected-note {{in instantiation of function template specialization 'f' requested here}}
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -173,6 +173,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
@@ -8500,6 +8502,15 @@
 << Init->getSourceRange();
   }
 
+  if (S.getLangOpts().MicrosoftExt && Args.size() == 1 &&
+  isa(Args[0])) {
+// Produce a Microsoft compatibility warning when initializing from a
+// predefined expression since MSVC treats predefined expressions as string
+// literals.
+Expr *Init = Args[0];
+S.Diag(Init->getBeginLoc(), diag::ext_init_from_predefined) << Init;
+  }
+
   // OpenCL v2.0 s6.13.11.1. atomic variables can be initialized in global scope
   QualType ETy = Entity.getType();
   bool HasGlobalAS = ETy.hasAddressSpace() &&
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -3580,7 +3580,8 @@
 }
   }
 
-  return 

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D143624#4315508 , @nikic wrote:

> In D143624#4315468 , @dmgreen wrote:
>
>> It looks like there is quite a lot more optimization that happens to the 
>> function being always-inlined (__SSAT) before this change. Through multiple 
>> rounds of instcombine, almost to the end of the pass pipeline. The new 
>> version runs a lot less before inlining, only running 
>> instcombine->simplifycfg and not seeing another instcombine to clean up the 
>> results. Is that because the AlwaysInlinePass is a module pass and it now 
>> only runs the passes up to that point?
>
> Yes, which is why I personally think this change isn't a good idea. This 
> essentially breaks our invariant that functions get simplified before they 
> are inlined. This significantly alters the way alwaysinline functions will be 
> optimized relative to normally inlined functions.

That invariant shouldn't matter if we're not using heuristics to inline. The 
normal heuristic-based inliner will still work on simplified callees, but now 
with the additional benefit of seeing the state of an SCC where there may be 
alwaysinline calls after the inlinings that must happen having happened.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143624

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

added the warning, not sure if this needs more tests for testing the 
interaction between `-pedantic`, `-Wmicrosoft`, 
`-Wmicrosoft-init-from-predefined` or if that's already assumed to work




Comment at: clang/test/Modules/predefined.cpp:6
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm 
-fms-extensions
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions 
-fsyntax-only
+

aaron.ballman wrote:
> Er, should we have a `-verify` on this as well as `// 
> expected-no-diagnostics`?
isn't that tested by the Sema test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 518887.
aeubanks added a comment.

add warning


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Modules/predefined.cpp
  clang/test/Sema/ms_predefined_expr.cpp

Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+
+void f() {
+ const char a[] = __FUNCTION__; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+ const char b[] = __FUNCDNAME__; // expected-warning{{initializing an array from a '__FUNCDNAME__' predefined identifier is a Microsoft extension}}
+ const char c[] = __FUNCSIG__; // expected-warning{{initializing an array from a '__FUNCSIG__' predefined identifier is a Microsoft extension}}
+ const char d[] = __func__; // expected-warning{{initializing an array from a '__func__' predefined identifier is a Microsoft extension}}
+ const char e[] = __PRETTY_FUNCTION__; // expected-warning{{initializing an array from a '__PRETTY_FUNCTION__' predefined identifier is a Microsoft extension}}
+}
Index: clang/test/Modules/predefined.cpp
===
--- /dev/null
+++ clang/test/Modules/predefined.cpp
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm -fms-extensions
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions -fsyntax-only
+
+//--- a.h
+
+export module A;
+
+export template 
+void f() {
+char a[] = __func__;
+}
+
+//--- a.cpp
+
+import A;
+
+void g() {
+f();
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -173,6 +173,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
@@ -8500,6 +8502,15 @@
 << Init->getSourceRange();
   }
 
+  if (S.getLangOpts().MicrosoftExt && Args.size() == 1 &&
+  isa(Args[0])) {
+// Produce a Microsoft compatibility warning when initializing from a
+// predefined expression since MSVC treats predefined expressions as string
+// literals.
+Expr *Init = Args[0];
+S.Diag(Init->getBeginLoc(), diag::ext_init_from_predefined) << Init;
+  }
+
   // OpenCL v2.0 s6.13.11.1. atomic variables can be initialized in global scope
   QualType ETy = Entity.getType();
   bool HasGlobalAS = ETy.hasAddressSpace() &&
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -3580,7 +3580,8 @@
 }
   }
 
-  return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
+  return PredefinedExpr::Create(Context, Loc, ResTy, IK, LangOpts.MicrosoftExt,
+SL);
 }
 
 ExprResult Sema::BuildSYCLUniqueStableNameExpr(SourceLocation OpLoc,
Index: clang/lib/AST/Expr.cpp

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1122
   }
+  void
+  getInliningDecisions(SmallVectorImpl ) const;

can this return a SmallVector instead of taking one as a mutable param?



Comment at: llvm/lib/IR/DiagnosticInfo.cpp:462
+  const MDOperand  = MDN->getOperand(0);
+  if (auto *MDT = dyn_cast(MO)) {
+for (const MDOperand  : MDT->operands()) {

seems simpler if this is always a `MDTuple` instead of special casing one entry 
to be `MDString`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141451

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 518777.
aeubanks added a comment.

add module test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Modules/predefined.cpp
  clang/test/Sema/ms_predefined_expr.cpp

Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+// expected-no-diagnostics
+
+void f() {
+ const char a[] = __FUNCTION__;
+ const char b[] = __FUNCDNAME__;
+ const char c[] = __FUNCSIG__;
+ const char d[] = __func__;
+ const char e[] = __PRETTY_FUNCTION__;
+}
Index: clang/test/Modules/predefined.cpp
===
--- /dev/null
+++ clang/test/Modules/predefined.cpp
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -x c++ -std=c++20 -emit-module-interface a.h -o a.pcm -fms-extensions
+// RUN: %clang_cc1 -std=c++20 a.cpp -fmodule-file=A=a.pcm -fms-extensions -fsyntax-only
+
+//--- a.h
+
+export module A;
+
+export template 
+void f() {
+char a[] = __func__;
+}
+
+//--- a.cpp
+
+import A;
+
+void g() {
+f();
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -173,6 +173,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -3580,7 +3580,8 @@
 }
   }
 
-  return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
+  return PredefinedExpr::Create(Context, Loc, ResTy, IK, LangOpts.MicrosoftExt,
+SL);
 }
 
 ExprResult Sema::BuildSYCLUniqueStableNameExpr(SourceLocation OpLoc,
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -663,13 +663,14 @@
 }
 
 PredefinedExpr::PredefinedExpr(SourceLocation L, QualType FNTy, IdentKind IK,
-   StringLiteral *SL)
+   bool IsTransparent, StringLiteral *SL)
 : Expr(PredefinedExprClass, FNTy, VK_LValue, OK_Ordinary) {
   PredefinedExprBits.Kind = IK;
   assert((getIdentKind() == IK) &&
  "IdentKind do not fit in PredefinedExprBitfields!");
   bool HasFunctionName = SL != nullptr;
   PredefinedExprBits.HasFunctionName = HasFunctionName;
+  PredefinedExprBits.IsTransparent = IsTransparent;
   PredefinedExprBits.Loc = L;
   if (HasFunctionName)
 setFunctionName(SL);
@@ -683,11 +684,11 @@
 
 PredefinedExpr *PredefinedExpr::Create(const ASTContext , SourceLocation L,
QualType FNTy, IdentKind IK,
-   StringLiteral *SL) {
+   bool IsTransparent, StringLiteral *SL) {
   bool HasFunctionName = SL != nullptr;
   void *Mem = Ctx.Allocate(totalSizeToAlloc(HasFunctionName),
alignof(PredefinedExpr));
-  

[PATCH] D149187: [clang] Canonicalize system headers in dependency file when -canonical-prefixes

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D149187#4311354 , @chapuni wrote:

> I am afraid this would be incompatible to bazel due to deep symlinks for 
> sandbox.

I assume bazel would use `-no-canonical-prefixes`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149187

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

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

In D146764#4310398 , @aaron.ballman 
wrote:

> I think you're missing changes in ASTReaderStmt.cpp and ASTWriterStmt.cpp, so 
> serialization through modules or PCH won't work without that.

done. how would this sort of change be tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 518548.
aeubanks added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/Sema/ms_predefined_expr.cpp

Index: clang/test/Sema/ms_predefined_expr.cpp
===
--- /dev/null
+++ clang/test/Sema/ms_predefined_expr.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+// expected-no-diagnostics
+
+void f() {
+ const char a[] = __FUNCTION__;
+ const char b[] = __FUNCDNAME__;
+ const char c[] = __FUNCSIG__;
+ const char d[] = __func__;
+ const char e[] = __PRETTY_FUNCTION__;
+}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -593,6 +593,7 @@
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
 Record.AddStmt(E->getFunctionName());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -582,6 +582,7 @@
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
 E->setFunctionName(cast(Record.readSubExpr()));
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -173,6 +173,8 @@
   E = GSE->getResultExpr();
 } else if (ChooseExpr *CE = dyn_cast(E)) {
   E = CE->getChosenSubExpr();
+} else if (PredefinedExpr *PE = dyn_cast(E)) {
+  E = PE->getFunctionName();
 } else {
   llvm_unreachable("unexpected expr in string literal init");
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -3580,7 +3580,8 @@
 }
   }
 
-  return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
+  return PredefinedExpr::Create(Context, Loc, ResTy, IK, LangOpts.MicrosoftExt,
+SL);
 }
 
 ExprResult Sema::BuildSYCLUniqueStableNameExpr(SourceLocation OpLoc,
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -663,13 +663,14 @@
 }
 
 PredefinedExpr::PredefinedExpr(SourceLocation L, QualType FNTy, IdentKind IK,
-   StringLiteral *SL)
+   bool IsTransparent, StringLiteral *SL)
 : Expr(PredefinedExprClass, FNTy, VK_LValue, OK_Ordinary) {
   PredefinedExprBits.Kind = IK;
   assert((getIdentKind() == IK) &&
  "IdentKind do not fit in PredefinedExprBitfields!");
   bool HasFunctionName = SL != nullptr;
   PredefinedExprBits.HasFunctionName = HasFunctionName;
+  PredefinedExprBits.IsTransparent = IsTransparent;
   PredefinedExprBits.Loc = L;
   if (HasFunctionName)
 setFunctionName(SL);
@@ -683,11 +684,11 @@
 
 PredefinedExpr *PredefinedExpr::Create(const ASTContext , SourceLocation L,
QualType FNTy, IdentKind IK,
-   StringLiteral *SL) {
+   bool IsTransparent, StringLiteral *SL) {
   bool HasFunctionName = SL != nullptr;
   void *Mem = Ctx.Allocate(totalSizeToAlloc(HasFunctionName),
alignof(PredefinedExpr));
-  return new (Mem) PredefinedExpr(L, FNTy, IK, SL);
+  return new (Mem) PredefinedExpr(L, FNTy, IK, IsTransparent, SL);
 }
 
 PredefinedExpr *PredefinedExpr::CreateEmpty(const ASTContext ,
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -7068,7 +7068,8 @@
 return std::move(Err);
 
   return PredefinedExpr::Create(Importer.getToContext(), ToBeginLoc, ToType,
-E->getIdentKind(), ToFunctionName);
+

[PATCH] D149187: [clang] Canonicalize system headers in dependency file when -canonical-prefixes

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8f0dd4ef3ed2: [clang] Canonicalize system headers in 
dependency file when -canonical-prefixes (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149187

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/DependencyOutputOptions.h
  clang/include/clang/Frontend/Utils.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/test/Driver/canonical-system-headers.c
  clang/test/Preprocessor/Inputs/canonical-system-headers/a.h
  clang/test/Preprocessor/canonical-system-headers.c

Index: clang/test/Preprocessor/canonical-system-headers.c
===
--- /dev/null
+++ clang/test/Preprocessor/canonical-system-headers.c
@@ -0,0 +1,16 @@
+// don't create symlinks on windows
+// UNSUPPORTED: system-windows
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/foo/
+// RUN: ln -f -s %S/Inputs/canonical-system-headers %t/foo/include
+// RUN: %clang_cc1 -isystem %T/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only
+// RUN: FileCheck %s --check-prefix=NOCANON --implicit-check-not=a.h < %t2
+// RUN: %clang_cc1 -isystem %T/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only -canonical-system-headers
+// RUN: FileCheck %s --check-prefix=CANON --implicit-check-not=a.h < %t2
+
+// NOCANON: foo/include/a.h
+// CANON: Inputs/canonical-system-headers/a.h
+
+#include 
Index: clang/test/Driver/canonical-system-headers.c
===
--- /dev/null
+++ clang/test/Driver/canonical-system-headers.c
@@ -0,0 +1,6 @@
+// RUN: %clang -MD -no-canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
+// RUN: %clang -MD -canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+// RUN: %clang -MD -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+
+// CHECK-YES: "-canonical-system-headers"
+// CHECK-NO-NOT: "-canonical-system-headers"
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -49,6 +49,7 @@
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
+  (),
   /*IsMissing*/ false);
   }
 
@@ -56,9 +57,11 @@
SrcMgr::CharacteristicKind FileType) override {
 StringRef Filename =
 llvm::sys::path::remove_leading_dotslash(SkippedFile.getName());
-DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+DepCollector.maybeAddDependency(Filename,
+/*FromModule=*/false,
 /*IsSystem=*/isSystem(FileType),
 /*IsModuleFile=*/false,
+(),
 /*IsMissing=*/false);
   }
 
@@ -69,9 +72,12 @@
   StringRef RelativePath, const Module *Imported,
   SrcMgr::CharacteristicKind FileType) override {
 if (!File)
-  DepCollector.maybeAddDependency(FileName, /*FromModule*/false,
- /*IsSystem*/false, /*IsModuleFile*/false,
- /*IsMissing*/true);
+  DepCollector.maybeAddDependency(FileName,
+  /*FromModule*/ false,
+  /*IsSystem*/ false,
+  /*IsModuleFile*/ false,
+  (),
+  /*IsMissing*/ true);
 // Files that actually exist are handled by FileChanged.
   }
 
@@ -82,9 +88,11 @@
   return;
 StringRef Filename =
 llvm::sys::path::remove_leading_dotslash(File->getName());
-DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+DepCollector.maybeAddDependency(Filename,
+/*FromModule=*/false,
 /*IsSystem=*/isSystem(FileType),
 /*IsModuleFile=*/false,
+(),
 /*IsMissing=*/false);
   }
 
@@ -100,10 +108,12 @@
   void moduleMapFileRead(SourceLocation Loc, const FileEntry ,
  bool IsSystem) override {
 StringRef Filename = Entry.getName();
-DepCollector.maybeAddDependency(Filename, /*FromModule*/false,
-/*IsSystem*/IsSystem,
-

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D129700: [clang] Don't emit type tests for dllexport/import classes

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf34ecb50e2c0: [clang] Dont emit type tests for 
dllexport/import classes (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129700

Files:
  clang/lib/CodeGen/CGVTables.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp


Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -74,10 +74,10 @@
   // MS: type.test{{.*}}!"?AUC2@@"
   c2->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C3"
-  // MS: type.test{{.*}}!"?AUC3@@"
+  // MS-NOT: type.test{{.*}}!"?AUC3@@"
   c3->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C4"
-  // MS: type.test{{.*}}!"?AUC4@@"
+  // MS-NOT: type.test{{.*}}!"?AUC4@@"
   c4->f();
   // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C5"
   // MS-NOT: type.test{{.*}}!"?AUC5@@"
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1212,7 +1212,8 @@
 }
 
 bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
-  if (RD->hasAttr() || RD->hasAttr())
+  if (RD->hasAttr() || RD->hasAttr() ||
+  RD->hasAttr() || RD->hasAttr())
 return true;
 
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
@@ -1239,13 +1240,9 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (getTriple().isOSBinFormatCOFF()) {
-if (RD->hasAttr() || RD->hasAttr())
-  return false;
-  } else {
-if (LV.getVisibility() != HiddenVisibility)
-  return false;
-  }
+  if (!getTriple().isOSBinFormatCOFF() &&
+  LV.getVisibility() != HiddenVisibility)
+return false;
 
   return !AlwaysHasLTOVisibilityPublic(RD);
 }


Index: clang/test/CodeGenCXX/lto-visibility-inference.cpp
===
--- clang/test/CodeGenCXX/lto-visibility-inference.cpp
+++ clang/test/CodeGenCXX/lto-visibility-inference.cpp
@@ -74,10 +74,10 @@
   // MS: type.test{{.*}}!"?AUC2@@"
   c2->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C3"
-  // MS: type.test{{.*}}!"?AUC3@@"
+  // MS-NOT: type.test{{.*}}!"?AUC3@@"
   c3->f();
   // ITANIUM: type.test{{.*}}!"_ZTS2C4"
-  // MS: type.test{{.*}}!"?AUC4@@"
+  // MS-NOT: type.test{{.*}}!"?AUC4@@"
   c4->f();
   // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C5"
   // MS-NOT: type.test{{.*}}!"?AUC5@@"
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1212,7 +1212,8 @@
 }
 
 bool CodeGenModule::AlwaysHasLTOVisibilityPublic(const CXXRecordDecl *RD) {
-  if (RD->hasAttr() || RD->hasAttr())
+  if (RD->hasAttr() || RD->hasAttr() ||
+  RD->hasAttr() || RD->hasAttr())
 return true;
 
   if (!getCodeGenOpts().LTOVisibilityPublicStd)
@@ -1239,13 +1240,9 @@
   if (!isExternallyVisible(LV.getLinkage()))
 return true;
 
-  if (getTriple().isOSBinFormatCOFF()) {
-if (RD->hasAttr() || RD->hasAttr())
-  return false;
-  } else {
-if (LV.getVisibility() != HiddenVisibility)
-  return false;
-  }
+  if (!getTriple().isOSBinFormatCOFF() &&
+  LV.getVisibility() != HiddenVisibility)
+return false;
 
   return !AlwaysHasLTOVisibilityPublic(RD);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129700: [clang] Don't emit type tests for dllexport/import classes

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.
Herald added a subscriber: bd1976llvm.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129700

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

[PATCH] D149187: [clang] Canonicalize system headers in dependency file when -canonical-prefixes

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Clang was writing paths to the dependency file that don't exist when using a 
sysroot with symlinks, causing everything to get rebuilt every time. This is 
reproducible on Linux by creating a symlink to '/', using that as the sysroot, 
and trying to build something with ninja that includes the C++ stdlib (e.g. a 
typical build of LLVM).

This fixes https://github.com/ninja-build/ninja/issues/1330 and somewhat 
matches gcc.

gcc canonicalizes system headers in dependency files under a 
-f[no-]canonical-system-headers, but it makes more sense to look at 
-canonical-prefixes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149187

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/DependencyOutputOptions.h
  clang/include/clang/Frontend/Utils.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/test/Driver/canonical-system-headers.c
  clang/test/Preprocessor/Inputs/canonical-system-headers/a.h
  clang/test/Preprocessor/canonical-system-headers.c

Index: clang/test/Preprocessor/canonical-system-headers.c
===
--- /dev/null
+++ clang/test/Preprocessor/canonical-system-headers.c
@@ -0,0 +1,16 @@
+// don't create symlinks on windows
+// UNSUPPORTED: system-windows
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/foo/
+// RUN: ln -f -s %S/Inputs/canonical-system-headers %t/foo/include
+// RUN: %clang_cc1 -isystem %T/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only
+// RUN: FileCheck %s --check-prefix=NOCANON --implicit-check-not=a.h < %t2
+// RUN: %clang_cc1 -isystem %T/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only -canonical-system-headers
+// RUN: FileCheck %s --check-prefix=CANON --implicit-check-not=a.h < %t2
+
+// NOCANON: foo/include/a.h
+// CANON: Inputs/canonical-system-headers/a.h
+
+#include 
Index: clang/test/Driver/canonical-system-headers.c
===
--- /dev/null
+++ clang/test/Driver/canonical-system-headers.c
@@ -0,0 +1,6 @@
+// RUN: %clang -MD -no-canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
+// RUN: %clang -MD -canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+// RUN: %clang -MD -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+
+// CHECK-YES: "-canonical-system-headers"
+// CHECK-NO-NOT: "-canonical-system-headers"
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -49,6 +49,7 @@
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
+  (),
   /*IsMissing*/ false);
   }
 
@@ -56,9 +57,11 @@
SrcMgr::CharacteristicKind FileType) override {
 StringRef Filename =
 llvm::sys::path::remove_leading_dotslash(SkippedFile.getName());
-DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+DepCollector.maybeAddDependency(Filename,
+/*FromModule=*/false,
 /*IsSystem=*/isSystem(FileType),
 /*IsModuleFile=*/false,
+(),
 /*IsMissing=*/false);
   }
 
@@ -69,9 +72,12 @@
   StringRef RelativePath, const Module *Imported,
   SrcMgr::CharacteristicKind FileType) override {
 if (!File)
-  DepCollector.maybeAddDependency(FileName, /*FromModule*/false,
- /*IsSystem*/false, /*IsModuleFile*/false,
- /*IsMissing*/true);
+  DepCollector.maybeAddDependency(FileName,
+  /*FromModule*/ false,
+  /*IsSystem*/ false,
+  /*IsModuleFile*/ false,
+  (),
+  /*IsMissing*/ true);
 // Files that actually exist are handled by FileChanged.
   }
 
@@ -82,9 +88,11 @@
   return;
 StringRef Filename =
 llvm::sys::path::remove_leading_dotslash(File->getName());
-DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+DepCollector.maybeAddDependency(Filename,
+/*FromModule=*/false,
 /*IsSystem=*/isSystem(FileType),
 /*IsModuleFile=*/false,
+

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

this regresses clang/test/Headers/mm_malloc.c (added here 
) because we now don't run instcombine before 
the inliner, and we lose return value alignment info when inlining. perhaps 
this is still a tradeoff worth making


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146764

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D145265#4225332 , @asbirlea wrote:

> Nit: the diffs would be easier to analyze with "-fno-discard-value-names".

done, see commit description

the IR diffs look pretty good now, I'm not seeing any major regressions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 513731.
aeubanks edited the summary of this revision.
aeubanks added a comment.

fix clang test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/test/Feature/optnone-opt.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-pgo-preinline.ll
  llvm/test/Other/new-pm-thinlto-postlink-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll
  llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon-opaque-ptr.ll
  llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
  llvm/test/Transforms/Coroutines/coro-retcon-value.ll
  llvm/test/Transforms/Coroutines/coro-retcon.ll
  llvm/test/Transforms/Coroutines/coro-swifterror.ll
  llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
  llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll

Index: llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
===
--- llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
+++ llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
@@ -16,10 +16,13 @@
 
 define void @caller1(i1 %c, ptr align 1 %ptr) {
 ; ASSUMPTIONS-OFF-LABEL: @caller1(
-; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-OFF-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-OFF:   false1:
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-OFF:   common.ret:
-; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR:%.*]], align 8
+; ASSUMPTIONS-OFF-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-OFF-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
@@ -27,15 +30,15 @@
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 -1, ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:ret void
-; ASSUMPTIONS-OFF:   false2:
-; ASSUMPTIONS-OFF-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-OFF-NEXT:br label [[COMMON_RET]]
 ;
 ; ASSUMPTIONS-ON-LABEL: @caller1(
-; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE2:%.*]]
+; ASSUMPTIONS-ON-NEXT:br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
+; ASSUMPTIONS-ON:   false1:
+; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR:%.*]], align 4
+; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ; ASSUMPTIONS-ON:   common.ret:
-; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE2]] ], [ 2, [[TMP0:%.*]] ]
-; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR:%.*]], i64 8) ]
+; ASSUMPTIONS-ON-NEXT:[[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
+; ASSUMPTIONS-ON-NEXT:call void @llvm.assume(i1 true) [ "align"(ptr [[PTR]], i64 8) ]
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 0, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
@@ -44,9 +47,6 @@
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 -1, ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:store volatile i64 [[DOTSINK]], ptr [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:ret void
-; ASSUMPTIONS-ON:   false2:
-; ASSUMPTIONS-ON-NEXT:store volatile i64 1, ptr [[PTR]], align 4
-; ASSUMPTIONS-ON-NEXT:br label [[COMMON_RET]]
 ;
   br i1 %c, label %true1, label %false1
 
Index: llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
===
--- llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
+++ llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll
@@ -1,11 +1,28 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -O3 -S < %s | FileCheck %s
 
+; FIXME: see the following issues for background
+;