r331806 - Change -foutline to -moutline
Author: paquette
Date: Tue May 8 13:53:19 2018
New Revision: 331806
URL: http://llvm.org/viewvc/llvm-project?rev=331806&view=rev
Log:
Change -foutline to -moutline
Nitpicky, but the MachineOutliner is a machine-level pass, and so we should
reflect that by using "m" instead of "n".
Figured we should get this in before people get used to the letter f. :)
Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
Modified: cfe/trunk/include/clang/Driver/Options.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=331806&r1=331805&r2=331806&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue May 8 13:53:19 2018
@@ -1317,8 +1317,6 @@ def fno_exceptions : Flag<["-"], "fno-ex
def fno_gnu_keywords : Flag<["-"], "fno-gnu-keywords">, Group,
Flags<[CC1Option]>;
def fno_inline_functions : Flag<["-"], "fno-inline-functions">,
Group, Flags<[CC1Option]>;
def fno_inline : Flag<["-"], "fno-inline">, Group,
Flags<[CC1Option]>;
-def foutline : Flag<["-"], "foutline">, Group,
Flags<[CC1Option]>,
-HelpText<"Enable function outlining (AArch64 only)">;
def fno_experimental_isel : Flag<["-"], "fno-experimental-isel">,
Group,
HelpText<"Disables the experimental global instruction selector">;
def fno_experimental_new_pass_manager : Flag<["-"],
"fno-experimental-new-pass-manager">,
@@ -1908,6 +1906,8 @@ def mmacos_version_min_EQ : Joined<["-"]
Group, Alias;
def mms_bitfields : Flag<["-"], "mms-bitfields">, Group,
Flags<[CC1Option]>,
HelpText<"Set the default structure layout to be compatible with the
Microsoft compiler standard">;
+def moutline : Flag<["-"], "moutline">, Group,
Flags<[CC1Option]>,
+HelpText<"Enable function outlining (AArch64 only)">;
def mno_ms_bitfields : Flag<["-"], "mno-ms-bitfields">, Group,
HelpText<"Do not set the default structure layout to be compatible with the
Microsoft compiler standard">;
def mstackrealign : Flag<["-"], "mstackrealign">, Group,
Flags<[CC1Option]>,
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=331806&r1=331805&r2=331806&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Tue May 8 13:53:19 2018
@@ -1485,7 +1485,7 @@ void Clang::AddAArch64TargetArgs(const A
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
- if (Args.getLastArg(options::OPT_foutline)) {
+ if (Args.getLastArg(options::OPT_moutline)) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-enable-machine-outliner");
}
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=331806&r1=331805&r2=331806&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Tue May 8 13:53:19 2018
@@ -1,4 +1,4 @@
// REQUIRES: aarch64-registered-target
-// RUN: %clang -target aarch64 -foutline -S %s -### 2>&1 | FileCheck %s
+// RUN: %clang -target aarch64 -moutline -S %s -### 2>&1 | FileCheck %s
// CHECK: "-mllvm" "-enable-machine-outliner"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r331810 - Add a mno-outline flag to disable the MachineOutliner
Author: paquette
Date: Tue May 8 13:58:32 2018
New Revision: 331810
URL: http://llvm.org/viewvc/llvm-project?rev=331810&view=rev
Log:
Add a mno-outline flag to disable the MachineOutliner
Since we're working on turning the MachineOutliner by default under -Oz for
AArch64, it makes sense to have an -mno-outline flag available. This currently
doesn't do much (it basically just undoes -moutline).
When the MachineOutliner is on by default under AArch64, this flag should
set -mllvm -enable-machine-outliner=never.
Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
Modified: cfe/trunk/include/clang/Driver/Options.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=331810&r1=331809&r2=331810&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue May 8 13:58:32 2018
@@ -1908,6 +1908,8 @@ def mms_bitfields : Flag<["-"], "mms-bit
HelpText<"Set the default structure layout to be compatible with the
Microsoft compiler standard">;
def moutline : Flag<["-"], "moutline">, Group,
Flags<[CC1Option]>,
HelpText<"Enable function outlining (AArch64 only)">;
+def mno_outline : Flag<["-"], "mno-outline">, Group,
Flags<[CC1Option]>,
+HelpText<"Disable function outlining (AArch64 only)">;
def mno_ms_bitfields : Flag<["-"], "mno-ms-bitfields">, Group,
HelpText<"Do not set the default structure layout to be compatible with the
Microsoft compiler standard">;
def mstackrealign : Flag<["-"], "mstackrealign">, Group,
Flags<[CC1Option]>,
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=331810&r1=331809&r2=331810&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Tue May 8 13:58:32 2018
@@ -1485,7 +1485,8 @@ void Clang::AddAArch64TargetArgs(const A
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
- if (Args.getLastArg(options::OPT_moutline)) {
+ if (!Args.hasArg(options::OPT_mno_outline) &&
+ Args.getLastArg(options::OPT_moutline)) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-enable-machine-outliner");
}
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=331810&r1=331809&r2=331810&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Tue May 8 13:58:32 2018
@@ -1,4 +1,9 @@
// REQUIRES: aarch64-registered-target
-// RUN: %clang -target aarch64 -moutline -S %s -### 2>&1 | FileCheck %s
-// CHECK: "-mllvm" "-enable-machine-outliner"
+// RUN: %clang -target aarch64 -moutline -S %s -### 2>&1 | FileCheck %s
-check-prefix=ON
+// ON: "-mllvm" "-enable-machine-outliner"
+
+// RUN: %clang -target aarch64 -moutline -mno-outline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF1
+// RUN: %clang -target aarch64 -mno-outline -moutline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF2
+// OFF1-NOT: "-mllvm" "-enable-machine-outliner"
+// OFF2-NOT: "-mllvm" "-enable-machine-outliner"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r357031 - Make -mno-outline pass -enable-machine-outliner=never to ld in LTO
Author: paquette
Date: Tue Mar 26 14:22:42 2019
New Revision: 357031
URL: http://llvm.org/viewvc/llvm-project?rev=357031&view=rev
Log:
Make -mno-outline pass -enable-machine-outliner=never to ld in LTO
Since AArch64 has default outlining behaviour, we need to make sure that
-mno-outline is actually passed along to the linker in this case. Otherwise,
it will run by default on minsize functions even when -mno-outline is specified.
Also fix the darwin-ld test for this, which wasn't actually doing anything.
Modified:
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/test/Driver/darwin-ld.c
Modified: cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Darwin.cpp?rev=357031&r1=357030&r2=357031&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Darwin.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Darwin.cpp Tue Mar 26 14:22:42 2019
@@ -494,14 +494,23 @@ void darwin::Linker::ConstructJob(Compil
}
// Propagate the -moutline flag to the linker in LTO.
- if (Args.hasFlag(options::OPT_moutline, options::OPT_mno_outline, false)) {
-if (getMachOToolChain().getMachOArchName(Args) == "arm64") {
- CmdArgs.push_back("-mllvm");
- CmdArgs.push_back("-enable-machine-outliner");
+ if (Arg *A =
+ Args.getLastArg(options::OPT_moutline, options::OPT_mno_outline)) {
+if (A->getOption().matches(options::OPT_moutline)) {
+ if (getMachOToolChain().getMachOArchName(Args) == "arm64") {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-machine-outliner");
- // Outline from linkonceodr functions by default in LTO.
+// Outline from linkonceodr functions by default in LTO.
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-linkonceodr-outlining");
+ }
+} else {
+ // Disable all outlining behaviour if we have mno-outline. We need to do
+ // this explicitly, because targets which support default outlining will
+ // try to do work if we don't.
CmdArgs.push_back("-mllvm");
- CmdArgs.push_back("-enable-linkonceodr-outlining");
+ CmdArgs.push_back("-enable-machine-outliner=never");
}
}
Modified: cfe/trunk/test/Driver/darwin-ld.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/darwin-ld.c?rev=357031&r1=357030&r2=357031&view=diff
==
--- cfe/trunk/test/Driver/darwin-ld.c (original)
+++ cfe/trunk/test/Driver/darwin-ld.c Tue Mar 26 14:22:42 2019
@@ -372,5 +372,11 @@
// Check that we can pass the outliner down to the linker.
// RUN: env IPHONEOS_DEPLOYMENT_TARGET=7.0 \
// RUN: %clang -target arm64-apple-darwin -moutline -### %t.o 2> %t.log
-// MOUTLINE: ld
+// RUN: FileCheck -check-prefix=MOUTLINE %s < %t.log
+// MOUTLINE: {{ld(.exe)?"}}
// MOUTLINE-SAME: "-mllvm" "-enable-machine-outliner" "-mllvm"
"-enable-linkonceodr-outlining"
+// RUN: env IPHONEOS_DEPLOYMENT_TARGET=7.0 \
+// RUN: %clang -target arm64-apple-darwin -mno-outline -### %t.o 2> %t.log
+// RUN: FileCheck -check-prefix=MNO_OUTLINE %s < %t.log
+// MNO_OUTLINE: {{ld(.exe)?"}}
+// MNO_OUTLINE-SAME: "-mllvm" "-enable-machine-outliner=never"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r331370 - Add -foutline option to enable the MachineOutliner in AArch64
Author: paquette
Date: Wed May 2 09:42:51 2018
New Revision: 331370
URL: http://llvm.org/viewvc/llvm-project?rev=331370&view=rev
Log:
Add -foutline option to enable the MachineOutliner in AArch64
Since we've been working on productizing the MachineOutliner in AArch64, it
makes sense to provide a more user-friendly way to enable it.
This allows users of AArch64 to enable the outliner using -foutline instead
of -mllvm -enable-machine-outliner. Other, less mature implementations (e.g,
x86-64) can still enable the pass using the -mllvm option.
Also add a test to make sure it works.
Added:
cfe/trunk/test/Driver/aarch64-outliner.c
Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
Modified: cfe/trunk/include/clang/Driver/Options.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=331370&r1=331369&r2=331370&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Wed May 2 09:42:51 2018
@@ -1317,6 +1317,8 @@ def fno_exceptions : Flag<["-"], "fno-ex
def fno_gnu_keywords : Flag<["-"], "fno-gnu-keywords">, Group,
Flags<[CC1Option]>;
def fno_inline_functions : Flag<["-"], "fno-inline-functions">,
Group, Flags<[CC1Option]>;
def fno_inline : Flag<["-"], "fno-inline">, Group,
Flags<[CC1Option]>;
+def foutline : Flag<["-"], "foutline">, Group,
Flags<[CC1Option]>,
+HelpText<"Enable function outlining (AArch64 only)">;
def fno_experimental_isel : Flag<["-"], "fno-experimental-isel">,
Group,
HelpText<"Disables the experimental global instruction selector">;
def fno_experimental_new_pass_manager : Flag<["-"],
"fno-experimental-new-pass-manager">,
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=331370&r1=331369&r2=331370&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Wed May 2 09:42:51 2018
@@ -1484,6 +1484,11 @@ void Clang::AddAArch64TargetArgs(const A
else
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
+
+ if (Arg *A = Args.getLastArg(options::OPT_foutline)) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-machine-outliner");
+ }
}
void Clang::AddMIPSTargetArgs(const ArgList &Args,
Added: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=331370&view=auto
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (added)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Wed May 2 09:42:51 2018
@@ -0,0 +1,4 @@
+// REQUIRES: aarch64-registered-target
+
+// RUN: %clang -target aarch64 -foutline -S %s -### 2>&1 | FileCheck %s
+// CHECK: "-mllvm" "-enable-machine-outliner"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r331370 - Add -foutline option to enable the MachineOutliner in AArch64
Hmm yeah, you’re right, especially considering D45916. It would be nice to have
the desired behaviour just “fall out” of that patch. I’ll write a patch adding
the flag.
- Jessica
> On May 2, 2018, at 2:48 PM, Friedman, Eli wrote:
>
> On 5/2/2018 9:42 AM, Jessica Paquette via cfe-commits wrote:
>> Author: paquette
>> Date: Wed May 2 09:42:51 2018
>> New Revision: 331370
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=331370&view=rev
>> Log:
>> Add -foutline option to enable the MachineOutliner in AArch64
>
> It probably makes sense to also have a -fno-outline option.
>
> -Eli
>
>>
>> Since we've been working on productizing the MachineOutliner in AArch64, it
>> makes sense to provide a more user-friendly way to enable it.
>>
>> This allows users of AArch64 to enable the outliner using -foutline instead
>> of -mllvm -enable-machine-outliner. Other, less mature implementations (e.g,
>> x86-64) can still enable the pass using the -mllvm option.
>>
>> Also add a test to make sure it works.
>>
>> Added:
>> cfe/trunk/test/Driver/aarch64-outliner.c
>> Modified:
>> cfe/trunk/include/clang/Driver/Options.td
>> cfe/trunk/lib/Driver/ToolChains/Clang.cpp
>>
>> Modified: cfe/trunk/include/clang/Driver/Options.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=331370&r1=331369&r2=331370&view=diff
>> ==
>> --- cfe/trunk/include/clang/Driver/Options.td (original)
>> +++ cfe/trunk/include/clang/Driver/Options.td Wed May 2 09:42:51 2018
>> @@ -1317,6 +1317,8 @@ def fno_exceptions : Flag<["-"], "fno-ex
>> def fno_gnu_keywords : Flag<["-"], "fno-gnu-keywords">, Group,
>> Flags<[CC1Option]>;
>> def fno_inline_functions : Flag<["-"], "fno-inline-functions">,
>> Group, Flags<[CC1Option]>;
>> def fno_inline : Flag<["-"], "fno-inline">, Group,
>> Flags<[CC1Option]>;
>> +def foutline : Flag<["-"], "foutline">, Group,
>> Flags<[CC1Option]>,
>> +HelpText<"Enable function outlining (AArch64 only)">;
>> def fno_experimental_isel : Flag<["-"], "fno-experimental-isel">,
>> Group,
>>HelpText<"Disables the experimental global instruction selector">;
>> def fno_experimental_new_pass_manager : Flag<["-"],
>> "fno-experimental-new-pass-manager">,
>>
>> Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=331370&r1=331369&r2=331370&view=diff
>> ==
>> --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
>> +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Wed May 2 09:42:51 2018
>> @@ -1484,6 +1484,11 @@ void Clang::AddAArch64TargetArgs(const A
>> else
>>CmdArgs.push_back("-aarch64-enable-global-merge=true");
>>}
>> +
>> + if (Arg *A = Args.getLastArg(options::OPT_foutline)) {
>> +CmdArgs.push_back("-mllvm");
>> +CmdArgs.push_back("-enable-machine-outliner");
>> + }
>> }
>>void Clang::AddMIPSTargetArgs(const ArgList &Args,
>>
>> Added: cfe/trunk/test/Driver/aarch64-outliner.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=331370&view=auto
>> ==
>> --- cfe/trunk/test/Driver/aarch64-outliner.c (added)
>> +++ cfe/trunk/test/Driver/aarch64-outliner.c Wed May 2 09:42:51 2018
>> @@ -0,0 +1,4 @@
>> +// REQUIRES: aarch64-registered-target
>> +
>> +// RUN: %clang -target aarch64 -foutline -S %s -### 2>&1 | FileCheck %s
>> +// CHECK: "-mllvm" "-enable-machine-outliner"
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r335503 - [MachineOutliner] Make last of -moutline/-mno-outline win
Author: paquette
Date: Mon Jun 25 10:27:51 2018
New Revision: 335503
URL: http://llvm.org/viewvc/llvm-project?rev=335503&view=rev
Log:
[MachineOutliner] Make last of -moutline/-mno-outline win
The expected behaviour of command-line flags to clang is to have
the last of -m(whatever) and -mno-(whatever) win. The outliner
didn't do that. This fixes that and updates the test.
Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=335503&r1=335502&r2=335503&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Mon Jun 25 10:27:51 2018
@@ -1477,10 +1477,12 @@ void Clang::AddAArch64TargetArgs(const A
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
- if (!Args.hasArg(options::OPT_mno_outline) &&
- Args.getLastArg(options::OPT_moutline)) {
-CmdArgs.push_back("-mllvm");
-CmdArgs.push_back("-enable-machine-outliner");
+ if (Arg *A = Args.getLastArg(options::OPT_moutline,
+ options::OPT_mno_outline)) {
+if (A->getOption().matches(options::OPT_moutline)) {
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-machine-outliner");
+}
}
}
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=335503&r1=335502&r2=335503&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Mon Jun 25 10:27:51 2018
@@ -1,9 +1,5 @@
// REQUIRES: aarch64-registered-target
-
// RUN: %clang -target aarch64 -moutline -S %s -### 2>&1 | FileCheck %s
-check-prefix=ON
// ON: "-mllvm" "-enable-machine-outliner"
-
-// RUN: %clang -target aarch64 -moutline -mno-outline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF1
-// RUN: %clang -target aarch64 -mno-outline -moutline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF2
-// OFF1-NOT: "-mllvm" "-enable-machine-outliner"
-// OFF2-NOT: "-mllvm" "-enable-machine-outliner"
+// RUN: %clang -target aarch64 -moutline -mno-outline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF
+// OFF-NOT: "-mllvm" "-enable-machine-outliner"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r335504 - [MachineOutliner] Outline from linkonceodrs by default in LTO when -moutline is passed
Author: paquette
Date: Mon Jun 25 10:36:05 2018
New Revision: 335504
URL: http://llvm.org/viewvc/llvm-project?rev=335504&view=rev
Log:
[MachineOutliner] Outline from linkonceodrs by default in LTO when -moutline is
passed
Pass -enable-linkonceodr-outlining by default when LTO is enabled.
The outliner shouldn't compete with any sort of linker deduplication
on linkonceodr functions when LTO is enabled. Therefore, this behaviour
should be the default.
Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=335504&r1=335503&r2=335504&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Mon Jun 25 10:36:05 2018
@@ -1482,6 +1482,14 @@ void Clang::AddAArch64TargetArgs(const A
if (A->getOption().matches(options::OPT_moutline)) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-enable-machine-outliner");
+
+ // The outliner shouldn't compete with linkers that dedupe linkonceodr
+ // functions in LTO. Enable that behaviour by default when compiling with
+ // LTO.
+ if (getToolChain().getDriver().isUsingLTO()) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-linkonceodr-outlining");
+ }
}
}
}
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=335504&r1=335503&r2=335504&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Mon Jun 25 10:36:05 2018
@@ -3,3 +3,7 @@
// ON: "-mllvm" "-enable-machine-outliner"
// RUN: %clang -target aarch64 -moutline -mno-outline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF
// OFF-NOT: "-mllvm" "-enable-machine-outliner"
+// RUN: %clang -target aarch64 -moutline -flto=thin -S %s -### 2>&1 |
FileCheck %s -check-prefix=FLTO
+// FLTO: "-mllvm" "-enable-linkonceodr-outlining"
+// RUN: %clang -target aarch64 -moutline -flto=full -S %s -### 2>&1 |
FileCheck %s -check-prefix=TLTO
+// TLTO: "-mllvm" "-enable-linkonceodr-outlining"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r335549 - [MachineOutliner] NFC - simplify -moutline/-mno-outline logic
Author: paquette
Date: Mon Jun 25 16:20:18 2018
New Revision: 335549
URL: http://llvm.org/viewvc/llvm-project?rev=335549&view=rev
Log:
[MachineOutliner] NFC - simplify -moutline/-mno-outline logic
It's a bit cleaner to use `hasFlag` instead of nested ifs. This
just refactors the -moutline/-mno-outline logic to use that.
Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=335549&r1=335548&r2=335549&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Mon Jun 25 16:20:18 2018
@@ -1477,19 +1477,16 @@ void Clang::AddAArch64TargetArgs(const A
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
- if (Arg *A = Args.getLastArg(options::OPT_moutline,
- options::OPT_mno_outline)) {
-if (A->getOption().matches(options::OPT_moutline)) {
- CmdArgs.push_back("-mllvm");
- CmdArgs.push_back("-enable-machine-outliner");
+ if (Args.hasFlag(options::OPT_moutline, options::OPT_mno_outline, false)) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-machine-outliner");
- // The outliner shouldn't compete with linkers that dedupe linkonceodr
- // functions in LTO. Enable that behaviour by default when compiling with
- // LTO.
- if (getToolChain().getDriver().isUsingLTO()) {
-CmdArgs.push_back("-mllvm");
-CmdArgs.push_back("-enable-linkonceodr-outlining");
- }
+// The outliner shouldn't compete with linkers that dedupe linkonceodr
+// functions in LTO. Enable that behaviour by default when compiling with
+// LTO.
+if (getToolChain().getDriver().isUsingLTO()) {
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-linkonceodr-outlining");
}
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r335672 - [MachineOutliner] Emit a warning when using -moutline on unsupported targets
Author: paquette
Date: Tue Jun 26 15:09:48 2018
New Revision: 335672
URL: http://llvm.org/viewvc/llvm-project?rev=335672&view=rev
Log:
[MachineOutliner] Emit a warning when using -moutline on unsupported targets
Instead of just saying "flag unused", we should tell the user that the
outliner isn't (at least officially) supported for some given architecture.
This adds a warning that will state something like
The 'blah' architecture does not support -moutline; flag ignored
when we call -moutline with the 'blah' architecture.
Since the outliner is still mostly an AArch64 thing, any architecture
other than AArch64 will emit this warning.
Modified:
cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
Modified: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td?rev=335672&r1=335671&r2=335672&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td Tue Jun 26 15:09:48
2018
@@ -385,4 +385,8 @@ def warn_drv_experimental_isel_incomplet
def warn_drv_experimental_isel_incomplete_opt : Warning<
"-fexperimental-isel support is incomplete for this architecture at the
current optimization level">,
InGroup;
+
+def warn_drv_moutline_unsupported_opt : Warning<
+ "The '%0' architecture does not support -moutline; flag ignored">,
+ InGroup;
}
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=335672&r1=335671&r2=335672&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Tue Jun 26 15:09:48 2018
@@ -1476,19 +1476,6 @@ void Clang::AddAArch64TargetArgs(const A
else
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
-
- if (Args.hasFlag(options::OPT_moutline, options::OPT_mno_outline, false)) {
-CmdArgs.push_back("-mllvm");
-CmdArgs.push_back("-enable-machine-outliner");
-
-// The outliner shouldn't compete with linkers that dedupe linkonceodr
-// functions in LTO. Enable that behaviour by default when compiling with
-// LTO.
-if (getToolChain().getDriver().isUsingLTO()) {
- CmdArgs.push_back("-mllvm");
- CmdArgs.push_back("-enable-linkonceodr-outlining");
-}
- }
}
void Clang::AddMIPSTargetArgs(const ArgList &Args,
@@ -4814,6 +4801,26 @@ void Clang::ConstructJob(Compilation &C,
options::OPT_fno_complete_member_pointers, false))
CmdArgs.push_back("-fcomplete-member-pointers");
+ if (Args.hasFlag(options::OPT_moutline, options::OPT_mno_outline, false)) {
+// We only support -moutline in AArch64 right now. If we're not compiling
+// for AArch64, emit a warning and ignore the flag. Otherwise, add the
+// proper mllvm flags.
+if (Triple.getArch() != llvm::Triple::aarch64) {
+ D.Diag(diag::warn_drv_moutline_unsupported_opt) << Triple.getArchName();
+} else {
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-machine-outliner");
+
+ // The outliner shouldn't compete with linkers that dedupe linkonceodr
+ // functions in LTO. Enable that behaviour by default when compiling with
+ // LTO.
+ if (getToolChain().getDriver().isUsingLTO()) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-linkonceodr-outlining");
+ }
+}
+ }
+
// Finally add the compile command to the compilation.
if (Args.hasArg(options::OPT__SLASH_fallback) &&
Output.getType() == types::TY_Object &&
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=335672&r1=335671&r2=335672&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Tue Jun 26 15:09:48 2018
@@ -7,3 +7,6 @@
// FLTO: "-mllvm" "-enable-linkonceodr-outlining"
// RUN: %clang -target aarch64 -moutline -flto=full -S %s -### 2>&1 |
FileCheck %s -check-prefix=TLTO
// TLTO: "-mllvm" "-enable-linkonceodr-outlining"
+// RUN: %clang -target x86_64 -moutline -S %s -### 2>&1 | FileCheck %s
-check-prefix=WARN
+// WARN: warning: The 'x86_64' architecture does not support -moutline; flag
ignored [-Woption-ignored]
+// WARN-NOT: "-mllvm" "-enable-machine-outliner"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r336001 - [MachineOutliner] Make -mno-outline use -enable-machine-outliner=never
Author: paquette
Date: Fri Jun 29 11:06:10 2018
New Revision: 336001
URL: http://llvm.org/viewvc/llvm-project?rev=336001&view=rev
Log:
[MachineOutliner] Make -mno-outline use -enable-machine-outliner=never
This updates -mno-outline so that it passes -enable-machine-outliner=never
instead of nothing. This puts it in sync with the behaviour in llc and
other tools.
Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=336001&r1=336000&r2=336001&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Fri Jun 29 11:06:10 2018
@@ -4797,23 +4797,30 @@ void Clang::ConstructJob(Compilation &C,
options::OPT_fno_complete_member_pointers, false))
CmdArgs.push_back("-fcomplete-member-pointers");
- if (Args.hasFlag(options::OPT_moutline, options::OPT_mno_outline, false)) {
-// We only support -moutline in AArch64 right now. If we're not compiling
-// for AArch64, emit a warning and ignore the flag. Otherwise, add the
-// proper mllvm flags.
-if (Triple.getArch() != llvm::Triple::aarch64) {
- D.Diag(diag::warn_drv_moutline_unsupported_opt) << Triple.getArchName();
-} else {
- CmdArgs.push_back("-mllvm");
- CmdArgs.push_back("-enable-machine-outliner");
-
- // The outliner shouldn't compete with linkers that dedupe linkonceodr
- // functions in LTO. Enable that behaviour by default when compiling with
- // LTO.
- if (getToolChain().getDriver().isUsingLTO()) {
+ if (Arg *A = Args.getLastArg(options::OPT_moutline,
+ options::OPT_mno_outline)) {
+if (A->getOption().matches(options::OPT_moutline)) {
+ // We only support -moutline in AArch64 right now. If we're not compiling
+ // for AArch64, emit a warning and ignore the flag. Otherwise, add the
+ // proper mllvm flags.
+ if (Triple.getArch() != llvm::Triple::aarch64) {
+D.Diag(diag::warn_drv_moutline_unsupported_opt) <<
Triple.getArchName();
+ } else {
CmdArgs.push_back("-mllvm");
-CmdArgs.push_back("-enable-linkonceodr-outlining");
+CmdArgs.push_back("-enable-machine-outliner");
+
+// The outliner shouldn't compete with linkers that dedupe linkonceodr
+// functions in LTO. Enable that behaviour by default when compiling
with
+// LTO.
+if (getToolChain().getDriver().isUsingLTO()) {
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-linkonceodr-outlining");
+}
}
+} else {
+ // Disable all outlining behaviour.
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-machine-outliner=never");
}
}
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=336001&r1=336000&r2=336001&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Fri Jun 29 11:06:10 2018
@@ -2,7 +2,7 @@
// RUN: %clang -target aarch64 -moutline -S %s -### 2>&1 | FileCheck %s
-check-prefix=ON
// ON: "-mllvm" "-enable-machine-outliner"
// RUN: %clang -target aarch64 -moutline -mno-outline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF
-// OFF-NOT: "-mllvm" "-enable-machine-outliner"
+// OFF: "-mllvm" "-enable-machine-outliner=never"
// RUN: %clang -target aarch64 -moutline -flto=thin -S %s -### 2>&1 |
FileCheck %s -check-prefix=FLTO
// FLTO: "-mllvm" "-enable-linkonceodr-outlining"
// RUN: %clang -target aarch64 -moutline -flto=full -S %s -### 2>&1 |
FileCheck %s -check-prefix=TLTO
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r336471 - [MachineOutliner] Properly pass -moutline along to the toolchain
Author: paquette
Date: Fri Jul 6 15:24:56 2018
New Revision: 336471
URL: http://llvm.org/viewvc/llvm-project?rev=336471&view=rev
Log:
[MachineOutliner] Properly pass -moutline along to the toolchain
This moves the LTO-specific code for outlining from ToolChains/Clang.cpp to
ToolChains/Darwin.cpp. Passing -mllvm flags isn't sufficient for making sure
that the specified pass will actually run in LTO. This makes sure that when
-moutline is passed, the MachineOutliner will actually be added to the LTO
pass pipeline as expected.
Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/test/Driver/aarch64-outliner.c
cfe/trunk/test/Driver/darwin-ld.c
Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=336471&r1=336470&r2=336471&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Fri Jul 6 15:24:56 2018
@@ -4757,16 +4757,8 @@ void Clang::ConstructJob(Compilation &C,
if (Triple.getArch() != llvm::Triple::aarch64) {
D.Diag(diag::warn_drv_moutline_unsupported_opt) <<
Triple.getArchName();
} else {
-CmdArgs.push_back("-mllvm");
-CmdArgs.push_back("-enable-machine-outliner");
-
-// The outliner shouldn't compete with linkers that dedupe linkonceodr
-// functions in LTO. Enable that behaviour by default when compiling
with
-// LTO.
-if (getToolChain().getDriver().isUsingLTO()) {
CmdArgs.push_back("-mllvm");
- CmdArgs.push_back("-enable-linkonceodr-outlining");
-}
+ CmdArgs.push_back("-enable-machine-outliner");
}
} else {
// Disable all outlining behaviour.
Modified: cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Darwin.cpp?rev=336471&r1=336470&r2=336471&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Darwin.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Darwin.cpp Fri Jul 6 15:24:56 2018
@@ -479,6 +479,18 @@ void darwin::Linker::ConstructJob(Compil
}
}
+ // Propagate the -moutline flag to the linker in LTO.
+ if (Args.hasFlag(options::OPT_moutline, options::OPT_mno_outline, false)) {
+if (getMachOToolChain().getMachOArchName(Args) == "arm64") {
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-machine-outliner");
+
+ // Outline from linkonceodr functions by default in LTO.
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("-enable-linkonceodr-outlining");
+}
+ }
+
// It seems that the 'e' option is completely ignored for dynamic executables
// (the default), and with static executables, the last one wins, as
expected.
Args.AddAllArgs(CmdArgs, {options::OPT_d_Flag, options::OPT_s,
options::OPT_t,
Modified: cfe/trunk/test/Driver/aarch64-outliner.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=336471&r1=336470&r2=336471&view=diff
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (original)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Fri Jul 6 15:24:56 2018
@@ -3,10 +3,6 @@
// ON: "-mllvm" "-enable-machine-outliner"
// RUN: %clang -target aarch64 -moutline -mno-outline -S %s -### 2>&1 |
FileCheck %s -check-prefix=OFF
// OFF: "-mllvm" "-enable-machine-outliner=never"
-// RUN: %clang -target aarch64 -moutline -flto=thin -S %s -### 2>&1 |
FileCheck %s -check-prefix=FLTO
-// FLTO: "-mllvm" "-enable-linkonceodr-outlining"
-// RUN: %clang -target aarch64 -moutline -flto=full -S %s -### 2>&1 |
FileCheck %s -check-prefix=TLTO
-// TLTO: "-mllvm" "-enable-linkonceodr-outlining"
// RUN: %clang -target x86_64 -moutline -S %s -### 2>&1 | FileCheck %s
-check-prefix=WARN
// WARN: warning: The 'x86_64' architecture does not support -moutline; flag
ignored [-Woption-ignored]
// WARN-NOT: "-mllvm" "-enable-machine-outliner"
Modified: cfe/trunk/test/Driver/darwin-ld.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/darwin-ld.c?rev=336471&r1=336470&r2=336471&view=diff
==
--- cfe/trunk/test/Driver/darwin-ld.c (original)
+++ cfe/trunk/test/Driver/darwin-ld.c Fri Jul 6 15:24:56 2018
@@ -371,3 +371,9 @@
// RUN: %clang -target x86_64-apple-darwin12 -fprofile-instr-generate -###
%t.o 2> %t.log
// RUN: FileCheck -check-prefix=NO_PROFILE_EXPORT %s < %t.log
// NO_PROFILE_EXPORT-NOT: "-exported_symbol"
+//
+// Check that we can pass the outliner down to the linker.
+// RUN: env IPHONEOS_DEPLOYMENT_TARGET=7.0 \
+// RUN: %clang -target arm64-apple-darwin -moutline -### %t.o 2> %t.log
+// MOUTLI
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
https://github.com/ornata edited https://github.com/llvm/llvm-project/pull/77214 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -98,3 +104,49 @@ int main() {// CHECK: File 0,
[[@LINE]]:12 -> {{[0-9]+}}:2 =
void ternary() {
true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 ->
}
+
+// FIXME: Do not generate coverage for discarded branches in if consteval
+// GH-57377
+// CHECK-LABEL: _Z16check_constevalAi:
+constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
+ if consteval {
+i *= 3; // CHECK-NEXT: [[@LINE-1]]:16 ->
[[@LINE+1]]:4 = #1
+ } else { // CHECK-NEXT: Gap,File 0, [[@LINE]]:4
-> [[@LINE]]:10 = #0
+i *= 5; // CHECK-NEXT: [[@LINE-1]]:10 ->
[[@LINE+1]]:4 = #0
+ }
+ return i; // CHECK-NEXT: [[@LINE]]:3 ->
[[@LINE]]:11 = (#0 + #1)
+}
+
+// CHECK-LABEL: _Z16check_constevalBi:
+constexpr int check_constevalB(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
ornata wrote:
Maybe `check_if_not_consteval_with_discarded_branch` would be a more
descriptive name?
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -7732,7 +7732,11 @@ TreeTransform::TransformIfStmt(IfStmt *S) {
if (Then.isInvalid())
return StmtError();
} else {
-Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
+// Discarded branch is replaced with empty CompoundStmt so we can keep
+// proper source location for start and end of original branch, so
+// subsequent transformations like CoverageMapping work properly
ornata wrote:
In the test, you mention that we're generating coverage for the discarded
regions. Is the reason that we're generating a `CompoundStmt` here? Could this
use a `FIXME`?
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -98,3 +104,49 @@ int main() {// CHECK: File 0,
[[@LINE]]:12 -> {{[0-9]+}}:2 =
void ternary() {
true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 ->
}
+
+// FIXME: Do not generate coverage for discarded branches in if consteval
+// GH-57377
+// CHECK-LABEL: _Z16check_constevalAi:
+constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
+ if consteval {
+i *= 3; // CHECK-NEXT: [[@LINE-1]]:16 ->
[[@LINE+1]]:4 = #1
+ } else { // CHECK-NEXT: Gap,File 0, [[@LINE]]:4
-> [[@LINE]]:10 = #0
+i *= 5; // CHECK-NEXT: [[@LINE-1]]:10 ->
[[@LINE+1]]:4 = #0
ornata wrote:
Specifically, it's the `else` region that shouldn't generate any coverage
mapping data, right?
It'd be good to mention that in the `FIXME`; something like
> FIXME: Do not generate coverage for discarded branches in if consteval. The
> else case is discarded and should not be covered.
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
https://github.com/ornata commented: Added some comments, mostly nits on the test. https://github.com/llvm/llvm-project/pull/77214 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -23,19 +23,29 @@ void foo() {// CHECK-NEXT: Gap,File 0,
[[@LINE+1]]:21 -> [[@
} // CHECK-NEXT: [[@LINE-2]]:9 -> [[@LINE-1]]:5
= #1
// CHECK-NEXT: [[@LINE-2]]:5 -> [[@LINE-2]]:8
= #1
-// FIXME: Do not generate coverage for discarded branches in if consteval and
if constexpr statements
-constexpr int check_consteval(int i) {
-if consteval {
- i++;
-}
-if !consteval {
- i++;
-}
-if consteval {
-return 42;
-} else {
-return i;
-}
+// FIXME: Do not generate coverage for discarded branches in if constexpr
+// CHECK-LABEL: _Z16check_constexprAi:
+int check_constexprA(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 =
#0
ornata wrote:
I think this test should be named `check_if_constexpr_true` or something similar
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -7741,6 +7745,10 @@ TreeTransform::TransformIfStmt(IfStmt *S) {
Else = getDerived().TransformStmt(S->getElse());
if (Else.isInvalid())
return StmtError();
+ } else if (S->getElse() && ConstexprConditionValue &&
ornata wrote:
Could you add a comment similar to the one in the `else` above?
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -98,3 +104,49 @@ int main() {// CHECK: File 0,
[[@LINE]]:12 -> {{[0-9]+}}:2 =
void ternary() {
true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 ->
}
+
+// FIXME: Do not generate coverage for discarded branches in if consteval
+// GH-57377
+// CHECK-LABEL: _Z16check_constevalAi:
+constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
+ if consteval {
+i *= 3; // CHECK-NEXT: [[@LINE-1]]:16 ->
[[@LINE+1]]:4 = #1
+ } else { // CHECK-NEXT: Gap,File 0, [[@LINE]]:4
-> [[@LINE]]:10 = #0
+i *= 5; // CHECK-NEXT: [[@LINE-1]]:10 ->
[[@LINE+1]]:4 = #0
+ }
+ return i; // CHECK-NEXT: [[@LINE]]:3 ->
[[@LINE]]:11 = (#0 + #1)
+}
+
+// CHECK-LABEL: _Z16check_constevalBi:
+constexpr int check_constevalB(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
ornata wrote:
Also does this one need a FIXME like in the above test?
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -23,19 +23,29 @@ void foo() {// CHECK-NEXT: Gap,File 0,
[[@LINE+1]]:21 -> [[@
} // CHECK-NEXT: [[@LINE-2]]:9 -> [[@LINE-1]]:5
= #1
// CHECK-NEXT: [[@LINE-2]]:5 -> [[@LINE-2]]:8
= #1
-// FIXME: Do not generate coverage for discarded branches in if consteval and
if constexpr statements
-constexpr int check_consteval(int i) {
-if consteval {
- i++;
-}
-if !consteval {
- i++;
-}
-if consteval {
-return 42;
-} else {
-return i;
-}
+// FIXME: Do not generate coverage for discarded branches in if constexpr
+// CHECK-LABEL: _Z16check_constexprAi:
+int check_constexprA(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 =
#0
+// CHECK-NEXT: [[@LINE+2]]:16 ->
[[@LINE+2]]:20 = #0
+// CHECK-NEXT: Branch,File 0, [[@LINE+1]]:16
-> [[@LINE+1]]:20 = 0, 0
+ if constexpr(true) { // CHECK-NEXT: Gap,File 0, [[@LINE]]:21 ->
[[@LINE]]:22 = #1
+i *= 3; // CHECK-NEXT: [[@LINE-1]]:22 -> [[@LINE+1]]:4
= #1
+ } else { // CHECK-NEXT: Gap,File 0, [[@LINE]]:4 ->
[[@LINE]]:10 = (#0 - #1)
+i *= 5; // CHECK-NEXT: [[@LINE-1]]:10 -> [[@LINE+1]]:4
= (#0 - #1)
+ }
+ return i;
+}
+
+// CHECK-LABEL: _Z16check_constexprBi:
+int check_constexprB(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 =
#0
ornata wrote:
Similarly, this would be better-named something like `check_if_constexpr_false`
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -98,3 +104,49 @@ int main() {// CHECK: File 0,
[[@LINE]]:12 -> {{[0-9]+}}:2 =
void ternary() {
true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 ->
}
+
+// FIXME: Do not generate coverage for discarded branches in if consteval
+// GH-57377
+// CHECK-LABEL: _Z16check_constevalAi:
+constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
+ if consteval {
+i *= 3; // CHECK-NEXT: [[@LINE-1]]:16 ->
[[@LINE+1]]:4 = #1
+ } else { // CHECK-NEXT: Gap,File 0, [[@LINE]]:4
-> [[@LINE]]:10 = #0
+i *= 5; // CHECK-NEXT: [[@LINE-1]]:10 ->
[[@LINE+1]]:4 = #0
+ }
+ return i; // CHECK-NEXT: [[@LINE]]:3 ->
[[@LINE]]:11 = (#0 + #1)
+}
+
+// CHECK-LABEL: _Z16check_constevalBi:
+constexpr int check_constevalB(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
+ if !consteval {
+i *= 3; // CHECK-NEXT: [[@LINE-1]]:17 ->
[[@LINE+1]]:4 = #0
+ } else { // CHECK-NEXT: Gap,File 0, [[@LINE]]:4
-> [[@LINE]]:10 = 0
+i *= 5; // CHECK-NEXT: [[@LINE-1]]:10 ->
[[@LINE+1]]:4 = 0
+ }
+ return i;
+}
+
+// CHECK-LABEL: _Z16check_constevalCi:
+constexpr int check_constevalC(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
ornata wrote:
Similarly, this test and the below one could just be called
`check_if_consteval` and `check_if_not_consteval`
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
@@ -98,3 +104,49 @@ int main() {// CHECK: File 0,
[[@LINE]]:12 -> {{[0-9]+}}:2 =
void ternary() {
true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 ->
}
+
+// FIXME: Do not generate coverage for discarded branches in if consteval
+// GH-57377
+// CHECK-LABEL: _Z16check_constevalAi:
+constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 ->
{{[0-9]+}}:2 = #0
ornata wrote:
Maybe `check_if_consteval_with_discarded_branch` would be a better name?
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= Message-ID: In-Reply-To: ornata wrote: LGTM https://github.com/llvm/llvm-project/pull/77214 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= Message-ID: In-Reply-To: https://github.com/ornata approved this pull request. https://github.com/llvm/llvm-project/pull/77214 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)
@@ -308,13 +309,21 @@ Error
RawCoverageMappingReader::readMappingRegionsSubArray(
return Err;
if (auto Err = readIntMax(FID, std::numeric_limits::max()))
return Err;
+ if (ID == 0)
+return make_error(
+coveragemap_error::malformed,
+"MCDCConditionID shouldn't be zero");
+ Params = CounterMappingRegion::MCDCBranchParameters{
+ (unsigned)ID, (unsigned)TID, (unsigned)FID};
ornata wrote:
`static_cast`?
https://github.com/llvm/llvm-project/pull/81227
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)
ornata wrote: Is the main benefit of this avoiding zero initialization? https://github.com/llvm/llvm-project/pull/81227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` (PR #81227)
https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/81227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC] Refactor: Introduce `MCDCTypes.h` for `coverage::mcdc` (PR #81459)
https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/81459 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clangCodeGen: Introduce `mcdc::State` with `MCDCState.h` (PR #81497)
@@ -0,0 +1,33 @@ +//=== MCDCState.h - MC/DC-related types for PGO -*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===--===// +// +// MC/DC-related types for PGO ornata wrote: comment seems misleading; this is only for the per-function MC/DC state, right? https://github.com/llvm/llvm-project/pull/81497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' [WIP] (PR #78033)
Hana =?utf-8?q?Dusíková?= Message-ID: In-Reply-To: ornata wrote: I think that skipping whitespace-only regions in llvm-cov intuitively makes sense. In general, showing coverage information about empty spaces doesn't give the user any useful information. I think I'd do that part in a separate patch though. https://github.com/llvm/llvm-project/pull/78033 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -746,21 +781,52 @@ struct MCDCCoverageBuilder {
// assign that ID to its LHS node. Its RHS will receive a new ID.
if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
// If Stmt has an ID, assign its ID to LHS
- CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];
+ setCondID(E->getLHS(), getCondID(E));
// Since the operator's LHS assumes the operator's same ID, pop the
// operator from the RHS stack so that if LHS short-circuits, it won't be
// incorrectly re-used as the node executed next.
popRHSifTop(E);
} else {
// Otherwise, assign ID+1 to LHS.
- CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
+ setCondID(E->getLHS(), NextID++);
}
// Assign ID+1 to RHS.
-CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
+setCondID(E->getRHS(), NextID++);
+
+// Assign the True and False condition IDs for the LHS and RHS.
+if (isLAnd(E)) {
ornata wrote:
I feel like it'd be easier to follow this if it was factored out into two
functions. E.g.
```
if (isLAnd(E))
assignCondIDsForLAnd(E);
else
assignCondIDsForLOr(E);
pushRHS(E);
```
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -722,6 +739,24 @@ struct MCDCCoverageBuilder {
return I->second;
}
+ /// Return the ID of the next condition when the given condition is True.
+ MCDCConditionID getNextIfTrueCondID(const Expr *Cond) const {
+auto I = TrueCondIDs.find(CodeGenFunction::stripCond(Cond));
+if (I == TrueCondIDs.end())
+ return 0;
ornata wrote:
Should document that this returns 0 when the condition is not found.
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -298,7 +298,7 @@ struct CounterMappingRegion {
unsigned ExpandedFileID, unsigned LineStart,
unsigned ColumnStart, unsigned LineEnd,
unsigned ColumnEnd, RegionKind Kind)
- : MCDCParams(MCDCParams), ExpandedFileID(ExpandedFileID),
+ : MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID),
ornata wrote:
Is this line the only bit necessary for the fix? Or are the other changes
necessary?
If this is all that is necessary, then I would recommend splitting this into
two patches.
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -722,6 +709,12 @@ struct MCDCCoverageBuilder {
return I->second;
}
+ /// Return the LHS Decision ({0,0} if not set).
+ const DecisionIDPair &back() {
+assert(DecisionStack.size() >= 1);
ornata wrote:
This assert should be unnecessary. `SmallVector` already asserts for you:
```
reference back() {
assert(!empty());
return end()[-1];
}
const_reference back() const {
assert(!empty());
return end()[-1];
}
```
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -676,41 +679,25 @@ struct MCDCCoverageBuilder {
return E->getOpcode() == BO_LAnd;
}
- /// Push an ID onto the corresponding RHS stack.
- void pushRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-rhs.push_back(CondIDs[CodeGenFunction::stripCond(E->getRHS())]);
- }
-
- /// Pop an ID from the corresponding RHS stack.
- void popRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-if (!rhs.empty())
- rhs.pop_back();
- }
-
- /// If the expected ID is on top, pop it off the corresponding RHS stack.
- void popRHSifTop(const BinaryOperator *E) {
-if (!OrRHS.empty() && CondIDs[E] == OrRHS.back())
- OrRHS.pop_back();
-else if (!AndRHS.empty() && CondIDs[E] == AndRHS.back())
- AndRHS.pop_back();
- }
-
public:
MCDCCoverageBuilder(CodeGenModule &CGM,
llvm::DenseMap &CondIDMap,
llvm::DenseMap &MCDCBitmapMap)
- : CGM(CGM), CondIDs(CondIDMap), MCDCBitmapMap(MCDCBitmapMap) {}
+ : CGM(CGM), DecisionStack(1), CondIDs(CondIDMap),
ornata wrote:
IIUC `DecisionStack(1)` is supposed to be a sentinel value.
Would it be possible to define a `DecisionStackSentinel` variable equal to 1,
so we can document it?
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -676,41 +679,25 @@ struct MCDCCoverageBuilder {
return E->getOpcode() == BO_LAnd;
}
- /// Push an ID onto the corresponding RHS stack.
- void pushRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-rhs.push_back(CondIDs[CodeGenFunction::stripCond(E->getRHS())]);
- }
-
- /// Pop an ID from the corresponding RHS stack.
- void popRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-if (!rhs.empty())
- rhs.pop_back();
- }
-
- /// If the expected ID is on top, pop it off the corresponding RHS stack.
- void popRHSifTop(const BinaryOperator *E) {
-if (!OrRHS.empty() && CondIDs[E] == OrRHS.back())
- OrRHS.pop_back();
-else if (!AndRHS.empty() && CondIDs[E] == AndRHS.back())
- AndRHS.pop_back();
- }
-
public:
MCDCCoverageBuilder(CodeGenModule &CGM,
llvm::DenseMap &CondIDMap,
llvm::DenseMap &MCDCBitmapMap)
- : CGM(CGM), CondIDs(CondIDMap), MCDCBitmapMap(MCDCBitmapMap) {}
+ : CGM(CGM), DecisionStack(1), CondIDs(CondIDMap),
+MCDCBitmapMap(MCDCBitmapMap) {}
- /// Return the ID of the RHS of the next, upper nest-level logical-OR.
- MCDCConditionID getNextLOrCondID() const {
-return OrRHS.empty() ? 0 : OrRHS.back();
- }
+ /// Return whether the control flow map is not presently being built. This
+ /// can be used to determine whether the flow is at the root node of an
+ /// expression if that expression is mapped.
+ bool isIdle() { return (NextID == 1 && !NotMapped); }
ornata wrote:
these can be `const`
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang][CoverageMapping] Refactor when setting MC/DC True/False (PR #78202)
@@ -676,41 +679,25 @@ struct MCDCCoverageBuilder {
return E->getOpcode() == BO_LAnd;
}
- /// Push an ID onto the corresponding RHS stack.
- void pushRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-rhs.push_back(CondIDs[CodeGenFunction::stripCond(E->getRHS())]);
- }
-
- /// Pop an ID from the corresponding RHS stack.
- void popRHS(const BinaryOperator *E) {
-llvm::SmallVector &rhs = isLAnd(E) ? AndRHS : OrRHS;
-if (!rhs.empty())
- rhs.pop_back();
- }
-
- /// If the expected ID is on top, pop it off the corresponding RHS stack.
- void popRHSifTop(const BinaryOperator *E) {
-if (!OrRHS.empty() && CondIDs[E] == OrRHS.back())
- OrRHS.pop_back();
-else if (!AndRHS.empty() && CondIDs[E] == AndRHS.back())
- AndRHS.pop_back();
- }
-
public:
MCDCCoverageBuilder(CodeGenModule &CGM,
llvm::DenseMap &CondIDMap,
llvm::DenseMap &MCDCBitmapMap)
- : CGM(CGM), CondIDs(CondIDMap), MCDCBitmapMap(MCDCBitmapMap) {}
+ : CGM(CGM), DecisionStack(1), CondIDs(CondIDMap),
+MCDCBitmapMap(MCDCBitmapMap) {}
- /// Return the ID of the RHS of the next, upper nest-level logical-OR.
- MCDCConditionID getNextLOrCondID() const {
-return OrRHS.empty() ? 0 : OrRHS.back();
- }
+ /// Return whether the control flow map is not presently being built. This
+ /// can be used to determine whether the flow is at the root node of an
ornata wrote:
I find the comment here kind of confusing.
I'd assume that `isIdle() == !isBuilding()` from this comment.
https://github.com/llvm/llvm-project/pull/78202
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' (PR #78033)
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?=
Message-ID:
In-Reply-To:
@@ -1700,43 +1776,116 @@ struct CounterCoverageMappingBuilder
Visit(S->getSubStmt());
}
+ void CoverIfConsteval(const IfStmt *S) {
ornata wrote:
nit: LLVM style guidelines say function names ought to be camel case, starting
with a lower-case letter
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
https://github.com/llvm/llvm-project/pull/78033
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' (PR #78033)
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?=
Message-ID:
In-Reply-To:
@@ -1700,43 +1776,116 @@ struct CounterCoverageMappingBuilder
Visit(S->getSubStmt());
}
+ void CoverIfConsteval(const IfStmt *S) {
+assert(S->isConsteval());
+
+const auto *Then = S->getThen();
+const auto *Else = S->getElse();
+
+// I'm using 'propagateCounts' later as new region is better and allows me
+// to properly calculate line coverage in llvm-cov utility
+const Counter ParentCount = getRegion().getCounter();
+
+extendRegion(S);
+
+if (S->isNegatedConsteval()) {
+ // ignore 'if consteval'
+ markSkipped(S->getIfLoc(), getStart(Then));
+ propagateCounts(ParentCount, Then);
+
+ if (Else) {
+// ignore 'else '
+markSkipped(getEnd(Then), getEnd(Else));
+ }
+} else {
+ assert(S->isNonNegatedConsteval());
+ // ignore 'if consteval [else]'
+ markSkipped(S->getIfLoc(), Else ? getStart(Else) : getEnd(Then));
+
+ if (Else)
+propagateCounts(ParentCount, Else);
+}
+ }
+
+ void CoverIfConstexpr(const IfStmt *S) {
+assert(S->isConstexpr());
+
+// evaluate constant condition...
+const auto *E = dyn_cast(S->getCond());
+assert(E != nullptr);
+const bool isTrue = E->getResultAsAPSInt().getExtValue();
+
+extendRegion(S);
+
+const auto *Init = S->getInit();
+const auto *Then = S->getThen();
+const auto *Else = S->getElse();
+
+// I'm using 'propagateCounts' later as new region is better and allows me
+// to properly calculate line coverage in llvm-cov utility
+const Counter ParentCount = getRegion().getCounter();
+
+// ignore 'if constexpr ('
+SourceLocation startOfSkipped = S->getIfLoc();
+
+if (Init) {
+ // don't mark initialisation as ignored
+ markSkipped(startOfSkipped, getStart(Init));
+ propagateCounts(ParentCount, Init);
+ // ignore after initialisation: '; )'...
+ startOfSkipped = getEnd(Init);
+}
+
+if (isTrue) {
+ // ignore ')'
+ markSkipped(startOfSkipped, getStart(Then));
+ propagateCounts(ParentCount, Then);
+
+ if (Else)
+// ignore 'else '
+markSkipped(getEnd(Then), getEnd(Else));
+} else {
+ // ignore ') [else]'
+ markSkipped(startOfSkipped, Else ? getStart(Else) : getEnd(Then));
+
+ if (Else) {
ornata wrote:
style nit: LLVM prefers omitting braces on single-line `if`/`while`/`for`
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
https://github.com/llvm/llvm-project/pull/78033
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' (PR #78033)
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?=
Message-ID:
In-Reply-To:
@@ -1251,6 +1264,69 @@ struct CounterCoverageMappingBuilder
popRegions(Index);
}
+ /// Find a valid range starting with \p StartingLoc and ending before \p
+ /// BeforeLoc.
+ std::optional findAreaStartingFromTo(SourceLocation StartingLoc,
+SourceLocation BeforeLoc) {
+// If StartingLoc is in function-like macro, use its start location.
+if (StartingLoc.isMacroID()) {
+ FileID FID = SM.getFileID(StartingLoc);
+ const SrcMgr::ExpansionInfo *EI = &SM.getSLocEntry(FID).getExpansion();
+ if (EI->isFunctionMacroExpansion())
+StartingLoc = EI->getExpansionLocStart();
+}
+
+size_t StartDepth = locationDepth(StartingLoc);
+size_t EndDepth = locationDepth(BeforeLoc);
+while (!SM.isWrittenInSameFile(StartingLoc, BeforeLoc)) {
+ bool UnnestStart = StartDepth >= EndDepth;
+ bool UnnestEnd = EndDepth >= StartDepth;
+ if (UnnestEnd) {
ornata wrote:
Could you add a comment explaining `UnnestStart` and `UnnestEnd`?
https://github.com/llvm/llvm-project/pull/78033
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' (PR #78033)
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?= ,
Hana =?utf-8?q?Dusíková?=
Message-ID:
In-Reply-To:
@@ -174,6 +179,10 @@ class SourceMappingRegion {
void setGap(bool Gap) { GapRegion = Gap; }
+ bool isSkipped() const { return SkippedRegion; }
ornata wrote:
Unrelated to this patch: I wonder if `SkippedRegion` is being used elsewhere.
This could be nice clean up.
https://github.com/llvm/llvm-project/pull/78033
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' (PR #78033)
Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= , Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= , Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= , Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= , Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= Message-ID: In-Reply-To: https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/78033 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 17cfd2e - [profiling] Improve error message for raw profile header mismatches
Author: Jessica Paquette
Date: 2023-04-27T14:51:38-07:00
New Revision: 17cfd2e025cb3aa929ad219c6ed0974d6198bf5b
URL:
https://github.com/llvm/llvm-project/commit/17cfd2e025cb3aa929ad219c6ed0974d6198bf5b
DIFF:
https://github.com/llvm/llvm-project/commit/17cfd2e025cb3aa929ad219c6ed0974d6198bf5b.diff
LOG: [profiling] Improve error message for raw profile header mismatches
When a user uses a mismatched clang + llvm-profdata, they didn't get a very
informative error message. It would just say "unsupported version".
As a result, users are often confused as to what they are supposed to do and
tend to assume that it's a bug in the profiling runtime.
This patch improves the error message by:
- Adding a new class of error (`raw_profile_version_mismatch`) to make it clear
that, specifically, the *raw profile* version is unsupported because of a
tool mismatch.
- Adding an error message that tells the user which raw profile version was
encountered, which version was expected, and instructs them to align their
tool versions.
To support this, this patch also updates `InstrProfError::take` to also
propagate the optional error message.
Differential Revision: https://reviews.llvm.org/D149361
Added:
llvm/test/tools/llvm-profdata/mismatched-raw-profile-header.test
Modified:
clang/lib/CodeGen/CodeGenPGO.cpp
llvm/include/llvm/ProfileData/InstrProf.h
llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
llvm/lib/ProfileData/InstrProf.cpp
llvm/lib/ProfileData/InstrProfReader.cpp
llvm/tools/llvm-profdata/llvm-profdata.cpp
llvm/unittests/ProfileData/InstrProfTest.cpp
Removed:
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp
b/clang/lib/CodeGen/CodeGenPGO.cpp
index 15a3d74666ca2..b80317529b72b 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1036,7 +1036,7 @@ void
CodeGenPGO::loadRegionCounts(llvm::IndexedInstrProfReader *PGOReader,
llvm::Expected RecordExpected =
PGOReader->getInstrProfRecord(FuncName, FunctionHash);
if (auto E = RecordExpected.takeError()) {
-auto IPE = llvm::InstrProfError::take(std::move(E));
+auto IPE = std::get<0>(llvm::InstrProfError::take(std::move(E)));
if (IPE == llvm::instrprof_error::unknown_function)
CGM.getPGOStats().addMissing(IsInMainFile);
else if (IPE == llvm::instrprof_error::hash_mismatch)
diff --git a/llvm/include/llvm/ProfileData/InstrProf.h
b/llvm/include/llvm/ProfileData/InstrProf.h
index 2a9da23f4f02f..ef01070832dee 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -330,7 +330,8 @@ enum class instrprof_error {
compress_failed,
uncompress_failed,
empty_raw_profile,
- zlib_unavailable
+ zlib_unavailable,
+ raw_profile_version_mismatch
};
/// An ordered list of functions identified by their NameRef found in
@@ -362,15 +363,18 @@ class InstrProfError : public ErrorInfo {
instrprof_error get() const { return Err; }
const std::string &getMessage() const { return Msg; }
- /// Consume an Error and return the raw enum value contained within it. The
- /// Error must either be a success value, or contain a single InstrProfError.
- static instrprof_error take(Error E) {
+ /// Consume an Error and return the raw enum value contained within it, and
+ /// the optional error message. The Error must either be a success value, or
+ /// contain a single InstrProfError.
+ static std::pair take(Error E) {
auto Err = instrprof_error::success;
-handleAllErrors(std::move(E), [&Err](const InstrProfError &IPE) {
+std::string Msg = "";
+handleAllErrors(std::move(E), [&Err, &Msg](const InstrProfError &IPE) {
assert(Err == instrprof_error::success && "Multiple errors encountered");
Err = IPE.get();
+ Msg = IPE.getMessage();
});
-return Err;
+return {Err, Msg};
}
static char ID;
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index d6aec276838e6..849ee80bfaa33 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -249,7 +249,7 @@ Error CoverageMapping::loadFunctionRecord(
std::vector Counts;
if (Error E = ProfileReader.getFunctionCounts(Record.FunctionName,
Record.FunctionHash, Counts)) {
-instrprof_error IPE = InstrProfError::take(std::move(E));
+instrprof_error IPE = std::get<0>(InstrProfError::take(std::move(E)));
if (IPE == instrprof_error::hash_mismatch) {
FuncHashMismatches.emplace_back(std::string(Record.FunctionName),
Record.FunctionHash);
diff --git a/llvm/lib/ProfileData/InstrProf.cpp
b/llvm/lib/ProfileData/InstrProf.cpp
index d8dbcf1ebbe9a..1dd1ce4b3e50a 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
[clang] [llvm] [MC/DC] Refactor: Let MCDCConditionID int16_t with zero-origin (PR #81257)
https://github.com/ornata approved this pull request. https://github.com/llvm/llvm-project/pull/81257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -983,7 +979,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
// for most embedded applications. Setting a maximum value prevents the
// bitmap footprint from growing too large without the user's knowledge. In
// the future, this value could be adjusted with a command-line option.
- unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;
+ unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 32767 : 0;
ornata wrote:
I think the limit should be controlled by a flag, with a reasonable default
value.
E.g.
`-mcdc-max-conditions=`
https://github.com/llvm/llvm-project/pull/82448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -484,10 +484,31 @@ MC/DC Instrumentation - When instrumenting for Modified Condition/Decision Coverage (MC/DC) using the -clang option ``-fcoverage-mcdc``, users are limited to at most **six** leaf-level -conditions in a boolean expression. A warning will be generated for boolean -expressions that contain more than six, and they will not be instrumented for -MC/DC. +clang option ``-fcoverage-mcdc``, there are two hard limits. ornata wrote: Suggest simplifying the phrasing here. > By default, Modified Condition/Decision Coverage (MC/DC) instrumentation is > limited to 32767 terms. Users may provide their own limit on the number of > terms, `n` by passing ``-Xclang -fmcdc-max-conditions=n``. https://github.com/llvm/llvm-project/pull/82448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
https://github.com/ornata edited https://github.com/llvm/llvm-project/pull/82448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
https://github.com/ornata commented: Some nits on documentation wording. Code looks fine to me. https://github.com/llvm/llvm-project/pull/82448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -484,10 +484,31 @@ MC/DC Instrumentation - When instrumenting for Modified Condition/Decision Coverage (MC/DC) using the -clang option ``-fcoverage-mcdc``, users are limited to at most **six** leaf-level -conditions in a boolean expression. A warning will be generated for boolean -expressions that contain more than six, and they will not be instrumented for -MC/DC. +clang option ``-fcoverage-mcdc``, there are two hard limits. + +The maximum number of terms is limited to 32767, which is practical for +handwritten expressions. To be more restrictive in order to enforce coding rules, +use ``-Xclang -fmcdc-max-conditions=n``. Expressions with exceeded condition +counts ``n`` will generate warnings. + +The number of test vectors (the maximum number of possible combinations of +expressions) is limited to 2,147,483,646. In this case, approximately +256MiB (==2GiB/8) is used to record test vectors. + +To reduce memory usage, you can limit the maximum number of test vectors per ornata wrote: Suggest consistently using "user" or "you" throughout docs. https://github.com/llvm/llvm-project/pull/82448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -484,10 +484,31 @@ MC/DC Instrumentation - When instrumenting for Modified Condition/Decision Coverage (MC/DC) using the -clang option ``-fcoverage-mcdc``, users are limited to at most **six** leaf-level -conditions in a boolean expression. A warning will be generated for boolean -expressions that contain more than six, and they will not be instrumented for -MC/DC. +clang option ``-fcoverage-mcdc``, there are two hard limits. ornata wrote: I think it's fine actually. https://github.com/llvm/llvm-project/pull/82448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
https://github.com/ornata approved this pull request. https://github.com/llvm/llvm-project/pull/82448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -190,18 +190,30 @@ class SourceMappingRegion {
bool isBranch() const { return FalseCount.has_value(); }
+ bool isMCDCBranch() const {
+const auto *BranchParams =
std::get_if(&MCDCParams);
+assert(BranchParams == nullptr || BranchParams->ID >= 0);
ornata wrote:
I think it would be nice if this assert was not necessary. Is it possible to
enforce that BranchParams->ID is never when we have MC/DC parameters?
https://github.com/llvm/llvm-project/pull/82448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -190,18 +190,30 @@ class SourceMappingRegion {
bool isBranch() const { return FalseCount.has_value(); }
+ bool isMCDCBranch() const {
+const auto *BranchParams =
std::get_if(&MCDCParams);
+assert(BranchParams == nullptr || BranchParams->ID >= 0);
+return (BranchParams != nullptr);
+ }
+
+ const auto &getMCDCBranchParams() const {
+return mcdc::getParams(MCDCParams);
+ }
+
bool isMCDCDecision() const {
const auto *DecisionParams =
std::get_if(&MCDCParams);
-assert(!DecisionParams || DecisionParams->NumConditions > 0);
-return DecisionParams;
+assert(DecisionParams == nullptr || DecisionParams->NumConditions > 0);
ornata wrote:
Similar to the other comment, it would be nice if we could enforce at
construction-time that MC/DC parameters have at least 1 condition.
https://github.com/llvm/llvm-project/pull/82448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -2050,23 +2069,74 @@ struct CounterCoverageMappingBuilder
subtractCounters(ParentCount, TrueCount));
}
- void createDecision(const BinaryOperator *E) {
+ void createOrCancelDecision(const BinaryOperator *E, unsigned Since) {
unsigned NumConds = MCDCBuilder.getTotalConditionsAndReset(E);
if (NumConds == 0)
return;
+// Extract [ID, Conds] to construct the graph.
+llvm::SmallVector CondIDs(NumConds);
+for (const auto &SR : ArrayRef(SourceRegions).slice(Since)) {
+ if (SR.isMCDCBranch()) {
ornata wrote:
nit: can you use `make_filter_range` from `STLExtras`?
https://github.com/llvm/llvm-project/pull/82448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [MC/DC][Coverage] Loosen the limit of NumConds from 6 (PR #82448)
@@ -983,7 +979,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
// for most embedded applications. Setting a maximum value prevents the
// bitmap footprint from growing too large without the user's knowledge. In
// the future, this value could be adjusted with a command-line option.
- unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;
+ unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 32767 : 0;
ornata wrote:
Also why 32767? Max signed 16-bit value? Is there a reason?
https://github.com/llvm/llvm-project/pull/82448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Suppress covmap and profdata for system headers. (PR #97952)
https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/97952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage][Expansion] handle nested macros in scratch space (PR #89869)
@@ -517,7 +552,7 @@ class CoverageMappingBuilder {
SourceRegionFilter Filter;
for (const auto &FM : FileIDMapping) {
SourceLocation ExpandedLoc = FM.second.second;
- SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);
+ SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc, false);
ornata wrote:
The boolean parameter for this function is only used in this single callsite.
Do we need it at all?
https://github.com/llvm/llvm-project/pull/89869
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Rework !SystemHeadersCoverage (PR #91446)
@@ -2064,7 +2082,20 @@ struct CounterCoverageMappingBuilder
createDecisionRegion(E, DecisionParams);
}
+ /// Check if E belongs to system headers.
+ bool isExprInSystemHeader(const BinaryOperator *E) const {
ornata wrote:
assert E is not nullptr?
https://github.com/llvm/llvm-project/pull/91446
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MC/DC][Coverage] Add assertions into emitSourceRegions() (PR #89572)
@@ -190,6 +190,16 @@ class SourceMappingRegion {
bool isBranch() const { return FalseCount.has_value(); }
+ bool isMCDCBranch() const {
+const auto *BranchParams =
std::get_if(&MCDCParams);
+assert(BranchParams == nullptr || BranchParams->ID >= 0);
+return (BranchParams != nullptr);
+ }
+
+ const auto &getMCDCBranchParams() const {
ornata wrote:
unused function?
https://github.com/llvm/llvm-project/pull/89572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Rework !SystemHeadersCoverage (PR #91446)
https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/91446 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage][Expansion] handle nested macros in scratch space (PR #89869)
@@ -517,7 +552,7 @@ class CoverageMappingBuilder {
SourceRegionFilter Filter;
for (const auto &FM : FileIDMapping) {
SourceLocation ExpandedLoc = FM.second.second;
- SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);
+ SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc, false);
ornata wrote:
I thought it was only used in one place, oops. :) The current way is fine.
https://github.com/llvm/llvm-project/pull/89869
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MC/DC][Coverage] Add assertions into emitSourceRegions() (PR #89572)
https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/89572 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] b9e57e0 - Revert "[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it"
Author: Jessica Paquette
Date: 2021-09-03T10:28:07-07:00
New Revision: b9e57e030560fef9ddc51caca8bacfefccdf8a62
URL:
https://github.com/llvm/llvm-project/commit/b9e57e030560fef9ddc51caca8bacfefccdf8a62
DIFF:
https://github.com/llvm/llvm-project/commit/b9e57e030560fef9ddc51caca8bacfefccdf8a62.diff
LOG: Revert "[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to
check entire function calls, rather than each ExplodedNode in it"
This reverts commit a375bfb5b729e0f3ca8d5e001f423fa89e74de87.
This was causing a bot to crash:
https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/23380/
Added:
Modified:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/unittests/StaticAnalyzer/CMakeLists.txt
clang/unittests/StaticAnalyzer/CallEventTest.cpp
clang/unittests/StaticAnalyzer/CheckerRegistration.h
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
Removed:
clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp
diff --git
a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index c42521376af92..139b0dcd51704 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -633,9 +633,6 @@ class CXXConstructorCall;
/// Descendants can define what a "state change is", like a change of value
/// to a memory region, liveness, etc. For function calls where the state did
/// not change as defined, a custom note may be constructed.
-///
-/// For a minimal example, check out
-/// clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp.
class NoStateChangeFuncVisitor : public BugReporterVisitor {
private:
/// Frames modifying the state as defined in \c wasModifiedBeforeCallExit.
@@ -646,8 +643,6 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor {
/// many times (going up the path for each node and checking whether the
/// region was written into) we instead lazily compute the stack frames
/// along the path.
- // TODO: Can't we just use a map instead? This is likely not as cheap as it
- // makes the code
diff icult to read.
llvm::SmallPtrSet FramesModifying;
llvm::SmallPtrSet FramesModifyingCalculated;
@@ -656,8 +651,6 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor {
/// The calculation is cached in FramesModifying.
bool isModifiedInFrame(const ExplodedNode *CallExitBeginN);
- void markFrameAsModifying(const StackFrameContext *SCtx);
-
/// Write to \c FramesModifying all stack frames along the path in the
current
/// stack frame which modifies the state.
void findModifyingFrames(const ExplodedNode *const CallExitBeginN);
@@ -665,37 +658,11 @@ class NoStateChangeFuncVisitor : public
BugReporterVisitor {
protected:
bugreporter::TrackingKind TKind;
- /// \return Whether the state was modified from the current node, \p CurrN,
to
- /// the end of the stack frame, at \p CallExitBeginN. \p CurrN and
- /// \p CallExitBeginN are always in the same stack frame.
- /// Clients should override this callback when a state change is important
- /// not only on the entire function call, but inside of it as well.
- /// Example: we may want to leave a note about the lack of locking/unlocking
- /// on a particular mutex, but not if inside the function its state was
- /// changed, but also restored. wasModifiedInFunction() wouldn't know of this
- /// change.
- virtual bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
- const ExplodedNode *CallExitBeginN) {
-return false;
- }
-
- /// \return Whether the state was modified in the inlined function call in
- /// between \p CallEnterN and \p CallExitEndN. Mind that the stack frame
- /// retrieved from a CallEnterN and CallExitEndN is the *caller's* stack
- /// frame! The inlined function's stack should be retrieved from either the
- /// immediate successor to \p CallEnterN or immediate predecessor to
- /// \p CallExitEndN.
- /// Clients should override this function if a state changes local to the
- /// inlined function are not interesting, only the change occuring as a
- /// result of it.
- /// Example: we want to leave a not about a leaked resource object not being
- /// deallocated / its ownership changed inside a function, and we don't care
- /// if it was assigned to a local variable (its change in ownership is
- /// inconsequential).
- virtual bool wasModifiedInFunction(const ExplodedNode *CallEnterN,
- const Ex
[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)
@@ -719,10 +720,10 @@ struct FunctionRecord {
Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
CountedBranchRegions.emplace_back(Region, Count, FalseCount,
HasSingleByteCoverage);
- // If both counters are hard-coded to zero, then this region represents a
+ // If either counters is hard-coded to zero, then this region represents
a
// constant-folded branch.
- if (Region.Count.isZero() && Region.FalseCount.isZero())
-CountedBranchRegions.back().Folded = true;
+ CountedBranchRegions.back().TrueFolded = Region.Count.isZero();
ornata wrote:
rename Region.Count to Region.TrueCount?
https://github.com/llvm/llvm-project/pull/112694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)
@@ -719,10 +720,10 @@ struct FunctionRecord {
Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
CountedBranchRegions.emplace_back(Region, Count, FalseCount,
HasSingleByteCoverage);
- // If both counters are hard-coded to zero, then this region represents a
+ // If either counters is hard-coded to zero, then this region represents
a
ornata wrote:
s/counters/counter
https://github.com/llvm/llvm-project/pull/112694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)
@@ -719,10 +720,10 @@ struct FunctionRecord {
Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
CountedBranchRegions.emplace_back(Region, Count, FalseCount,
HasSingleByteCoverage);
- // If both counters are hard-coded to zero, then this region represents a
+ // If either counters is hard-coded to zero, then this region represents
a
ornata wrote:
s/counters/counter
https://github.com/llvm/llvm-project/pull/112694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)
@@ -19,18 +19,18 @@ using namespace coverage;
static void sumBranches(size_t &NumBranches, size_t &CoveredBranches,
const ArrayRef &Branches) {
for (const auto &BR : Branches) {
-// Skip folded branches.
-if (BR.Folded)
- continue;
-
-// "True" Condition Branches.
-++NumBranches;
-if (BR.ExecutionCount > 0)
- ++CoveredBranches;
-// "False" Condition Branches.
-++NumBranches;
-if (BR.FalseExecutionCount > 0)
- ++CoveredBranches;
+if (!BR.TrueFolded) {
+ // "True" Condition Branches.
+ ++NumBranches;
+ if (BR.ExecutionCount > 0)
ornata wrote:
Rename ExecutionCount to TrueExecutionCount?
https://github.com/llvm/llvm-project/pull/112694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)
@@ -125,7 +125,7 @@ json::Array renderRegions(ArrayRef
Regions) {
json::Array renderBranchRegions(ArrayRef Regions) {
json::Array RegionArray;
for (const auto &Region : Regions)
-if (!Region.Folded)
+if (!Region.TrueFolded || !Region.FalseFolded)
ornata wrote:
replace with a helper function? e.g. Region.isFolded()
https://github.com/llvm/llvm-project/pull/112694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Coverage] Introduce "partial fold" on BranchRegion (PR #112694)
@@ -19,18 +19,18 @@ using namespace coverage;
static void sumBranches(size_t &NumBranches, size_t &CoveredBranches,
const ArrayRef &Branches) {
for (const auto &BR : Branches) {
-// Skip folded branches.
-if (BR.Folded)
- continue;
-
-// "True" Condition Branches.
-++NumBranches;
-if (BR.ExecutionCount > 0)
- ++CoveredBranches;
-// "False" Condition Branches.
-++NumBranches;
-if (BR.FalseExecutionCount > 0)
- ++CoveredBranches;
+if (!BR.TrueFolded) {
+ // "True" Condition Branches.
+ ++NumBranches;
+ if (BR.ExecutionCount > 0)
+++CoveredBranches;
+}
+if (!BR.FalseFolded) {
ornata wrote:
Can this be a lambda or static helper function?
https://github.com/llvm/llvm-project/pull/112694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1592,6 +1605,13 @@ struct CounterCoverageMappingBuilder
llvm::EnableSingleByteCoverage
? getRegionCounter(S->getCond())
: addCounters(ParentCount, BackedgeCount, BC.ContinueCount);
+auto [ExecCount, ExitCount] =
+(llvm::EnableSingleByteCoverage
+ ? std::make_pair(getRegionCounter(S), Counter::getZero())
+ : getBranchCounterPair(S, CondCount));
+if (!llvm::EnableSingleByteCoverage) {
ornata wrote:
probably needs to be wrapped in NDEBUG or whatever?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -941,6 +941,19 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ std::pair getBranchCounterPair(const Stmt *S,
+ Counter ParentCnt) {
+Counter ExecCnt = getRegionCounter(S);
+return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ }
+
+ bool IsCounterEqual(Counter OutCount, Counter ParentCount) {
ornata wrote:
Should OutCount and ParentCount be const?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1709,6 +1730,13 @@ struct CounterCoverageMappingBuilder
: addCounters(
addCounters(ParentCount, BackedgeCount,
BodyBC.ContinueCount),
IncrementBC.ContinueCount);
+auto [ExecCount, ExitCount] =
+(llvm::EnableSingleByteCoverage
+ ? std::make_pair(getRegionCounter(S), Counter::getZero())
+ : getBranchCounterPair(S, CondCount));
+if (!llvm::EnableSingleByteCoverage) {
ornata wrote:
wrap in `#ifndef NDEBUG`?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1804,9 +1832,10 @@ struct CounterCoverageMappingBuilder
Counter LoopCount =
addCounters(ParentCount, BackedgeCount, BC.ContinueCount);
-Counter OutCount =
-addCounters(BC.BreakCount, subtractCounters(LoopCount, BodyCount));
-if (OutCount != ParentCount) {
+auto [ExecCount, ExitCount] = getBranchCounterPair(S, LoopCount);
+assert(ExecCount.isZero() || ExecCount == BodyCount);
+Counter OutCount = addCounters(BC.BreakCount, ExitCount);
ornata wrote:
this looks like it ought to be a helper function as well
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -941,6 +941,19 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ std::pair getBranchCounterPair(const Stmt *S,
ornata wrote:
it's not immediately obvious what the pair represents from this function name.
`getExecutionCountAnd...`?
or, make a lightweight POD struct to represent this
```
struct ExecCntAnd... {
Counter ExecCnt
Counter ...
};
```
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -941,6 +941,19 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ std::pair getBranchCounterPair(const Stmt *S,
+ Counter ParentCnt) {
+Counter ExecCnt = getRegionCounter(S);
+return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ }
+
+ bool IsCounterEqual(Counter OutCount, Counter ParentCount) {
+if (OutCount == ParentCount)
ornata wrote:
```
return OutCount == ParentCount;
```
?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1645,22 +1662,26 @@ struct CounterCoverageMappingBuilder
Counter CondCount = llvm::EnableSingleByteCoverage
? getRegionCounter(S->getCond())
: addCounters(BackedgeCount, BC.ContinueCount);
+auto [ExecCount, ExitCount] =
+(llvm::EnableSingleByteCoverage
+ ? std::make_pair(getRegionCounter(S), Counter::getZero())
+ : getBranchCounterPair(S, CondCount));
+if (!llvm::EnableSingleByteCoverage) {
ornata wrote:
probably also needs to be wrapped in `#ifndef NDEBUG `
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1709,6 +1730,13 @@ struct CounterCoverageMappingBuilder : addCounters( addCounters(ParentCount, BackedgeCount, BodyBC.ContinueCount), IncrementBC.ContinueCount); +auto [ExecCount, ExitCount] = +(llvm::EnableSingleByteCoverage ornata wrote: can this be a static helper function? https://github.com/llvm/llvm-project/pull/112702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -941,6 +941,19 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ std::pair getBranchCounterPair(const Stmt *S,
+ Counter ParentCnt) {
+Counter ExecCnt = getRegionCounter(S);
+return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ }
+
+ bool IsCounterEqual(Counter OutCount, Counter ParentCount) {
ornata wrote:
I think it's fine
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -938,6 +938,37 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ struct BranchCounterPair {
ornata wrote:
A doxygen comment here might be useful.
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -938,6 +938,37 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ struct BranchCounterPair {
+Counter Executed;
ornata wrote:
Comments on member variables?
E.g.
```
/// Counter tracking number of times the branch was encountered and taken.
Counter Executed;
/// Counter tracking the number of times the branch was encountered, but not
taken.
Counter Skipped;
```
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -938,6 +938,37 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ struct BranchCounterPair {
ornata wrote:
IIUC the point of this pair is given:
```
if (x) {
// ... do something
} else if (y) {
// ... something else
} else {
// ... another something else
}
```
You want to track, _from the perspective of a branch_, how many times it is
executed.
For example, there will be a `BranchAndCounterPair` for the `if`, the `else
if`, and the `else`.
Assuming I'm understanding this correctly, I think we could explain this struct
like so:
```
/// Structure to track how many times a conditional was executed or skipped
over when evaluated at runtime.
///
/// E.g. given a conditional like
///
/// if (x) {...}
///
/// Keep track of the number of times the `if` branch is taken, and the number
of times it is not taken.
```
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -938,6 +938,37 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ struct BranchCounterPair {
+Counter Executed;
+Counter Skipped;
+ };
+
+ BranchCounterPair getBranchCounterPair(const Stmt *S, Counter ParentCnt) {
+Counter ExecCnt = getRegionCounter(S);
+return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ }
+
+ /// Returns {TrueCnt,FalseCnt} for "implicit default".
+ /// FalseCnt is considered as the False count on SwitchStmt.
ornata wrote:
Comment out of date?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -938,6 +938,37 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ struct BranchCounterPair {
+Counter Executed;
+Counter Skipped;
+ };
+
+ BranchCounterPair getBranchCounterPair(const Stmt *S, Counter ParentCnt) {
ornata wrote:
Can we add Doxygen comments for this?
I'd like to be able to know what, e.g. `ParentCnt` is only by looking at this
function.
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1592,6 +1605,13 @@ struct CounterCoverageMappingBuilder
llvm::EnableSingleByteCoverage
? getRegionCounter(S->getCond())
: addCounters(ParentCount, BackedgeCount, BC.ContinueCount);
+auto [ExecCount, ExitCount] =
+(llvm::EnableSingleByteCoverage
+ ? std::make_pair(getRegionCounter(S), Counter::getZero())
+ : getBranchCounterPair(S, CondCount));
+if (!llvm::EnableSingleByteCoverage) {
ornata wrote:
I'm bothered that we have an `if` that is only wrapping an assert.
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -939,8 +939,17 @@ struct CounterCoverageMappingBuilder
Counter Skipped;
};
- BranchCounterPair getBranchCounterPair(const Stmt *S, Counter ParentCnt) {
+ BranchCounterPair
+ getBranchCounterPair(const Stmt *S, Counter ParentCnt,
+ std::optional SkipCntForOld = std::nullopt) {
Counter ExecCnt = getRegionCounter(S);
+
+// The old behavior of SingleByte shouldn't emit Branches.
+if (llvm::EnableSingleByteCoverage) {
+ assert(SkipCntForOld);
ornata wrote:
Can you add a string this this assert like
```
assert(SkipCntForOld && "Single byte coverage shouldn't emit branches when
using old behaviour!");
```
Or something?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -939,8 +939,17 @@ struct CounterCoverageMappingBuilder
Counter Skipped;
};
- BranchCounterPair getBranchCounterPair(const Stmt *S, Counter ParentCnt) {
+ BranchCounterPair
+ getBranchCounterPair(const Stmt *S, Counter ParentCnt,
+ std::optional SkipCntForOld = std::nullopt) {
ornata wrote:
Can you add a doxygen comment for `SkipCntForOld`?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -939,8 +939,17 @@ struct CounterCoverageMappingBuilder
Counter Skipped;
};
- BranchCounterPair getBranchCounterPair(const Stmt *S, Counter ParentCnt) {
+ BranchCounterPair
+ getBranchCounterPair(const Stmt *S, Counter ParentCnt,
+ std::optional SkipCntForOld = std::nullopt) {
Counter ExecCnt = getRegionCounter(S);
+
+// The old behavior of SingleByte shouldn't emit Branches.
+if (llvm::EnableSingleByteCoverage) {
+ assert(SkipCntForOld);
ornata wrote:
if someone else runs into a problem, it will make it easier for them to
understand what is going on
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MC/DC] Update CoverageMapping tests (PR #125404)
https://github.com/ornata approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/125404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MC/DC] Refactor MCDC::State::Decision. NFC. (PR #125408)
@@ -30,8 +32,20 @@ struct State {
unsigned BitmapBits = 0;
struct Decision {
+using IndicesTy = llvm::SmallVector>;
+static constexpr auto InvalidID = std::numeric_limits::max();
+
unsigned BitmapIdx;
-llvm::SmallVector> Indices;
+IndicesTy Indices;
+unsigned ID = InvalidID;
+
+bool isValid() const { return ID != InvalidID; }
+
+void update(unsigned I, IndicesTy &&X) {
+ assert(ID != InvalidID);
ornata wrote:
Also, how expensive is this? Would it make sense for this to be a real error in
release builds as well?
https://github.com/llvm/llvm-project/pull/125408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MC/DC] Refactor MCDC::State::Decision. NFC. (PR #125408)
@@ -30,8 +32,20 @@ struct State {
unsigned BitmapBits = 0;
struct Decision {
+using IndicesTy = llvm::SmallVector>;
+static constexpr auto InvalidID = std::numeric_limits::max();
+
unsigned BitmapIdx;
-llvm::SmallVector> Indices;
+IndicesTy Indices;
+unsigned ID = InvalidID;
+
+bool isValid() const { return ID != InvalidID; }
+
+void update(unsigned I, IndicesTy &&X) {
+ assert(ID != InvalidID);
ornata wrote:
just use `isValid()`?
https://github.com/llvm/llvm-project/pull/125408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Introduce SingleByteCoverage tests (w/yaml2obj) (PR #113114)
ornata wrote: Can you explain why we want to use yaml2obj for testcases in the commit message? https://github.com/llvm/llvm-project/pull/113114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce the type `CounterPair` for RegionCounterMap. NFC. (PR #112724)
@@ -1869,7 +1871,10 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
// If we are at an unreachable point, we don't need to emit the initializer
// unless it contains a label.
if (!HaveInsertPoint()) {
-if (!Init || !ContainsLabel(Init)) return;
+if (!Init || !ContainsLabel(Init)) {
+ PGO.markStmtMaybeUsed(Init);
ornata wrote:
why?
https://github.com/llvm/llvm-project/pull/112724
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -941,6 +941,19 @@ struct CounterCoverageMappingBuilder
return Counter::getCounter(CounterMap[S]);
}
+ std::pair getBranchCounterPair(const Stmt *S,
+ Counter ParentCnt) {
+Counter ExecCnt = getRegionCounter(S);
+return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ }
+
+ bool IsCounterEqual(Counter OutCount, Counter ParentCount) {
ornata wrote:
Did you try this?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -941,6 +941,19 @@ struct CounterCoverageMappingBuilder return Counter::getCounter(CounterMap[S]); } + std::pair getBranchCounterPair(const Stmt *S, ornata wrote: I think changing the name is sufficient for making me less confused https://github.com/llvm/llvm-project/pull/112702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1709,6 +1730,13 @@ struct CounterCoverageMappingBuilder : addCounters( addCounters(ParentCount, BackedgeCount, BodyBC.ContinueCount), IncrementBC.ContinueCount); +auto [ExecCount, ExitCount] = +(llvm::EnableSingleByteCoverage ornata wrote: if it's going to change further, then this is fine as-is https://github.com/llvm/llvm-project/pull/112702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1592,6 +1605,13 @@ struct CounterCoverageMappingBuilder
llvm::EnableSingleByteCoverage
? getRegionCounter(S->getCond())
: addCounters(ParentCount, BackedgeCount, BC.ContinueCount);
+auto [ExecCount, ExitCount] =
+(llvm::EnableSingleByteCoverage
+ ? std::make_pair(getRegionCounter(S), Counter::getZero())
+ : getBranchCounterPair(S, CondCount));
+if (!llvm::EnableSingleByteCoverage) {
ornata wrote:
Do you mean that more stuff will be under this `if`?
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -1804,9 +1832,10 @@ struct CounterCoverageMappingBuilder
Counter LoopCount =
addCounters(ParentCount, BackedgeCount, BC.ContinueCount);
-Counter OutCount =
-addCounters(BC.BreakCount, subtractCounters(LoopCount, BodyCount));
-if (OutCount != ParentCount) {
+auto [ExecCount, ExitCount] = getBranchCounterPair(S, LoopCount);
+assert(ExecCount.isZero() || ExecCount == BodyCount);
+Counter OutCount = addCounters(BC.BreakCount, ExitCount);
ornata wrote:
I can't remember what I wanted here.
https://github.com/llvm/llvm-project/pull/112702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce `getBranchCounterPair()`. NFC. (PR #112702)
@@ -2221,27 +2249,27 @@ struct CounterCoverageMappingBuilder extendRegion(E->getRHS()); propagateCounts(getRegionCounter(E), E->getRHS()); +if (llvm::EnableSingleByteCoverage) ornata wrote: I think that in the future, it would be really useful to add debug output to this code. It is becoming quite complex, with all of the types of coverage... https://github.com/llvm/llvm-project/pull/112702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Coverage] Introduce the type `CounterPair` for RegionCounterMap. NFC. (PR #112724)
@@ -362,6 +362,8 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D, return GV; } + PGO.markStmtMaybeUsed(D.getInit()); // FIXME: Too lazy ornata wrote: Too lazy? https://github.com/llvm/llvm-project/pull/112724 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Introduce SingleByteCoverage tests (w/yaml2obj) (PR #113114)
https://github.com/ornata approved this pull request. I don't see any problem with this change. https://github.com/llvm/llvm-project/pull/113114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
