[PATCH] D50678: [InlineAsm] Update the min-legal-vector-width function attribute based on inputs and outputs to inline assembly

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

This makes sense to me, but definitely wait for someone more familiar w/ 
Clang's IR gen to review...


https://reviews.llvm.org/D50678



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

[PATCH] D47814: Teach libc++ to use native NetBSD's max_align_t

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D47814#1206372, @krytarowski wrote:

> If there are no more comments, I will land this by the end of this week.


Just for the record, this is not OK and not how LLVM's code review works.

You can and must wait for review. I think Joerg already marked this as 
accepted, but it didn't send out an email[1]. That means it got reviewed, so 
all is good here, but I wanted to make it clear to others on the list what the 
expectations are here.

-Chandler

[1]: FYI, it's useful to put *some* text in the text box when marking a 
revision as accepted on Phab so that it will in fact send email. I typically 
put "LGTM" or a brief thank-you note to the author.


Repository:
  rL LLVM

https://reviews.llvm.org/D47814



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

[PATCH] D47814: Teach libc++ to use native NetBSD's max_align_t

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

(To be clear, this continues to not be related to this patch, but happy to 
discuss...)




Comment at: include/stddef.h:55
 
 // Re-use the compiler's  max_align_t where possible.
 #if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \

ldionne wrote:
> chandlerc wrote:
> > Unrelated to your patch, but this comment is now amazingly out of place.
> Nope, based on `git blame` I think it's still where it should be.
I don't understand ... I'm not saying this patch has anything to do with it. 
But commenting *this* block with the above comment doesn't parse for me at 
all... Maybe its that the wording of the comment doesn't parse for me.

Perhaps what it means is "Only provide our own max_align_t (rather than relying 
on a compiler-provided one) when we have to by testing for the cases where the 
compiler's  won't work."?


Repository:
  rCXX libc++

https://reviews.llvm.org/D47814



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

[PATCH] D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
chandlerc added reviewers: echristo, rnk, craig.topper.
Herald added subscribers: hiraditya, mcrosier, sanjoy.
Herald added a reviewer: javed.absar.

This is in preparation for enabling *only* the call retpolines when
using speculative load hardening.

I've continued to use subtarget features for now as they continue to
seem the best fit given the lack of other retpoline like constructs so
far.

The LLVM side is pretty simple. I'd like to eventually get rid of the
old feature, but not sure what backwards compatibility issues that will
cause.

This does remove the "implies" from requesting an external thunk. This
always seemed somewhat questionable and is now clearly not desirable --
you specify a thunk the same way no matter which set of things are
getting retpolines.

I really want to keep this nicely isolated from end users and just an
LLVM implementation detail, so I've moved the `-mretpoline` flag in
Clang to no longer rely on a specific subtarget feature by that name and
instead to be directly handled. In some ways this is simpler, but in
order to preserve existing behavior I've had to add some fallback code
so that users who relied on merely passing -mretpoline-external-thunk
continue to get the same behavior. We should eventually remove this
I suspect (we have never tested that it works!) but I've not done that
in this patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D51150

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h
  clang/lib/Driver/ToolChains/Arch/X86.cpp
  clang/test/Driver/x86-target-features.c
  llvm/lib/Target/X86/X86.td
  llvm/lib/Target/X86/X86FastISel.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86InstrCompiler.td
  llvm/lib/Target/X86/X86InstrControl.td
  llvm/lib/Target/X86/X86InstrInfo.td
  llvm/lib/Target/X86/X86MCInstLower.cpp
  llvm/lib/Target/X86/X86RetpolineThunks.cpp
  llvm/lib/Target/X86/X86Subtarget.h
  llvm/test/CodeGen/X86/retpoline-external.ll
  llvm/test/CodeGen/X86/retpoline-regparm.ll
  llvm/test/CodeGen/X86/retpoline.ll

Index: llvm/test/CodeGen/X86/retpoline.ll
===
--- llvm/test/CodeGen/X86/retpoline.ll
+++ llvm/test/CodeGen/X86/retpoline.ll
@@ -156,7 +156,7 @@
 ; X86FAST:   jmp direct_callee # TAILCALL
 
 
-declare void @nonlazybind_callee() #1
+declare void @nonlazybind_callee() #2
 
 define void @nonlazybind_caller() #0 {
   call void @nonlazybind_callee()
@@ -183,6 +183,153 @@
 ; X86FAST:   jmp nonlazybind_callee@PLT # TAILCALL
 
 
+; Check that a switch gets lowered using a jump table when retpolines are only
+; enabled for calls.
+define void @switch_jumptable(i32* %ptr, i64* %sink) #0 {
+; X64-LABEL: switch_jumptable:
+; X64: jmpq *
+; X86-LABEL: switch_jumptable:
+; X86: jmpl *
+entry:
+  br label %header
+
+header:
+  %i = load volatile i32, i32* %ptr
+  switch i32 %i, label %bb0 [
+i32 1, label %bb1
+i32 2, label %bb2
+i32 3, label %bb3
+i32 4, label %bb4
+i32 5, label %bb5
+i32 6, label %bb6
+i32 7, label %bb7
+i32 8, label %bb8
+i32 9, label %bb9
+  ]
+
+bb0:
+  store volatile i64 0, i64* %sink
+  br label %header
+
+bb1:
+  store volatile i64 1, i64* %sink
+  br label %header
+
+bb2:
+  store volatile i64 2, i64* %sink
+  br label %header
+
+bb3:
+  store volatile i64 3, i64* %sink
+  br label %header
+
+bb4:
+  store volatile i64 4, i64* %sink
+  br label %header
+
+bb5:
+  store volatile i64 5, i64* %sink
+  br label %header
+
+bb6:
+  store volatile i64 6, i64* %sink
+  br label %header
+
+bb7:
+  store volatile i64 7, i64* %sink
+  br label %header
+
+bb8:
+  store volatile i64 8, i64* %sink
+  br label %header
+
+bb9:
+  store volatile i64 9, i64* %sink
+  br label %header
+}
+
+
+@indirectbr_preserved.targets = constant [10 x i8*] [i8* blockaddress(@indirectbr_preserved, %bb0),
+ i8* blockaddress(@indirectbr_preserved, %bb1),
+ i8* blockaddress(@indirectbr_preserved, %bb2),
+ i8* blockaddress(@indirectbr_preserved, %bb3),
+ i8* blockaddress(@indirectbr_preserved, %bb4),
+ i8* blockaddress(@indirectbr_preserved, %bb5),
+ i8* blockaddress(@indirectbr_preserved, %bb6),
+ i8* blockaddress(@indirectbr_preserved, %bb7),
+ i8* blockaddress(@indirectbr_preserved, %bb8),
+ i8* blockaddress(@indirectbr_preserved, %bb9)]
+
+; Check that we preserve indirectbr when only call

[PATCH] D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Thanks!

I'm going to go ahead and land this, but happy to iterate on anything if others 
have comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D51150



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

[PATCH] D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC340515: [x86/retpoline] Split the LLVM concept of retpolines 
into separate (authored by chandlerc, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51150?vs=162130&id=162138#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51150

Files:
  include/clang/Driver/Options.td
  lib/Basic/Targets/X86.cpp
  lib/Basic/Targets/X86.h
  lib/Driver/ToolChains/Arch/X86.cpp
  test/Driver/x86-target-features.c

Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1999,6 +1999,9 @@
 def mno_soft_float : Flag<["-"], "mno-soft-float">, Group;
 def mno_stackrealign : Flag<["-"], "mno-stackrealign">, Group;
 
+def mretpoline : Flag<["-"], "mretpoline">, Group, Flags<[CoreOption,DriverOption]>;
+def mno_retpoline : Flag<["-"], "mno-retpoline">, Group, Flags<[CoreOption,DriverOption]>;
+
 def mrelax : Flag<["-"], "mrelax">, Group,
   HelpText<"Enable linker relaxation">;
 def mno_relax : Flag<["-"], "mno-relax">, Group,
@@ -2824,8 +2827,6 @@
 def mno_xsaves : Flag<["-"], "mno-xsaves">, Group;
 def mshstk : Flag<["-"], "mshstk">, Group;
 def mno_shstk : Flag<["-"], "mno-shstk">, Group;
-def mretpoline : Flag<["-"], "mretpoline">, Group;
-def mno_retpoline : Flag<["-"], "mno-retpoline">, Group;
 def mretpoline_external_thunk : Flag<["-"], "mretpoline-external-thunk">, Group;
 def mno_retpoline_external_thunk : Flag<["-"], "mno-retpoline-external-thunk">, Group;
 
Index: test/Driver/x86-target-features.c
===
--- test/Driver/x86-target-features.c
+++ test/Driver/x86-target-features.c
@@ -132,8 +132,8 @@
 
 // RUN: %clang -target i386-linux-gnu -mretpoline %s -### -o %t.o 2>&1 | FileCheck -check-prefix=RETPOLINE %s
 // RUN: %clang -target i386-linux-gnu -mno-retpoline %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-RETPOLINE %s
-// RETPOLINE: "-target-feature" "+retpoline"
-// NO-RETPOLINE: "-target-feature" "-retpoline"
+// RETPOLINE: "-target-feature" "+retpoline-indirect-calls" "-target-feature" "+retpoline-indirect-branches"
+// NO-RETPOLINE-NOT: retpoline
 
 // RUN: %clang -target i386-linux-gnu -mretpoline -mretpoline-external-thunk %s -### -o %t.o 2>&1 | FileCheck -check-prefix=RETPOLINE-EXTERNAL-THUNK %s
 // RUN: %clang -target i386-linux-gnu -mretpoline -mno-retpoline-external-thunk %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-RETPOLINE-EXTERNAL-THUNK %s
Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -144,6 +144,26 @@
   Features.push_back("+ssse3");
   }
 
+  // Translate the high level `-mretpoline` flag to the specific target feature
+  // flags. We also detect if the user asked for retpoline external thunks but
+  // failed to ask for retpolines themselves. This is a bit hacky but keeps
+  // existing usages working. We should consider deprecated this and instead
+  // warning if the user requests external retpoline thunks and *doesn't*
+  // request some form of retpolines.
+  if (Args.hasArgNoClaim(options::OPT_mretpoline, options::OPT_mno_retpoline)) {
+if (Args.hasFlag(options::OPT_mretpoline, options::OPT_mno_retpoline,
+ false)) {
+  Features.push_back("+retpoline-indirect-calls");
+  Features.push_back("+retpoline-indirect-branches");
+}
+  } else if (Args.hasFlag(options::OPT_mretpoline_external_thunk,
+  options::OPT_mno_retpoline_external_thunk, false)) {
+// FIXME: Add a warning about failing to specify `-mretpoline` and
+// eventually switch to an error here.
+Features.push_back("+retpoline-indirect-calls");
+Features.push_back("+retpoline-indirect-branches");
+  }
+
   // Now add any that the user explicitly requested on the command line,
   // which may override the defaults.
   handleTargetFeaturesGroup(Args, Features, options::OPT_m_x86_Features_Group);
Index: lib/Basic/Targets/X86.cpp
===
--- lib/Basic/Targets/X86.cpp
+++ lib/Basic/Targets/X86.cpp
@@ -796,8 +796,6 @@
   HasCLDEMOTE = true;
 } else if (Feature == "+rdpid") {
   HasRDPID = true;
-} else if (Feature == "+retpoline") {
-  HasRetpoline = true;
 } else if (Feature == "+retpoline-external-thunk") {
   HasRetpolineExternalThunk = true;
 } else if (Feature == "+sahf") {
@@ -1397,7 +1395,6 @@
   .Case("rdpid", HasRDPID)
   .Case("rdrnd", HasRDRND)
   .Case("rdseed", HasRDSEED)
-  .Case("retpoline", HasRetpoline)
   .Case("retpoline-external-thunk", HasRetpolineExternalThunk)
   .Case("rtm", HasRTM)
   .Case("sahf", HasLAHFSAHF)
Index: lib/Basic/Targets/X86.h

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
chandlerc added reviewers: rsmith, rnk, echristo, craig.topper, kristof.beyls.
Herald added subscribers: jfb, dexonsmith, steven_wu, hiraditya, eraman, 
mcrosier, mehdi_amini, sanjoy.

Wires up the existing pass to work with a proper IR attribute rather
than just a hidden/internal flag. The internal flag continues to work
for now, but I'll likely remove it soon.

Most of the churn here is adding the IR attribute. I talked about this
Kristof Beyls and he seemed at least initially OK with this direction.
The idea of using a full attribute here is that we *do* expect at least
some forms of this for other architectures. There isn't anything
*inherently* x86-specific about this technique, just that we only have
an implementation for x86 at the moment.

While we could potentially expose this as a Clang-level attribute as
well, that seems like a good question to defer for the moment as it
isn't 100% clear whether that or some other programmer interface (or
both?) would be best. We'll defer the programmer interface side of this
for now, but at least get to the point where the feature can be enabled
without relying on implementation details.

This also allows us to do something that was really hard before: we can
enable *just* the indirect call retpolines when using SLH. For x86, we
don't have any other way to mitigate indirect calls. Other architectures
may take a different approach of course, and none of this is surfaced to
user-level flags.

I can split this patch up into an LLVM-side and a Clang-side, but it
seemed honestly harder to review there, as this way it is clear how
these pieces are intended to be used. That said, if reviewers would like
them split, I'm happy to oblige.


Repository:
  rL LLVM

https://reviews.llvm.org/D51157

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CodeGenOptions.def
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Arch/X86.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/x86-target-features.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/O3-pipeline.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-gather.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
  llvm/test/CodeGen/X86/speculative-load-hardening.ll
  llvm/test/Transforms/Inline/attributes.ll

Index: llvm/test/Transforms/Inline/attributes.ll
===
--- llvm/test/Transforms/Inline/attributes.ll
+++ llvm/test/Transforms/Inline/attributes.ll
@@ -26,6 +26,10 @@
   ret i32 %i
 }
 
+define i32 @slh_callee(i32 %i) speculative_load_hardening {
+  ret i32 %i
+}
+
 define i32 @alwaysinline_callee(i32 %i) alwaysinline {
   ret i32 %i
 }
@@ -161,6 +165,27 @@
 ; CHECK-NEXT: ret i32
 }
 
+; Can inline a normal function into an SLH'ed function.
+define i32 @test_caller_slh(i32 %i) speculative_load_hardening {
+; CHECK-LABEL: @test_caller_slh(
+; CHECK-SAME: ) [[SLH:.*]] {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @noattr_callee(i32 %i)
+  ret i32 %callee
+}
+
+; Can inline a SLH'ed function into a normal one, dropping the SLH.
+define i32 @test_callee_slh(i32 %i) {
+; CHECK-LABEL: @test_callee_slh(i32 %i) {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @slh_callee(i32 %i)
+  ret i32 %callee
+}
+
 ; Check that a function doesn't get inlined if target-cpu strings don't match
 ; exactly.
 define i32 @test_target_cpu_callee0(i32 %i) "target-cpu"="corei7" {
@@ -384,6 +409,7 @@
 ; CHECK-NEXT: ret i32
 }
 
+; CHECK: attributes [[SLH]] = { speculative_load_hardening }
 ; CHECK: attributes [[FPMAD_FALSE]] = { "less-precise-fpmad"="false" }
 ; CHECK: attributes [[FPMAD_TRUE]] = { "less-precise-fpmad"="true" }
 ; CHECK: attributes [[NOIMPLICITFLOAT]] = { noimplicitfloat }
Index: llvm/test/CodeGen/X86/speculative-load-hardening.ll
===
--- llvm/test/CodeGen/X86/speculative-load-hardening.ll
+++ llvm/test/CodeGen/X86/speculative-load-hardening.ll
@@ -1,14 +1,14 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening | FileCheck %s --check-prefix=X64
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
+; RUN: llc < %s -mtriple=x86_64-un

[PATCH] D46593: Allow copy elision in path concatenation

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Sorry folks, but you can't just take patches to libstdc++ and apply them to 
libc++.

These libraries have different licenses, and so the author of the patch 
(Jonathan Wakely in this case) need's to *explicitly* contribute that patch to 
libc++ under libc++'s license. (Or potentially an employer who owns the rights 
to it.)

So this needs to be reverted, or Jonathan needs to hop on this thread and very 
explicitly give permission to relicense this and contribute it to LLVM. 
Generally, I would strongly encourage the original authors making contributions 
as it simplifies everything.


Repository:
  rCXX libc++

https://reviews.llvm.org/D46593



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

[PATCH] D46593: Allow copy elision in path concatenation

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D46593#1093758, @jwakely wrote:

> @chandlerc thanks for catching this.
>
> As the original author I agree to contribute this patch to libc++ under the 
> terms of the MIT and the University of Illinois licences, and under the terms 
> of "Apache 2 with LLVM exception" if necessary in future.
>
> This permission applies only to this patch, not any other code from GCC.


Thanks so much for the fast response! And yeah, let's try to follow a more 
obvious process in the future. =D


Repository:
  rCXX libc++

https://reviews.llvm.org/D46593



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

[PATCH] D38824: [X86] Synchronize the CPU predefined macros with gcc

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: lib/Basic/Targets/X86.cpp:844-845
-// FIXME: Historically, we defined this legacy name, it would be nice to
-// remove it at some point. We've never exposed fine-grained names for
-// recent primary x86 CPUs, and we should keep it that way.
-defineCPUMacros(Builder, "corei7");

This seems to undo the idea that we should keep avoiding exposing fine-grained 
CPU names? What's new that changes this?



Comment at: lib/Basic/Targets/X86.cpp:852
+defineCPUMacros(Builder, "core_avx2");
+defineCPUMacros(Builder, "haswell");
 break;

I find calling a Westmere CPU `nehalem` a little odd. Calling IvyBridge a 
`sandybridge' CPU seems quite confusing. But calling Skylake (client) and 
Cannonlake (all? client?) `haswell` seems  deeply weird.


https://reviews.llvm.org/D38824



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

[PATCH] D38824: [X86] Synchronize the CPU predefined macros with gcc

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: lib/Basic/Targets/X86.cpp:844-845
-// FIXME: Historically, we defined this legacy name, it would be nice to
-// remove it at some point. We've never exposed fine-grained names for
-// recent primary x86 CPUs, and we should keep it that way.
-defineCPUMacros(Builder, "corei7");

craig.topper wrote:
> chandlerc wrote:
> > This seems to undo the idea that we should keep avoiding exposing 
> > fine-grained CPU names? What's new that changes this?
> CPUs newer than the ones with that comment seem to have ignored said comment.
> 
> Probably be cause we don't have a definition for what to do for new CPUs if 
> we aren't going to expose fine grained names. Do we just call everything 
> corei7 forever?
My hope would have been that people use *ISA*-based macros rather than anything 
w/ a specific CPU. So I would have *removed* the corei7 for future CPUs and 
simply not defined anything for them at all.

I think exposing something beyond ISA featureset through preprocessor macros is 
questionable at best. I would at least want to see concrete use cases first.



Comment at: lib/Basic/Targets/X86.cpp:852
+defineCPUMacros(Builder, "core_avx2");
+defineCPUMacros(Builder, "haswell");
 break;

craig.topper wrote:
> chandlerc wrote:
> > I find calling a Westmere CPU `nehalem` a little odd. Calling IvyBridge a 
> > `sandybridge' CPU seems quite confusing. But calling Skylake (client) and 
> > Cannonlake (all? client?) `haswell` seems  deeply weird.
> This implementation matches what gcc does. I agree its weird.
> 
> gcc doesn't implement cannonlake yet so i don't know what they'll do.
Ok, but maybe we should ask GCC to stop doing this. ;] We could talk to them 
and try to figure out the right strategy is. My proposed strategy (see other 
comment) is to restrict macros to ISA facilities.


https://reviews.llvm.org/D38824



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 162319.
chandlerc marked 3 inline comments as done.
chandlerc added a comment.

Address review comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D51157

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CodeGenOptions.def
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Arch/X86.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/x86-target-features.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/O3-pipeline.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-gather.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
  llvm/test/CodeGen/X86/speculative-load-hardening.ll
  llvm/test/Transforms/Inline/attributes.ll

Index: llvm/test/Transforms/Inline/attributes.ll
===
--- llvm/test/Transforms/Inline/attributes.ll
+++ llvm/test/Transforms/Inline/attributes.ll
@@ -26,6 +26,10 @@
   ret i32 %i
 }
 
+define i32 @slh_callee(i32 %i) speculative_load_hardening {
+  ret i32 %i
+}
+
 define i32 @alwaysinline_callee(i32 %i) alwaysinline {
   ret i32 %i
 }
@@ -161,6 +165,27 @@
 ; CHECK-NEXT: ret i32
 }
 
+; Can inline a normal function into an SLH'ed function.
+define i32 @test_caller_slh(i32 %i) speculative_load_hardening {
+; CHECK-LABEL: @test_caller_slh(
+; CHECK-SAME: ) [[SLH:.*]] {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @noattr_callee(i32 %i)
+  ret i32 %callee
+}
+
+; Can inline a SLH'ed function into a normal one, dropping the SLH.
+define i32 @test_callee_slh(i32 %i) {
+; CHECK-LABEL: @test_callee_slh(i32 %i) {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @slh_callee(i32 %i)
+  ret i32 %callee
+}
+
 ; Check that a function doesn't get inlined if target-cpu strings don't match
 ; exactly.
 define i32 @test_target_cpu_callee0(i32 %i) "target-cpu"="corei7" {
@@ -384,6 +409,7 @@
 ; CHECK-NEXT: ret i32
 }
 
+; CHECK: attributes [[SLH]] = { speculative_load_hardening }
 ; CHECK: attributes [[FPMAD_FALSE]] = { "less-precise-fpmad"="false" }
 ; CHECK: attributes [[FPMAD_TRUE]] = { "less-precise-fpmad"="true" }
 ; CHECK: attributes [[NOIMPLICITFLOAT]] = { noimplicitfloat }
Index: llvm/test/CodeGen/X86/speculative-load-hardening.ll
===
--- llvm/test/CodeGen/X86/speculative-load-hardening.ll
+++ llvm/test/CodeGen/X86/speculative-load-hardening.ll
@@ -1,14 +1,14 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening | FileCheck %s --check-prefix=X64
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
 ;
 ; FIXME: Add support for 32-bit and other EH ABIs.
 
 declare void @leak(i32 %v1, i32 %v2)
 
 declare void @sink(i32)
 
-define i32 @test_trivial_entry_load(i32* %ptr) {
+define i32 @test_trivial_entry_load(i32* %ptr) speculative_load_hardening {
 ; X64-LABEL: test_trivial_entry_load:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:movq %rsp, %rcx
@@ -29,7 +29,7 @@
   ret i32 %v
 }
 
-define void @test_basic_conditions(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2, i32** %ptr3) {
+define void @test_basic_conditions(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2, i32** %ptr3) speculative_load_hardening {
 ; X64-LABEL: test_basic_conditions:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %r15
@@ -189,7 +189,7 @@
   ret void
 }
 
-define void @test_basic_loop(i32 %a, i32 %b, i32* %ptr1, i32* %ptr2) nounwind {
+define void @test_basic_loop(i32 %a, i32 %b, i32* %ptr1, i32* %ptr2) nounwind speculative_load_hardening {
 ; X64-LABEL: test_basic_loop:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %rbp
@@ -293,7 +293,7 @@
   ret void
 }
 
-define void @test_basic_nested_loop(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2) nounwind {
+define void @test_basic_nested_loop(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2) nounwind speculative_load_hardening {
 ; X64-LABEL: test_basic_nested_loop:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %rbp
@@ -48

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Thanks, should all be addressed now.




Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:169-170
   options::OPT_mno_retpoline_external_thunk, false)) {
 // FIXME: Add a warning about failing to specify `-mretpoline` and
 // eventually switch to an error here.
 Features.push_back("+retpoline-indirect-calls");

rsmith wrote:
> `-mspeculative-load-hardening -mretpoline-external-thunk` does not enable 
> `+retpoline-indirect-branches` (but `-mretpoline-external-thunk` by itself 
> does). Is that intentional?
Yes, as that's is specifically useful (and that is one thing that motivated my 
above comment that we should remove the ability to use 
`-mretpoline-external-thunk` by itself eventually).

You might only need SLH's minimal retpolines, but you might need to provide 
your own thunks for them in the Kernel for example.



Comment at: llvm/include/llvm/IR/Attributes.td:249
 def : MergeRule<"adjustNullPointerValidAttr">;
+

rnk wrote:
> I can't tell if this is just vim fixing the lack of a proper trailing 
> newline, but revert any whitespace change if possible.
Soryr, my annoying editor. Will fix before submit.



Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1169
+  case Attribute::SpeculativeLoadHardening:
+return 1ULL << 60;
   case Attribute::Dereferenceable:

rsmith wrote:
> rnk wrote:
> > These appear to repeat LLVMBitcodes.h, unless I am mistaken. Weird. Anyway, 
> > fixing that is out of scope.
> The values "above the fold" in Phabricator aren't all the same as the enum 
> value plus one. *shrug*
Yeah, this isn't factorable easily. Anyways


Repository:
  rL LLVM

https://reviews.llvm.org/D51157



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 162322.
chandlerc added a comment.

Add a test file that I somehow missed earlier (sorry about that).


Repository:
  rL LLVM

https://reviews.llvm.org/D51157

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CodeGenOptions.def
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Arch/X86.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/attr-speculative-load-hardening.c
  clang/test/Driver/x86-target-features.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/O3-pipeline.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-gather.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
  llvm/test/CodeGen/X86/speculative-load-hardening.ll
  llvm/test/Transforms/Inline/attributes.ll

Index: llvm/test/Transforms/Inline/attributes.ll
===
--- llvm/test/Transforms/Inline/attributes.ll
+++ llvm/test/Transforms/Inline/attributes.ll
@@ -26,6 +26,10 @@
   ret i32 %i
 }
 
+define i32 @slh_callee(i32 %i) speculative_load_hardening {
+  ret i32 %i
+}
+
 define i32 @alwaysinline_callee(i32 %i) alwaysinline {
   ret i32 %i
 }
@@ -161,6 +165,27 @@
 ; CHECK-NEXT: ret i32
 }
 
+; Can inline a normal function into an SLH'ed function.
+define i32 @test_caller_slh(i32 %i) speculative_load_hardening {
+; CHECK-LABEL: @test_caller_slh(
+; CHECK-SAME: ) [[SLH:.*]] {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @noattr_callee(i32 %i)
+  ret i32 %callee
+}
+
+; Can inline a SLH'ed function into a normal one, dropping the SLH.
+define i32 @test_callee_slh(i32 %i) {
+; CHECK-LABEL: @test_callee_slh(i32 %i) {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @slh_callee(i32 %i)
+  ret i32 %callee
+}
+
 ; Check that a function doesn't get inlined if target-cpu strings don't match
 ; exactly.
 define i32 @test_target_cpu_callee0(i32 %i) "target-cpu"="corei7" {
@@ -384,6 +409,7 @@
 ; CHECK-NEXT: ret i32
 }
 
+; CHECK: attributes [[SLH]] = { speculative_load_hardening }
 ; CHECK: attributes [[FPMAD_FALSE]] = { "less-precise-fpmad"="false" }
 ; CHECK: attributes [[FPMAD_TRUE]] = { "less-precise-fpmad"="true" }
 ; CHECK: attributes [[NOIMPLICITFLOAT]] = { noimplicitfloat }
Index: llvm/test/CodeGen/X86/speculative-load-hardening.ll
===
--- llvm/test/CodeGen/X86/speculative-load-hardening.ll
+++ llvm/test/CodeGen/X86/speculative-load-hardening.ll
@@ -1,14 +1,14 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening | FileCheck %s --check-prefix=X64
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
 ;
 ; FIXME: Add support for 32-bit and other EH ABIs.
 
 declare void @leak(i32 %v1, i32 %v2)
 
 declare void @sink(i32)
 
-define i32 @test_trivial_entry_load(i32* %ptr) {
+define i32 @test_trivial_entry_load(i32* %ptr) speculative_load_hardening {
 ; X64-LABEL: test_trivial_entry_load:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:movq %rsp, %rcx
@@ -29,7 +29,7 @@
   ret i32 %v
 }
 
-define void @test_basic_conditions(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2, i32** %ptr3) {
+define void @test_basic_conditions(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2, i32** %ptr3) speculative_load_hardening {
 ; X64-LABEL: test_basic_conditions:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %r15
@@ -189,7 +189,7 @@
   ret void
 }
 
-define void @test_basic_loop(i32 %a, i32 %b, i32* %ptr1, i32* %ptr2) nounwind {
+define void @test_basic_loop(i32 %a, i32 %b, i32* %ptr1, i32* %ptr2) nounwind speculative_load_hardening {
 ; X64-LABEL: test_basic_loop:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %rbp
@@ -293,7 +293,7 @@
   ret void
 }
 
-define void @test_basic_nested_loop(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2) nounwind {
+define void @test_basic_nested_loop(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2) nounwind speculative_load_hardening {
 ; X64-LABEL: test_basic_nested_loop:
 ; X64:  

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 162325.
chandlerc added a comment.

Move to a more conservative model suggested by Kristof.


Repository:
  rL LLVM

https://reviews.llvm.org/D51157

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CodeGenOptions.def
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Arch/X86.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/attr-speculative-load-hardening.c
  clang/test/Driver/x86-target-features.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/O3-pipeline.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-gather.ll
  llvm/test/CodeGen/X86/speculative-load-hardening-indirect.ll
  llvm/test/CodeGen/X86/speculative-load-hardening.ll
  llvm/test/Transforms/Inline/attributes.ll

Index: llvm/test/Transforms/Inline/attributes.ll
===
--- llvm/test/Transforms/Inline/attributes.ll
+++ llvm/test/Transforms/Inline/attributes.ll
@@ -26,6 +26,10 @@
   ret i32 %i
 }
 
+define i32 @slh_callee(i32 %i) speculative_load_hardening {
+  ret i32 %i
+}
+
 define i32 @alwaysinline_callee(i32 %i) alwaysinline {
   ret i32 %i
 }
@@ -161,6 +165,28 @@
 ; CHECK-NEXT: ret i32
 }
 
+; Can inline a normal function into an SLH'ed function.
+define i32 @test_caller_slh(i32 %i) speculative_load_hardening {
+; CHECK-LABEL: @test_caller_slh(
+; CHECK-SAME: ) [[SLH:.*]] {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @noattr_callee(i32 %i)
+  ret i32 %callee
+}
+
+; Can inline a SLH'ed function into a normal one, propagating SLH.
+define i32 @test_callee_slh(i32 %i) {
+; CHECK-LABEL: @test_callee_slh(
+; CHECK-SAME: ) [[SLH:.*]] {
+; CHECK-NOT: call
+; CHECK: ret i32
+entry:
+  %callee = call i32 @slh_callee(i32 %i)
+  ret i32 %callee
+}
+
 ; Check that a function doesn't get inlined if target-cpu strings don't match
 ; exactly.
 define i32 @test_target_cpu_callee0(i32 %i) "target-cpu"="corei7" {
@@ -384,6 +410,7 @@
 ; CHECK-NEXT: ret i32
 }
 
+; CHECK: attributes [[SLH]] = { speculative_load_hardening }
 ; CHECK: attributes [[FPMAD_FALSE]] = { "less-precise-fpmad"="false" }
 ; CHECK: attributes [[FPMAD_TRUE]] = { "less-precise-fpmad"="true" }
 ; CHECK: attributes [[NOIMPLICITFLOAT]] = { noimplicitfloat }
Index: llvm/test/CodeGen/X86/speculative-load-hardening.ll
===
--- llvm/test/CodeGen/X86/speculative-load-hardening.ll
+++ llvm/test/CodeGen/X86/speculative-load-hardening.ll
@@ -1,14 +1,14 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening | FileCheck %s --check-prefix=X64
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-speculative-load-hardening -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -x86-slh-lfence | FileCheck %s --check-prefix=X64-LFENCE
 ;
 ; FIXME: Add support for 32-bit and other EH ABIs.
 
 declare void @leak(i32 %v1, i32 %v2)
 
 declare void @sink(i32)
 
-define i32 @test_trivial_entry_load(i32* %ptr) {
+define i32 @test_trivial_entry_load(i32* %ptr) speculative_load_hardening {
 ; X64-LABEL: test_trivial_entry_load:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:movq %rsp, %rcx
@@ -29,7 +29,7 @@
   ret i32 %v
 }
 
-define void @test_basic_conditions(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2, i32** %ptr3) {
+define void @test_basic_conditions(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2, i32** %ptr3) speculative_load_hardening {
 ; X64-LABEL: test_basic_conditions:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %r15
@@ -189,7 +189,7 @@
   ret void
 }
 
-define void @test_basic_loop(i32 %a, i32 %b, i32* %ptr1, i32* %ptr2) nounwind {
+define void @test_basic_loop(i32 %a, i32 %b, i32* %ptr1, i32* %ptr2) nounwind speculative_load_hardening {
 ; X64-LABEL: test_basic_loop:
 ; X64:   # %bb.0: # %entry
 ; X64-NEXT:pushq %rbp
@@ -293,7 +293,7 @@
   ret void
 }
 
-define void @test_basic_nested_loop(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2) nounwind {
+define void @test_basic_nested_loop(i32 %a, i32 %b, i32 %c, i32* %ptr1, i32* %ptr2) nounwind speculative_load_hardening {
 ; X64-LABEL: test_basic_nested_loop:
 ; 

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

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



Comment at: llvm/docs/LangRef.rst:1659-1661
+that hardening. It should also be possible to *not* harden a hot and/or 
safe
+function and have code inlined there *not* be hardened (even if the generic
+form is hardened).

kristof.beyls wrote:
> It feels wrong to me to have source code that is annotated to get hardened, 
> but that actually will not get hardened (whether it is due to it being 
> inlined somewhere or due to any other automatic 
> behind-the-back-of-the-developer transformation). I fear this may lower trust 
> in the protection this attribute provides.
> I assume there is a use case where the developer wants to indicate "no 
> hardening in this function nor in any functions inlined here". If that needs 
> to be supported, my feel is that we may need to support that in another way.
> I guess that there must be some cases where just duplicating the function to 
> be inlined in the source code into a hardened and a non-hardened version 
> could be too hard to do for some programs.
> So, in short, I don't know what the best solution here is. I just want to 
> raise my concern that I don't think it's a good idea that functions that are 
> marked to be hardened end up not getting hardened under some circumstances.
I actually started out with this, so I'm very sympathetic to the perspective.

Given that you prefer it, let's start with this very conservative stance. We 
can always add specific mechanisms to be less conservative later, but I agree 
that it seems much better to start with a safe position.


Repository:
  rL LLVM

https://reviews.llvm.org/D51157



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

[PATCH] D51567: CMake: Consolidate gtest detection code

[Chandler Carruth via Phabricator via cfe-commits]

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

I mean, sure.

I really don't know that supporting this ever expanding diversity of build 
strategies is worth its cost, but I don't see a specific reason to not take 
this patch


Repository:
  rC Clang

https://reviews.llvm.org/D51567



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc marked 5 inline comments as done.
chandlerc added a comment.

All outstanding comments addressed, and landing this. Thanks all for the 
reviews and let me know if I missed anything.




Comment at: llvm/include/llvm/IR/Attributes.td:181-185
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and the default merge strategy of retaining the caller's
+/// attribute. This specifically matches the intent for this attribute which is
+/// that the context dominates, and inlined code will become hardened or lose
+/// its hardening based on the caller's attribute.

kristof.beyls wrote:
> After updating the LangRef.rst text, I think this comment also needs to be 
> updated. As is, it still documents the old inlining behaviour?
> I'm also not sure in how far the comment makes most sense here. This is 
> already documented in LangRef.rst, and AFAIK, the inlining compatibility mode 
> is not something that is defined here?
Updated the comment.

You can override the compatibility here -- see the `CompatRule` productions 
just before the `MergeRule` ones.

I'd like this to be documented near the code in addition to in the langref 
personally. It reminds the reader to look below at the rule sections.



Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:78-82
+static cl::opt EnableSpeculativeLoadHardening(
+"x86-speculative-load-hardening",
+cl::desc("Force enable speculative load hardening"), cl::init(false),
+cl::Hidden);
+

kristof.beyls wrote:
> I'm not sure, but do you really still need an option to force enable SLH when 
> you have function attributes now to enable it?
> When you generate LLVM-IR using clang, you now have a clang option to enable 
> that function attribute on all functions, so do you still have scenarios 
> where you need an LLVM backend option to override the function attribute?
Long term, no, we don't.

I just have some users that are already passing this flag. Out of convenience, 
I wanted to leave this working until they have picked up the new version of 
Clang and LLVM and migrated. I don't expect it to last long (a week or two?).


Repository:
  rL LLVM

https://reviews.llvm.org/D51157



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

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
chandlerc marked an inline comment as done.
Closed by commit rL341363: [x86/SLH] Add a real Clang flag and LLVM IR 
attribute for Speculative (authored by chandlerc, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51157?vs=162325&id=163789#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51157

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/attr-speculative-load-hardening.c
  cfe/trunk/test/Driver/x86-target-features.c
  llvm/trunk/docs/LangRef.rst
  llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/trunk/include/llvm/IR/Attributes.td
  llvm/trunk/lib/AsmParser/LLLexer.cpp
  llvm/trunk/lib/AsmParser/LLParser.cpp
  llvm/trunk/lib/AsmParser/LLToken.h
  llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/trunk/lib/IR/Attributes.cpp
  llvm/trunk/lib/IR/Verifier.cpp
  llvm/trunk/lib/Target/X86/X86SpeculativeLoadHardening.cpp
  llvm/trunk/lib/Target/X86/X86TargetMachine.cpp
  llvm/trunk/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/trunk/test/CodeGen/X86/O0-pipeline.ll
  llvm/trunk/test/CodeGen/X86/O3-pipeline.ll
  llvm/trunk/test/CodeGen/X86/speculative-load-hardening-gather.ll
  llvm/trunk/test/CodeGen/X86/speculative-load-hardening.ll
  llvm/trunk/test/Transforms/Inline/attributes.ll

Index: llvm/trunk/docs/LangRef.rst
===
--- llvm/trunk/docs/LangRef.rst
+++ llvm/trunk/docs/LangRef.rst
@@ -1636,6 +1636,28 @@
 This attribute indicates that HWAddressSanitizer checks
 (dynamic address safety analysis based on tagged pointers) are enabled for
 this function.
+``speculative_load_hardening``
+This attribute indicates that
+`Speculative Load Hardening `_
+should be enabled for the function body. This is a best-effort attempt to
+mitigate all known speculative execution information leak vulnerabilities
+that are based on the fundamental principles of modern processors'
+speculative execution. These vulnerabilities are classified as "Spectre
+variant #1" vulnerabilities typically. Notably, this does not attempt to
+mitigate any vulnerabilities where the speculative execution and/or
+prediction devices of specific processors can be *completely* undermined
+(such as "Branch Target Injection", a.k.a, "Spectre variant #2"). Instead,
+this is a target-independent request to harden against the completely
+generic risk posed by speculative execution to incorrectly load secret data,
+making it available to some micro-architectural side-channel for information
+leak. For a processor without any speculative execution or predictors, this
+is expected to be a no-op.
+
+When inlining, the attribute is sticky. Inlining a function that carries
+this attribute will cause the caller to gain the attribute. This is intended
+to provide a maximally conservative model where the code in a function
+annotated with this attribute will always (even after inlining) end up
+hardened.
 ``speculatable``
 This function attribute indicates that the function does not have any
 effects besides calculating its result and does not have undefined behavior.
Index: llvm/trunk/include/llvm/IR/Attributes.td
===
--- llvm/trunk/include/llvm/IR/Attributes.td
+++ llvm/trunk/include/llvm/IR/Attributes.td
@@ -176,6 +176,14 @@
 /// HWAddressSanitizer is on.
 def SanitizeHWAddress : EnumAttr<"sanitize_hwaddress">;
 
+/// Speculative Load Hardening is enabled.
+///
+/// Note that this uses the default compatibility (always compatible during
+/// inlining) and a conservative merge strategy where inlining an attributed
+/// body will add the attribute to the caller. This ensures that code carrying
+/// this attribute will always be lowered with hardening enabled.
+def SpeculativeLoadHardening : EnumAttr<"speculative_load_hardening">;
+
 /// Argument is swift error.
 def SwiftError : EnumAttr<"swifterror">;
 
@@ -232,6 +240,7 @@
 def : MergeRule<"setOR">;
 def : MergeRule<"setOR">;
 def : MergeRule<"setOR">;
+def : MergeRule<"setOR">;
 def : MergeRule<"adjustCallerSSPLevel">;
 def : MergeRule<"adjustCallerStackProbes">;
 def : MergeRule<"adjustCallerStackProbeSize">;
Index: llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
===
--- llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
+++ llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h
@@ -591,6 +591,7 @@
   ATTR_KIND_NOCF_CHECK = 

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: docs/DeveloperPolicy.rst:395-408
+Commits with No Functional Change
+-
+
+It may be permissible to commit changes without prior review when the changes
+have no semantic impact on the code if the changes being made are obvious and
+not invasive. For instance, removing trailing whitespace from a line, fixing a
+line ending to be consistent with the rest of the file, fixing a typo, code

I think this is a much broader statement than is warranted to address the 
specific problem. And I'm not completely comfortable with it.

I don't think guidance around "no functional change" is the right way to give 
guidance about what is or isn't "obvious" and fine to commit with post-commit 
review personally.

I'd really suggest just being direct about *formatting* and whitespace changes, 
and specifically suggest that they not be made unless the file or code region 
in question is an area that the author is planning subsequent changes to.


https://reviews.llvm.org/D50055



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

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: docs/DeveloperPolicy.rst:395-408
+Commits with No Functional Change
+-
+
+It may be permissible to commit changes without prior review when the changes
+have no semantic impact on the code if the changes being made are obvious and
+not invasive. For instance, removing trailing whitespace from a line, fixing a
+line ending to be consistent with the rest of the file, fixing a typo, code

hubert.reinterpretcast wrote:
> hfinkel wrote:
> > aaron.ballman wrote:
> > > chandlerc wrote:
> > > > I think this is a much broader statement than is warranted to address 
> > > > the specific problem. And I'm not completely comfortable with it.
> > > > 
> > > > I don't think guidance around "no functional change" is the right way 
> > > > to give guidance about what is or isn't "obvious" and fine to commit 
> > > > with post-commit review personally.
> > > > 
> > > > I'd really suggest just being direct about *formatting* and whitespace 
> > > > changes, and specifically suggest that they not be made unless the file 
> > > > or code region in question is an area that the author is planning 
> > > > subsequent changes to.
> > > We talk about formatting and whitespace in the CodingStandards document, 
> > > but we talk about obviousness and post-commit review in DeveloperPolicy. 
> > > Where would you like these new words to live? To me, they're more about 
> > > the policy and less about the coding standard -- the coding standard says 
> > > what the code should look like and the policy says what you should and 
> > > should not do procedurally, but then I feel the need to tie it back to 
> > > "obviousness". How about this in the developer policy:
> > > ```
> > > The Obviousness of Formatting Changes
> > > -
> > > 
> > > While formatting and whitespace changes may be "obvious", they can also 
> > > create
> > > needless churn that causes difficulties for out-of-tree users carrying 
> > > local
> > > patches. Do not commit formatting or whitespace changes unless they are 
> > > related
> > > to a file or code region that you intend to make subsequent changes to. 
> > > The
> > > formatting and whitespace changes should be highly localized, committed 
> > > before
> > > you begin functionality-changing work on the code region, and the commit 
> > > message
> > > should clearly state that the commit is not intended to change 
> > > functionality,
> > > usually by stating it is :ref:`NFC `.
> > > 
> > > 
> > > If you wish to make a change to formatting or whitespace that touches an 
> > > entire
> > > library or code base, please seek pre-commit approval first.
> > > ```
> > I agree with @chandlerc about the current proposed text, and I think that 
> > this is better. I wonder if we want to insist on separating the commits, of 
> > if, combined localized commits are okay?
> > 
> It depends on how much noise there is when combining the commits; and when 
> evaluating for that, we have to remember that people use different diff tools.
I like Hal's separation in the other comment.

Here, I tihnk we can address all of this by making this more of a (strong) 
suggestion and not a hard rule.

"Avoid committing formatting or whitespace only changes outside of code you 
plan to make subsequent changes to." or something similar.

Then it also becomes natural to suggest:

"Also, try to separate formatting or whitespace changes from functional 
changes, either by correcting the format first (ideally) or afterward."

I think you can also shorten some of the discussion along these lines.


https://reviews.llvm.org/D50055



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

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.

This looks really good to me and seems like a nice clarification of historical 
practice. =D Thanks so much for driving an actual documentation update for 
folks that simply would never know about these practices otherwise, I think it 
will help folks a lot.

Maybe double check with Reid and/or Hal to make sure we've all ended up on the 
same page before landing though.


https://reviews.llvm.org/D50055



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

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+line of code. Some common editors will automatically remove trailing whitespace
+when saving a file which causes unrelated changes to appear in diffs and
+commits.

Meinersbur wrote:
> Just to note that this policy will prohibit the use of remove 
> trailing-whitespace feature in editors since it makes it impossible to edit 
> the file without also 'editing' any unrelated whitespace. At the same time, 
> since not being able to use the feature, I risk committing trailing 
> whitespace myself (that is, some additional steps are necessary: I use 'git 
> clang-format' which should remove traling whitespace only on edited lines and 
> 'git diff' shows trailing whitespace in red; these are still additional steps 
> that are easy to miss)
I also have editor settings that risk violating this, but I just reduce my 
patdh before submitting. This is a touch annoying with svn, but with got it is 
pretty simple to use `git add -p` and ignore the unnecessary removals if 
trailing whitespace 


https://reviews.llvm.org/D50055



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

[PATCH] D43203: [Driver] Generate .eh_frame_hdr for static executables too.

[Chandler Carruth via Phabricator via cfe-commits]

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

Agreed.


Repository:
  rC Clang

https://reviews.llvm.org/D43203



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to 
`lib/Passes/PassRegistry.def`?




Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:423
+
+class AARGetter {
+  FunctionAnalysisManager &AM;

timshen wrote:
> mehdi_amini wrote:
> > Can't you do it with a lambda?
> I can, except that AAR needs to be allocated outside of the lambda, on the 
> stack:
> 
> Optional AAR;
> auto F = [&AAR](Function &) -> AAResults& { ... };
> 
> (AAR can't be captured by value, since AAResults is not copyable) this way 
> AAR out-lives the lambda, and I feel less readable.
You don't need to keep a copy though... You can directly expose the reference 
returned from the `FunctionAnalysisManager` -- that reference will stay alive 
until the result is invalidated. So this will become something more like:

  [&FAM](Function &F) { return FAM.getResult(F); }


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D33525#766050, @timshen wrote:

> In https://reviews.llvm.org/D33525#764251, @chandlerc wrote:
>
> > (focusing on the LLVM side of this review for now)
> >
> > Can you add an LLVM-based test? Can you add this to 
> > `lib/Passes/PassRegistry.def`?
>
>
> Talked offline. Given the fact that BitcodeWriter (and possibly assembly 
> writer?) is not registered either, it seems to be a larger issue that's out 
> of the scope of this patch.


While *generic* testing is out of scope, I think you need at least *some* 
testing of this in this patch. It can be an option directly in the `llvm-lto` 
tool or a direct option in the `opt` tool itself, but we shouldn't add a bunch 
of code to LLVM that can only ever be exercised by using some other tool.


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:913-914
+std::error_code EC;
+ThinLinkOS.emplace(CodeGenOpts.ThinLinkBitcodeFile, EC,
+   llvm::sys::fs::F_None);
+if (EC) {

The clang side of this should probably be a separate review on cfe-commits, but 
one drive-by coment.

I find this usage of emplace much harder to read than:

  ThinLinkOS.reset(raw_fd_ostream(CodeGenOpts.ThinLinkBitcodeFile, EC,
  llvm::sys::fs::F_None));

Unless we just can't do the above for some reason (it won't compile =[) I would 
prefer it.



Comment at: llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h:31-32
+public:
+  ThinLTOBitcodeWriterPass(raw_ostream &OS, raw_ostream *ThinLinkOS)
+  : OS(OS), ThinLinkOS(ThinLinkOS) {}
+

Move (or add) a comment about this API to thea header? Notably, under what 
circumstances can `ThinLinkOS` be null? What happens if it is?



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:9-12
 //
-// This pass prepares a module containing type metadata for ThinLTO by 
splitting
-// it into regular and thin LTO parts if possible, and writing both parts to
-// a multi-module bitcode file. Modules that do not contain type metadata are
-// written unmodified as a single module.
+// Pass implementation.
 //
 
//===--===//

Can just omit the added section I suspect.



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+  writeThinLTOBitcode(OS, ThinLinkOS,
+  [&FAM](Function &F) -> AAResults & {
+return FAM.getResult(F);

Are we unable to deduce the return type here?



Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1
+; RUN: opt -passes='lto' -debug-pass-manager -thinlto-bc 
-thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM
+; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=BITCODE

Why run any passes here? 



Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }

Why an optional pointer? Maybe just allow the pointer to be null if there isn't 
a thin link output file?



Comment at: llvm/tools/opt/opt.cpp:534
+if (OutputThinLTOBC)
+  ThinLTOBC.emplace(ThinLinkOut.get());
+if (!runPassPipeline(argv[0], *M, TM.get(), Out.get(), PassPipeline, OK, 
VK,

Maybe unnecessary based on the above comment, but generally I would just assign 
rather than using emplace.



Comment at: llvm/tools/opt/opt.cpp:541-542
+
+if (ThinLinkOut)
+  ThinLinkOut->keep();
+

We do `Out->keep()` in `runPassPipeline`, why not do this there as well?


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1
+; RUN: opt -passes='lto' -debug-pass-manager -thinlto-bc 
-thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM
+; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=BITCODE

timshen wrote:
> chandlerc wrote:
> > Why run any passes here? 
> I saw that the runPassPipeline call is in  `if 
> (PassPipeline.getNumOccurrences() > 0) {`. I take that as "must specify 
> -passes" in order to run the new pass manager (which is created in 
> runPassPipeline). And specifiy an empty -passes cause an error message 
> "unable to parse pass pipeline description".
You can use '-passes=no-op-module' or something similar to at least minimize 
how much occurs using the new PM?



Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }

timshen wrote:
> chandlerc wrote:
> > Why an optional pointer? Maybe just allow the pointer to be null if there 
> > isn't a thin link output file?
> There are 3 states:
> 1) Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the 
> output file is not null.
> 2) OutputThinLTOBC is true, but output file is null (don't write to the file).
> 3) OutputThinLTOBC is false.
> 
> An optional nullable pointer, confusing as it might be (but I'm not sure how 
> much), provides 3 states.
> 
> If we instead pass in a bool and a pointer, that's 4 states, one of which is 
> invalid.
Ok, but is #2 in your list actually useful? Maybe it is, but if it is then at 
the very least the comment should make it super clear what is going on here. 
Otherwise, I suspect many readers will assume than when the optional is engaged 
the pointer isn't null as that's just too "obvious". 


https://reviews.llvm.org/D33525



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

[PATCH] D33692: [ThinLTO] Wire up ThinLTO and new PM

[Chandler Carruth via Phabricator via cfe-commits]

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

This patch LGTM whenever the underlying LLVM change lands, thanks


https://reviews.llvm.org/D33692



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

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
Herald added subscribers: mcrosier, sanjoy, klimek.

This really is a collection of improvements to the rules for LLVM
include sorting:

- We have gmock headers now, so it adds support for those to one of the 
categories.
- LLVM does use 'FooTest.cpp' files to test 'Foo.h' so it adds that suffix for 
finding a main header.
- At times the test file's case may not match the header file's case, so adds 
support for case-insensitive regex matching of these things.

With this set of changes, I can't spot any misbehaviors when re-sorting
all of LLVM's unittest '#include' lines.

Note that I don't know the first thing about testing clang-format, so please
feel free to tell me what all I need to be doing here.


https://reviews.llvm.org/D33932

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/SortIncludesTest.cpp

Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -264,6 +264,24 @@
  "#include \"c.h\"\n"
  "#include \"llvm/a.h\"\n",
  "a.cc"));
+
+  // Support case-sensitive and case insensitive matching.
+  Style.IncludeRegexCaseInsensitive = false;
+  EXPECT_EQ("#include \"b.h\"\n"
+"#include \"c.h\"\n"
+"#include \"llvm/A.h\"\n",
+sort("#include \"c.h\"\n"
+ "#include \"b.h\"\n"
+ "#include \"llvm/A.h\"\n",
+ "a_TEST.cc"));
+  Style.IncludeRegexCaseInsensitive = true;
+  EXPECT_EQ("#include \"llvm/A.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n",
+sort("#include \"c.h\"\n"
+ "#include \"b.h\"\n"
+ "#include \"llvm/A.h\"\n",
+ "a_TEST.cc"));
 }
 
 TEST_F(SortIncludesTest, NegativePriorities) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -346,6 +346,8 @@
 IO.mapOptional("ForEachMacros", Style.ForEachMacros);
 IO.mapOptional("IncludeCategories", Style.IncludeCategories);
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeIsMainRegex);
+IO.mapOptional("IncludeRegexCaseInsensitive",
+   Style.IncludeRegexCaseInsensitive);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
@@ -572,9 +574,10 @@
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IncludeCategories = {{"^\"(llvm|llvm-c|clang|clang-c)/", 2},
- {"^(<|\"(gtest|isl|json)/)", 3},
+ {"^(<|\"(gtest|gmock|isl|json)/)", 3},
  {".*", 1}};
-  LLVMStyle.IncludeIsMainRegex = "$";
+  LLVMStyle.IncludeIsMainRegex = "(Test)?$";
+  LLVMStyle.IncludeRegexCaseInsensitive = true;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
@@ -1400,7 +1403,10 @@
   : Style(Style), FileName(FileName) {
 FileStem = llvm::sys::path::stem(FileName);
 for (const auto &Category : Style.IncludeCategories)
-  CategoryRegexs.emplace_back(Category.Regex);
+  CategoryRegexs.emplace_back(Category.Regex,
+  Style.IncludeRegexCaseInsensitive
+  ? llvm::Regex::IgnoreCase
+  : llvm::Regex::NoFlags);
 IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
  FileName.endswith(".cpp") || FileName.endswith(".c++") ||
  FileName.endswith(".cxx") || FileName.endswith(".m") ||
@@ -1428,9 +1434,13 @@
   return false;
 StringRef HeaderStem =
 llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(1));
-if (FileStem.startswith(HeaderStem)) {
+if (FileStem.startswith(HeaderStem) ||
+(Style.IncludeRegexCaseInsensitive &&
+ FileStem.startswith_lower(HeaderStem))) {
   llvm::Regex MainIncludeRegex(
-  (HeaderStem + Style.IncludeIsMainRegex).str());
+  (HeaderStem + Style.IncludeIsMainRegex).str(),
+  Style.IncludeRegexCaseInsensitive ? llvm::Regex::IgnoreCase
+: llvm::Regex::NoFlags);
   if (MainIncludeRegex.match(FileStem))
 return true;
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -914,7 +914,7 @@
   ///   IncludeCategories:
   /// - Regex:   '^"(llvm|llvm-c|clang|clang-c)/'
   ///   Priority:2
-  /// - Regex:   '^(<|"(gtest|isl|json)/)'
+

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 101544.
chandlerc added a comment.

Add test coverage for the case-insensitive category logic.


https://reviews.llvm.org/D33932

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/SortIncludesTest.cpp

Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -264,6 +264,38 @@
  "#include \"c.h\"\n"
  "#include \"llvm/a.h\"\n",
  "a.cc"));
+
+  // Support case-sensitive and case insensitive matching.
+  Style.IncludeRegexCaseInsensitive = false;
+  EXPECT_EQ("#include \"GTest/GTest.h\"\n"
+"#include \"LLVM/z.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n"
+"#include \"llvm/A.h\"\n"
+"#include \"gmock/gmock.h\"\n",
+sort("#include \"c.h\"\n"
+ "#include \"b.h\"\n"
+ "#include \"GTest/GTest.h\"\n"
+ "#include \"llvm/A.h\"\n"
+ "#include \"gmock/gmock.h\"\n"
+ "#include \"LLVM/z.h\"\n",
+ "a_TEST.cc"));
+  Style.IncludeRegexCaseInsensitive = true;
+  EXPECT_EQ("#include \"llvm/A.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n"
+"#include \"LLVM/z.h\"\n"
+"#include \"llvm/X.h\"\n"
+"#include \"GTest/GTest.h\"\n"
+"#include \"gmock/gmock.h\"\n",
+sort("#include \"c.h\"\n"
+ "#include \"b.h\"\n"
+ "#include \"GTest/GTest.h\"\n"
+ "#include \"llvm/A.h\"\n"
+ "#include \"gmock/gmock.h\"\n"
+ "#include \"llvm/X.h\"\n"
+ "#include \"LLVM/z.h\"\n",
+ "a_TEST.cc"));
 }
 
 TEST_F(SortIncludesTest, NegativePriorities) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -346,6 +346,8 @@
 IO.mapOptional("ForEachMacros", Style.ForEachMacros);
 IO.mapOptional("IncludeCategories", Style.IncludeCategories);
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeIsMainRegex);
+IO.mapOptional("IncludeRegexCaseInsensitive",
+   Style.IncludeRegexCaseInsensitive);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
@@ -572,9 +574,10 @@
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IncludeCategories = {{"^\"(llvm|llvm-c|clang|clang-c)/", 2},
- {"^(<|\"(gtest|isl|json)/)", 3},
+ {"^(<|\"(gtest|gmock|isl|json)/)", 3},
  {".*", 1}};
-  LLVMStyle.IncludeIsMainRegex = "$";
+  LLVMStyle.IncludeIsMainRegex = "(Test)?$";
+  LLVMStyle.IncludeRegexCaseInsensitive = true;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
@@ -1400,7 +1403,10 @@
   : Style(Style), FileName(FileName) {
 FileStem = llvm::sys::path::stem(FileName);
 for (const auto &Category : Style.IncludeCategories)
-  CategoryRegexs.emplace_back(Category.Regex);
+  CategoryRegexs.emplace_back(Category.Regex,
+  Style.IncludeRegexCaseInsensitive
+  ? llvm::Regex::IgnoreCase
+  : llvm::Regex::NoFlags);
 IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
  FileName.endswith(".cpp") || FileName.endswith(".c++") ||
  FileName.endswith(".cxx") || FileName.endswith(".m") ||
@@ -1428,9 +1434,13 @@
   return false;
 StringRef HeaderStem =
 llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(1));
-if (FileStem.startswith(HeaderStem)) {
+if (FileStem.startswith(HeaderStem) ||
+(Style.IncludeRegexCaseInsensitive &&
+ FileStem.startswith_lower(HeaderStem))) {
   llvm::Regex MainIncludeRegex(
-  (HeaderStem + Style.IncludeIsMainRegex).str());
+  (HeaderStem + Style.IncludeIsMainRegex).str(),
+  Style.IncludeRegexCaseInsensitive ? llvm::Regex::IgnoreCase
+: llvm::Regex::NoFlags);
   if (MainIncludeRegex.match(FileStem))
 return true;
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -914,7 +914,7 @@
   ///   IncludeCategories:
   /// - Regex:   '^"(llvm|llvm-c|clang|clang-c)/'
   ///   Priority:2
-  /// - Regex:   '^(<|"(gtest|isl

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

[Chandler Carruth via Phabricator via cfe-commits]

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

Added more tests, PTAL?


https://reviews.llvm.org/D33932



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

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc updated this revision to Diff 103885.
chandlerc marked an inline comment as done.
chandlerc added a comment.

Update based on review comments.


https://reviews.llvm.org/D33932

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/SortIncludesTest.cpp

Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -266,6 +266,47 @@
  "a.cc"));
 }
 
+TEST_F(SortIncludesTest, SupportCaseInsensitiveMatching) {
+  // Setup an regex for main includes so we can cover those as well.
+  Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
+
+  // First check that case sensitive matching works correctly by having both
+  // main header and grouping that depend on case.
+  Style.IncludeRegexCaseInsensitive = false;
+  EXPECT_EQ("#include \"GTest/GTest.h\"\n"
+"#include \"LLVM/z.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n"
+"#include \"llvm/A.h\"\n"
+"#include \"gmock/gmock.h\"\n",
+sort("#include \"c.h\"\n"
+ "#include \"b.h\"\n"
+ "#include \"GTest/GTest.h\"\n"
+ "#include \"llvm/A.h\"\n"
+ "#include \"gmock/gmock.h\"\n"
+ "#include \"LLVM/z.h\"\n",
+ "a_TEST.cc"));
+
+  // Now turn insitive matching on and ensure both main header detection and
+  // grouping work in a case insensitive manner.
+  Style.IncludeRegexCaseInsensitive = true;
+  EXPECT_EQ("#include \"llvm/A.h\"\n"
+"#include \"b.h\"\n"
+"#include \"c.h\"\n"
+"#include \"LLVM/z.h\"\n"
+"#include \"llvm/X.h\"\n"
+"#include \"GTest/GTest.h\"\n"
+"#include \"gmock/gmock.h\"\n",
+sort("#include \"c.h\"\n"
+ "#include \"b.h\"\n"
+ "#include \"GTest/GTest.h\"\n"
+ "#include \"llvm/A.h\"\n"
+ "#include \"gmock/gmock.h\"\n"
+ "#include \"llvm/X.h\"\n"
+ "#include \"LLVM/z.h\"\n",
+ "a_TEST.cc"));
+}
+
 TEST_F(SortIncludesTest, NegativePriorities) {
   Style.IncludeCategories = {{".*important_os_header.*", -1}, {".*", 1}};
   EXPECT_EQ("#include \"important_os_header.h\"\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -349,6 +349,8 @@
 IO.mapOptional("ForEachMacros", Style.ForEachMacros);
 IO.mapOptional("IncludeCategories", Style.IncludeCategories);
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeIsMainRegex);
+IO.mapOptional("IncludeRegexCaseInsensitive",
+   Style.IncludeRegexCaseInsensitive);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
@@ -579,9 +581,10 @@
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IncludeCategories = {{"^\"(llvm|llvm-c|clang|clang-c)/", 2},
- {"^(<|\"(gtest|isl|json)/)", 3},
+ {"^(<|\"(gtest|gmock|isl|json)/)", 3},
  {".*", 1}};
-  LLVMStyle.IncludeIsMainRegex = "$";
+  LLVMStyle.IncludeIsMainRegex = "(Test)?$";
+  LLVMStyle.IncludeRegexCaseInsensitive = true;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
@@ -1409,7 +1412,10 @@
   : Style(Style), FileName(FileName) {
 FileStem = llvm::sys::path::stem(FileName);
 for (const auto &Category : Style.IncludeCategories)
-  CategoryRegexs.emplace_back(Category.Regex);
+  CategoryRegexs.emplace_back(Category.Regex,
+  Style.IncludeRegexCaseInsensitive
+  ? llvm::Regex::IgnoreCase
+  : llvm::Regex::NoFlags);
 IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
  FileName.endswith(".cpp") || FileName.endswith(".c++") ||
  FileName.endswith(".cxx") || FileName.endswith(".m") ||
@@ -1437,9 +1443,13 @@
   return false;
 StringRef HeaderStem =
 llvm::sys::path::stem(IncludeName.drop_front(1).drop_back(1));
-if (FileStem.startswith(HeaderStem)) {
+if (FileStem.startswith(HeaderStem) ||
+(Style.IncludeRegexCaseInsensitive &&
+ FileStem.startswith_lower(HeaderStem))) {
   llvm::Regex MainIncludeRegex(
-  (HeaderStem + Style.IncludeIsMainRegex).str());
+  (HeaderStem + Style.IncludeIsMainRegex).str(),
+  Style.IncludeRegexCaseInsensitive ? llvm::Regex::IgnoreCase
+: llvm::

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested review of this revision.
chandlerc added a comment.

Tried to address the test comment -- let me know if something else was intended.


https://reviews.llvm.org/D33932



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

[PATCH] D36836: [clang-tidy] Implement sonarsource-function-cognitive-complexity check

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D36836#931995, @lebedev.ri wrote:

> - Rebased
> - As advised by @aaron.ballman, moved into it's own directory/module. Please 
> review that, i'm not entirely sure i have done that fully correctly.
>
>   @chandlerc Hi! @aaron.ballman has suggested for me to try to talk to you 
> about this. Is there some precedent for the licensing 'issue' at hand? Do you 
> have any opinion? @dberlin did not react to the pings, so i'm not sure i 
> personally can come up with anything better for `LICENSE.txt`


Sadly, what you need here is legal advice for a good way to handle this, and 
I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of 
which what Aaron brought up that this is not in fact a license.

> If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts and can 
devote the foundation's lawyer's (sadly precious) time to this, I think either 
you or sonarsource working to understand the best legal way to contribute this 
would be the best way forward. Sorry that this is a tricky situation. Happy to 
have a private discussion w/ your or sonarsources's lawyer via my @llvm.org 
email address if useful.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D36836



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

[PATCH] D43995: Do not generate calls to fentry with __attribute__((no_instrument_function))

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc edited reviewers, added: rnk, rsmith, rjmccall; removed: chandlerc.
chandlerc added a comment.

Adding more Clang CodeGen folks to this review rather than myself...


Repository:
  rC Clang

https://reviews.llvm.org/D43995



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

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D42787#1000687, @krasimir wrote:

> We could adapt the single-argument version instead, turning:
>
>   foo(bb +
>   c);
>
>
> into:
>
>   foo(bb +
>   c);
>


I have a mild preference for this, and it seems more consistent. But I don't 
have a strong preference.

However, for each of the other cases shown in the thread so far, i strongly 
prefer clang-format's current behavior.


Repository:
  rC Clang

https://reviews.llvm.org/D42787



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

[PATCH] D6260: Add -mlong-double-64 flag

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc edited subscribers, added: brooksmoses, chandlerc; removed: rnk.
chandlerc edited reviewers, added: echristo, timshen, dlj; removed: kcc.
chandlerc added a comment.

Ok folks, I know this is a blast from the past, but can we actually review 
this? I'm willing to commandeer it, rebase it, and get it landed if you all can 
review this and confirm that it is the right direction.


https://reviews.llvm.org/D6260



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

[PATCH] D48464: [x86] Teach the builtin argument range check to allow invalid ranges in dead code.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc created this revision.
chandlerc added reviewers: craig.topper, rsmith.
Herald added subscribers: llvm-commits, atanasyan, kbarton, nemanjai, mcrosier, 
sanjoy.
Herald added a reviewer: javed.absar.

This is important for C++ templates that essentially compute the valid
input in a way that is constant and will cause all the invalid cases to
be dead code that is deleted. Code in the wild actually does this and
GCC also accepts these kinds of patterns so it is important to support
it.

To make this work, we provide a non-error path to diagnose these issues,
and use a default-error warning instead. This keeps the relatively
strict handling but prevents nastiness like SFINAE on these errors. It
also allows us to safely use the system to diagnose this only when it
occurs at runtime (in emitted code).

Entertainingly, this required fixing the syntax in various other ways
for the x86 test because we never bothered to diagnose that the returns
were invalid.

Since debugging these compile failures was super confusing, I've also
improved the diagnostic to actually say what the value was. Most of the
checks I've made ignore this to simplify maintenance, but I've checked
it in a few places to make sure the diagnsotic is working.

Depends on https://reviews.llvm.org/D48462. Without that, we might actually 
crash some part of
the compiler after bypassing the error here.

Thanks to Richard, Ben Kramer, and especially Craig Topper for all the
help here.


Repository:
  rL LLVM

https://reviews.llvm.org/D48464

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-mips-args.c
  clang/test/CodeGen/builtins-systemz-vector-error.c
  clang/test/CodeGen/builtins-systemz-vector2-error.c
  clang/test/CodeGen/builtins-systemz-zvector-error.c
  clang/test/CodeGen/builtins-systemz-zvector2-error.c
  clang/test/CodeGen/hexagon-check-builtins.c
  clang/test/Sema/aarch64-neon-fp16-ranges.c
  clang/test/Sema/aarch64-neon-ranges.c
  clang/test/Sema/arm-neon-types.c
  clang/test/Sema/builtin-object-size.c
  clang/test/Sema/builtin-prefetch.c
  clang/test/Sema/builtins-arm.c
  clang/test/Sema/builtins-arm64.c
  clang/test/Sema/builtins-ppc.c
  clang/test/Sema/builtins-x86.c
  clang/test/Sema/builtins-x86.cpp
  clang/test/SemaCXX/neon-vector-types.cpp

Index: clang/test/SemaCXX/neon-vector-types.cpp
===
--- clang/test/SemaCXX/neon-vector-types.cpp
+++ clang/test/SemaCXX/neon-vector-types.cpp
@@ -35,7 +35,7 @@
 extern float32x4_t vec;
 return __extension__ ({ 
 float32x4_t __a = (vec); 
-(float32_t)__builtin_neon_vgetq_lane_f32(__a, I);  // expected-error{{argument should be a value from 0 to 3}}
+(float32_t)__builtin_neon_vgetq_lane_f32(__a, I);  // expected-error-re{{argument value {{.*}} is outside the valid range}}
   });
   }
 
Index: clang/test/Sema/builtins-x86.cpp
===
--- /dev/null
+++ clang/test/Sema/builtins-x86.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple=x86_64-apple-darwin -fsyntax-only -verify %s
+//
+// Ensure that when we use builtins in C++ code with templates that compute the
+// valid immediate, the dead code with the invalid immediate doesn't error.
+
+typedef short __v8hi __attribute__((__vector_size__(16)));
+
+template 
+__v8hi test(__v8hi a) {
+if (Imm < 4)
+  return __builtin_ia32_pshuflw(a, 0x55 * Imm);
+else
+  return __builtin_ia32_pshuflw(a, 0x55 * (Imm - 4));
+}
+
+template __v8hi test<0>(__v8hi);
+template __v8hi test<1>(__v8hi);
+template __v8hi test<2>(__v8hi);
+template __v8hi test<3>(__v8hi);
+template __v8hi test<4>(__v8hi);
+template __v8hi test<5>(__v8hi);
+template __v8hi test<6>(__v8hi);
+template __v8hi test<7>(__v8hi);
\ No newline at end of file
Index: clang/test/Sema/builtins-x86.c
===
--- clang/test/Sema/builtins-x86.c
+++ clang/test/Sema/builtins-x86.c
@@ -22,145 +22,145 @@
 }
 
 __m128 test__builtin_ia32_cmpps(__m128 __a, __m128 __b) {
-  __builtin_ia32_cmpps(__a, __b, 32); // expected-error {{argument should be a value from 0 to 31}}
+  return __builtin_ia32_cmpps(__a, __b, 32); // expected-error {{argument value 32 is outside the valid range [0, 31]}}
 }
 
 __m128d test__builtin_ia32_cmppd(__m128d __a, __m128d __b) {
-  __builtin_ia32_cmppd(__a, __b, 32); // expected-error {{argument should be a value from 0 to 31}}
+  return __builtin_ia32_cmppd(__a, __b, 32); // expected-error {{argument value 32 is outside the valid range [0, 31]}}
 }
 
 __m128 test__builtin_ia32_cmpss(__m128 __a, __m128 __b) {
-  __builtin_ia32_cmpss(__a, __b, 32); // expected-error {{argument should be a value from 0 to 31}}
+  return __builtin_ia32_cmpss(__a, __b, 32); // expected-error {{argument value 32 is outside the valid range [0, 31]}}
 }
 
 __m128d test__built

[PATCH] D48462: [X86] Update handling in CGBuiltin to be tolerant of out of range immediates.

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM, thanks!


https://reviews.llvm.org/D48462



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

[PATCH] D48464: [x86] Teach the builtin argument range check to allow invalid ranges in dead code.

[Chandler Carruth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC335309: [x86] Teach the builtin argument range check to 
allow invalid ranges in (authored by chandlerc, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48464?vs=152404&id=152411#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48464

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaChecking.cpp
  test/CodeGen/builtins-mips-args.c
  test/CodeGen/builtins-systemz-vector-error.c
  test/CodeGen/builtins-systemz-vector2-error.c
  test/CodeGen/builtins-systemz-zvector-error.c
  test/CodeGen/builtins-systemz-zvector2-error.c
  test/CodeGen/hexagon-check-builtins.c
  test/Sema/aarch64-neon-fp16-ranges.c
  test/Sema/aarch64-neon-ranges.c
  test/Sema/arm-neon-types.c
  test/Sema/builtin-object-size.c
  test/Sema/builtin-prefetch.c
  test/Sema/builtins-arm.c
  test/Sema/builtins-arm64.c
  test/Sema/builtins-ppc.c
  test/Sema/builtins-x86.c
  test/Sema/builtins-x86.cpp
  test/SemaCXX/neon-vector-types.cpp

Index: test/Sema/aarch64-neon-ranges.c
===
--- test/Sema/aarch64-neon-ranges.c
+++ test/Sema/aarch64-neon-ranges.c
@@ -11,12 +11,12 @@
   vextq_u8(big, big, 15);
   vextq_p8(big, big, 15);
 
-  vext_s8(small, small, 8); // expected-error {{argument should be a value from 0 to 7}}
-  vext_u8(small, small, 8); // expected-error {{argument should be a value from 0 to 7}}
-  vext_p8(small, small, 8); // expected-error {{argument should be a value from 0 to 7}}
-  vextq_s8(big, big, 16); // expected-error {{argument should be a value from 0 to 15}}
-  vextq_u8(big, big, 16); // expected-error {{argument should be a value from 0 to 15}}
-  vextq_p8(big, big, 16); // expected-error {{argument should be a value from 0 to 15}}
+  vext_s8(small, small, 8); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vext_u8(small, small, 8); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vext_p8(small, small, 8); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vextq_s8(big, big, 16); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vextq_u8(big, big, 16); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vextq_p8(big, big, 16); // expected-error-re {{argument value {{.*}} is outside the valid range}}
 }
 
 void test_mul_lane_f64(float64x1_t small, float64x2_t big, float64x2_t rhs) {
@@ -29,11 +29,11 @@
   vfmaq_lane_f64(big, big, small, 0);
   vfmaq_laneq_f64(big, big, big, 1);
 
-  vmul_lane_f64(small, small, 1); // expected-error {{argument should be a value from 0 to 0}}
-  vmul_laneq_f64(small, big, 2); // expected-error {{argument should be a value from 0 to 1}}
-  vfma_lane_f64(small, small, small, 1); // expected-error {{argument should be a value from 0 to 0}}
-  vfma_laneq_f64(small, small, big, 2); // expected-error {{argument should be a value from 0 to 1}}
-  vfmaq_laneq_f64(big, big, big, 2); // expected-error {{argument should be a value from 0 to 1}}
+  vmul_lane_f64(small, small, 1); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vmul_laneq_f64(small, big, 2); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vfma_lane_f64(small, small, small, 1); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vfma_laneq_f64(small, small, big, 2); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vfmaq_laneq_f64(big, big, big, 2); // expected-error-re {{argument value {{.*}} is outside the valid range}}
 }
 
 void test_ld1st1(int8x8_t small, int8x16_t big, void *addr) {
@@ -47,15 +47,15 @@
   vld1q_lane_s32(addr, big, 3);
   vld1q_lane_s64(addr, big, 1);
 
-  vld1_lane_s8(addr, small, 8); // expected-error {{argument should be a value from 0 to 7}}
-  vld1_lane_s16(addr, small, 4); // expected-error {{argument should be a value from 0 to 3}}
-  vld1_lane_s32(addr, small, 2); // expected-error {{argument should be a value from 0 to 1}}
-  vld1_lane_s64(addr, small, 1); // expected-error {{argument should be a value from 0 to 0}}
-
-  vld1q_lane_s8(addr, big, 16); // expected-error {{argument should be a value from 0 to 15}}
-  vld1q_lane_s16(addr, big, 8); // expected-error {{argument should be a value from 0 to 7}}
-  vld1q_lane_s32(addr, big, 4); // expected-error {{argument should be a value from 0 to 3}}
-  vld1q_lane_s64(addr, big, 2); // expected-error {{argument should be a value from 0 to 1}}
+  vld1_lane_s8(addr, small, 8); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vld1_lane_s16(addr, small, 4); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vld1_lane_s32(addr, small, 2); // expected-error-re {{argument value {{.*}} is outside the valid range}}
+  vld1_lane_s64(addr, small, 1); // expecte

[PATCH] D56690: [Nios2] Remove Nios2 backend

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM as a patch, but same as the other wait to land until the -dev thread 
settles.


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

https://reviews.llvm.org/D56690



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

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a subscriber: hans.
chandlerc added a comment.

Adding Hans so he can be aware of the progress.


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

https://reviews.llvm.org/D56571



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

[PATCH] D56353: Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer' with'-mframe-pointer='

[Chandler Carruth via Phabricator via cfe-commits]

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

Wow, thanks for the cleanups. This is much easier to read as a consequence. And 
sorry it took a while to get you another round of review. Comments below.




Comment at: include/clang/Driver/CC1Options.td:270
+def mframe_pointer_EQ : Joined<["-"], "mframe-pointer=">,
+  HelpText<"Specify effect of frame pointer elimination optimization 
(all,non-leaf,none)">, Values<"all,non-leaf,none">;
 def mdisable_tail_calls : Flag<["-"], "mdisable-tail-calls">,

Should say more `Specify which frame pointers to retain (all, non-leaf, none)`.



Comment at: lib/Driver/ToolChains/Clang.cpp:574
 
-static bool shouldUseFramePointer(const ArgList &Args,
-  const llvm::Triple &Triple) {
-  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
-   options::OPT_fomit_frame_pointer))
-return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
-   mustUseNonLeafFramePointerForTarget(Triple);
-
-  if (Args.hasArg(options::OPT_pg))
-return true;
-
-  return useFramePointerForTargetByDefault(Args, Triple);
-}
+static StringRef DetermineFramePointer(const ArgList &Args,
+   const llvm::Triple &Triple) {

Keep LLVM coding convention naming pattern please.



Comment at: lib/Driver/ToolChains/Clang.cpp:576-579
+  Arg *FP = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+options::OPT_fomit_frame_pointer);
+  Arg *LeafFP = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
+options::OPT_momit_leaf_frame_pointer);

This still doesn't make sense to me...

If the user specifies `-fomit-frame-point` or `-fno-omit-frame-pointer` *after* 
`-momit-leaf-frame-pointer` or `-mno-omit-leaf-frame-pointer`, then that last 
flag should win...

I think you need to first get the "base" FP state by checking the main two 
flags. Then you need to get the "last" FP state by checking *all four flags*. 
When the last flag is a leaf flag, then the state is determined by the base + 
the last. When the last flag is not one of the leaf flags, then the last flag 
fully specifies the result.

I think you can also process these variables into something simpler to test 
below, essentially handling all the matching logic in one place.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56353



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

[PATCH] D56932: [Driver] [NetBSD] Pass default library search paths to linker

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

There was a long discussion between two NetBSD maintainers about this (both 
already in the reviewers list of this patch). I'm not sure if there is an 
existing thread that would be better to follow up on as opposed to starting a 
fresh thread.

I think the question was: should the path search logic be handled in the 
`clang` driver or in LLD itself. FWIW, I prefer that NetBSD follow the same 
design as every other platform here with the (somewhat C and C++ specific) 
search logic provided by the `clang` driver.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56932



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

[PATCH] D54547: PTH-- Remove feature entirely-

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Should likely add release notes about this.

Also might be worth sending a note to cfe-dev as a heads up and give folks some 
time to say "wait wait".


Repository:
  rC Clang

https://reviews.llvm.org/D54547



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Chandler Carruth via Phabricator via cfe-commits]

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

I think this should be `internal-driver-option` and the text updated? I don't 
think these are necessarily experimental, they're internals of the compiler 
implementation, and not a supported interface for users. Unsure how to best 
explain this.


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

https://reviews.llvm.org/D55150



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

[PATCH] D58374: [Clang][NewPM] Don't bail out if the target machine is empty

[Chandler Carruth via Phabricator via cfe-commits]

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

Maybe update at least some of the tests using these targets to additionally run 
with the new pass manager explicitly enabled via flag?




Comment at: clang/lib/CodeGen/BackendUtil.cpp:950
 
-  // The new pass manager always makes a target machine available to passes
-  // during construction.
-  CreateTargetMachine(/*MustCreateTM*/ true);
-  if (!TM)
-// This will already be diagnosed, just bail.
+  bool UsesCodeGen = (Action != Backend_EmitNothing &&
+  Action != Backend_EmitBC &&

I would say `RequiresCodeGen` instead of `UsesCodeGen` as we will still *use* 
the target manager even if it isn't required when emitting a `.ll` file.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58374



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

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: llvm/test/tools/gold/X86/opt-level.ll:53
+  ; CHECK-O1-OLDPM: select
+  ; The new PM does not do as many optimizations at O1
+  ; CHECK-O1-NEWPM: phi

tejohnson wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > This is intended? I'm surprised the two PMs don't have the same list of 
> > > passes for a given opt level?
> > I'm really not sure. I did compare the post-link LTO pipelines of both PMs 
> > at O0/O1/O2 and confirmed that the old PM is doing many more passes than 
> > the new PM at O1. Probably a question for @chandlerc ? In any case, I 
> > didn't want to address it here since it is orthogonal.
> Some more info:
> 
> This is the regular LTO post-link pipeline, ThinLTO post-link pipeline uses 
> essentially the same as a normal opt pipeline so would be consistent at -O1.
> 
> The optimization missing from the new PM regular LTO post link pipeline that 
> affects this test is SimplifyCFG. This and a few other optimizations are 
> added in the old PM at O1 via 
> PassManagerBuilder::addLateLTOOptimizationPasses. Note that 
> PassManagerBuilder::addLTOOptimizationPasses does exit early at -O1 (similar 
> to where we exit early in the new PM for the regular LTO post link 
> compilation). But the "late" LTO optimization passes are added 
> unconditionally above -O0. Is this correct/desired? There isn't an equivalent 
> in the new PM.
I don't think it was intentional, but I'm not sure I would directly replicate 
it if it requires adding an entirely new customization point. Is their some 
simpler way of getting equivalent results at O1?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61022



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

[PATCH] D61138: [PGO] Enable InstrProf lowering for Clang PGO instrumentation in the new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

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

Awesome, LGTM

You might also update one of the instr prof tests to have two `RUN` lines, one 
for each pass manager. Feel free to do that (or not) and submit.


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

https://reviews.llvm.org/D61138



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

[PATCH] D61142: Set LoopInterleaved in the PassManagerBuilder.

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM, maybe add a comment here to archive some of the reasoning? (IE, that when 
unrolling is disabled we disable both literal unrolling but also interleaving 
for historical reasons)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61142



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

[PATCH] D58374: [Clang][NewPM] Don't bail out if the target machine is empty

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58374



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

[PATCH] D61620: [NewPassManager] Add tuning option: LoopUnrolling [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Same comment as on other Clang patch -- let's update an IR generation test to 
reflect this?

Should just be adding a RUN line.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61620



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

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

Can you update some Clang IR generation test that uses these flags to run w/ 
the new PM? It should fail without this and pass with this.

LGTM w/ that test updated.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

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

The file name of the test seems odd? How about `vectorize-loops.c`? I'd also 
make it a C test and put it in `test/CodeGen` instead of a C++ test.




Comment at: test/CodeGenCXX/no-pragma-loop.cpp:7
+// CHECK-ENABLE-VECT: for.body:
+// CHECK-ENABLE-VECT: vector.body:
+

I'd check for an instruction on a vector type as the block labels aren't really 
that stable.

Should be able to just check for something like:

```
// CHECK-LABEL: define @vectorize_loop_test {
// CHECK: mul <{{[0-9]+}} x double>
```

Or something...

Also, you'll want to explicitly pass a target triple here and require that 
target to be registered so we're not flaky when the host target changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

(Sorry for still more comments)




Comment at: test/CodeGen/loop-vectorize.c:10
+// CHECK-DISABLE-VECT-LABEL: @for_test()
+// CHECK-DISABLE-VECT: fmul double
+

You'll want to check that the vector form *doesn't* show up. I would expect 
this to pass even w/ vectorization enabled due to potential fallback loops.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D57640: [NewPM][MSan] Add Options Handling

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Minor past-commit nit.




Comment at: lib/CodeGen/BackendUtil.cpp:1039
 [](FunctionPassManager &FPM, PassBuilder::OptimizationLevel Level) 
{
-  FPM.addPass(MemorySanitizerPass());
+  FPM.addPass(MemorySanitizerPass({}));
 });

This change isn't needed any more (since you added the default back).


Repository:
  rC Clang

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

https://reviews.llvm.org/D57640



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

[PATCH] D28248: Work around GCC PR37804

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Hey Marshall and Michael,

As mentioned in my email to all the lists[1], patches posted to Phabricator 
before the new license was installed should be confirmed as under the new 
license before being rebased and applied. Not sure that happened here as the 
file headers are still the old file headers.

I'll update the file headers anyways, and I think this is fine as Michael is 
contributing with an @apple.com email address and so all of this is covered by 
their agreement anyways. But I wanted to mention it in case there are other 
in-flight patches on Phabricator where this is relevant.

-Chandler

[1]: http://lists.llvm.org/pipermail/llvm-dev/2018-December/128695.html 
(llvm-dev), http://lists.llvm.org/pipermail/libcxx-dev/2019-January/000140.html 
(libcxx-dev)


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

https://reviews.llvm.org/D28248



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

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

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

Based on the -dev discussion, update once the target machine differences are 
addressed by mimicing the way the legacy PM works, which will hopefully 
restrict this similarly to what Eli is suggesting as well...


Repository:
  rC Clang

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

https://reviews.llvm.org/D58375



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

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D58375#1403144 , @efriedma wrote:

> I can understand why tests that use -O1 or -O2 would produce different 
> results with the new pass manager, but it looks like not all the tests are 
> like that.  Do you know why those tests are failing?
>
> For the tests that do use -O, instead of marking them unsupported, could you 
> use -fno-experimental-new-pass-manager or something like that?


For at least some of them, maybe we should run it in both modes (using the 
explicit flag as suggested by Eli) and check both forms.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58375



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

[PATCH] D58424: [NewPM] Add other sanitizers at O0

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58424



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

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

But, the verifier itself will just "crash". It won't print a stack trace, but I 
don't see why that's much better? And this flag is supposed to be a developer 
option and not a user facing one, so I'm somewhat confused at what the intent 
is here...


https://reviews.llvm.org/D38042



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

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D38042#875328, @aprantl wrote:

> In https://reviews.llvm.org/D38042#875303, @chandlerc wrote:
>
> > But, the verifier itself will just "crash". It won't print a stack trace, 
> > but I don't see why that's much better? And this flag is supposed to be a 
> > developer option and not a user facing one, so I'm somewhat confused at 
> > what the intent is here...
>
>
> No, it will report_fatal_error() instead of crashing the compiler later on.
>  In any case, that is not my primary motivation: The intent is exactly what 
> is illustrated by the testcase, stripping malformed debug info metadata 
> produced by older, buggy versions of clang. The backstory to this is that 
> historically the Verifier was very weak when it came to verifying debug info. 
> To allow us to make the Verifier better (stricter), while still supporting 
> importing of older .bc files produced by older compilers, we added a 
> mechanism that strips all debug info metadata if the verification of the 
> debug info failed, but the bitcode is otherwise correct.


Ok, that use case makes way more sense. I'd replace the change description with 
some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad 
that the verifier *changes the IR*. Don't get me wrong, what you're trying to 
do (strip malformed debug info) makes perfect sense. But I think that the thing 
which does that shouldn't be called a verifier. It might *use* the verifier of 
course.

Last but not least, I still suspect that this shouldn't be run here. If the 
user wants to disable LLVM passes *and emits LLVM IR*, they should get it 
unperturbed. The stripping of malformed debug info seems like it should happen 
later as part of the passes to emit code, and I'd actually expect the LLVM code 
generator to add the necessary pass rather than relying on every frontend 
remembering to do so?


https://reviews.llvm.org/D38042



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

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D38042#875412, @aprantl wrote:

> In https://reviews.llvm.org/D38042#875334, @chandlerc wrote:
>
> > In https://reviews.llvm.org/D38042#875328, @aprantl wrote:
> >
> > > In https://reviews.llvm.org/D38042#875303, @chandlerc wrote:
> > >
> > > > But, the verifier itself will just "crash". It won't print a stack 
> > > > trace, but I don't see why that's much better? And this flag is 
> > > > supposed to be a developer option and not a user facing one, so I'm 
> > > > somewhat confused at what the intent is here...
> > >
> > >
> > > No, it will report_fatal_error() instead of crashing the compiler later 
> > > on.
> > >  In any case, that is not my primary motivation: The intent is exactly 
> > > what is illustrated by the testcase, stripping malformed debug info 
> > > metadata produced by older, buggy versions of clang. The backstory to 
> > > this is that historically the Verifier was very weak when it came to 
> > > verifying debug info. To allow us to make the Verifier better (stricter), 
> > > while still supporting importing of older .bc files produced by older 
> > > compilers, we added a mechanism that strips all debug info metadata if 
> > > the verification of the debug info failed, but the bitcode is otherwise 
> > > correct.
> >
> >
> > Ok, that use case makes way more sense. I'd replace the change description 
> > with some discussion of this use case.
> >
> > My next question is -- why is this done by the verifier? It seems *really* 
> > bad that the verifier *changes the IR*. Don't get me wrong, what you're 
> > trying to do (strip malformed debug info) makes perfect sense. But I think 
> > that the thing which does that shouldn't be called a verifier. It might 
> > *use* the verifier of course.
>
>
> That was a purely pragmatic decision: Most (but not all) LLVM-based tools are 
> running the Verifier as an LLVM pass so adding the stripping into the pass 
> was the least invasive way of implementing this feature. If we are truly 
> bothered by this, I think what could work is to separate out a second 
> StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to 
> run immediately after it. I don't see this adding much value though, and we 
> would have to modify all tools to schedule the new pass explicitly. Do you 
> think that would be worth pursuing?


Absolutely. I think the verifier should never, under any circumstances, mutate 
the IR. Think about it, with the current design if a pass corrupts the debug 
info the verifier may "hide" this by stripping it out rather than allowing us 
to find it.

> 
> 
>> Last but not least, I still suspect that this shouldn't be run here. If the 
>> user wants to disable LLVM passes *and emits LLVM IR*, they should get it 
>> unperturbed. The stripping of malformed debug info seems like it should 
>> happen later as part of the passes to emit code, and I'd actually expect the 
>> LLVM code generator to add the necessary pass rather than relying on every 
>> frontend remembering to do so?
> 
> The user wants to disable LLVM optimizations (`-disable-llvm-optzns`) not 
> LLVM passes.

(sorry for the off-list duplication, but it belongs here anyways)

I disagree. `-disable-llvm-optzns` is a developer flag, and was almost an alias 
for `-disable-llvm-passes`. After discussion on the list we made it an actual 
legacy and deprecated alias for `-disable-llvm-passes` because the latter was 
more clear, understandable, and correct. We had cases where the passes run by 
`-disable-llvm-optzns` were actually problematic and we wanted to debug them 
but were unable to do so.

> Also, I believe the Verifier is special. The Verifier protects LLVM from 
> crashing and as a user I think I would prefer a Verifier error over clang 
> crashing while emitting bitcode.

I think this distinction is not a meaningful one ultimately. And the verifier 
should *never* be a functional requirement because it should have no effect. 
I'm happy for us to verify when we read input, but even then it should not 
mutate the IR.

> Because of auto-upgrades users already don't get the IR unperturbed, and I 
> view stripping of broken debug info as a (in all fairness: very brutal :-) 
> auto-upgrade.

But auto-upgrade happens on *read*. If you want to add the debug info stripping 
to auto-upgrade, that's a reasonable discussion to have, and no change here 
will be required. There might be concerns about that on the LLVM side, I don't 
know. But the verifier (as well as running it here) does not seem like the 
right solution.


https://reviews.llvm.org/D38042



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

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D38042#875441, @aprantl wrote:

> In https://reviews.llvm.org/D38042#875418, @chandlerc wrote:
>
> > Absolutely. I think the verifier should never, under any circumstances, 
> > mutate the IR. Think about it, with the current design if a pass corrupts 
> > the debug info the verifier may "hide" this by stripping it out rather than 
> > allowing us to find it.
>
>
> Okay, I think I agree. Before I venture off implementing this, do you think 
> that separating out a StripBrokenDebugInfoPass that depends on the Verifier 
> is right way forward?


I don't know what you mean by "depends on" and I'm not sure I'm the right 
person to say what the exact best design is... But I'd throw something together 
and start that review on llvm-commits where we can get folks more familiar w/ 
these details involved to figure it out. It at least seems to be in the right 
direction.

My only concern is that passes are supposed to be able to rely on the verifier 
passing, and this wouldn't... Not sure how to handle that case.

> 
> 
>> But auto-upgrade happens on *read*. If you want to add the debug info 
>> stripping to auto-upgrade, that's a reasonable discussion to have, and no 
>> change here will be required. There might be concerns about that on the LLVM 
>> side, I don't know. But the verifier (as well as running it here) does not 
>> seem like the right solution.
> 
> Would splitting the VerifierPass into a VerifierPass and a 
> StripBrokenDebugInfoPass *together* with adding a -enable-llvm-verifier (an 
> explicit opposite of -disable-llvm-verifier) work for you?

If you want a way to use the `clang` binary to strip broken debug info from a 
bitcode input, I would add a flag that does exactly this, and leave the 
verifier as an independent component. I don't know whether such a flag makes 
sense or not (Richard or other more deep in Clang-land would have a better feel 
than I would).

But I think that whether the verifier is enabled or not should be orthogonal 
from any debug info stripping. Stripping the debug info might impact whether 
something verifies, but the flags should be completely independent.

However, if the debug info stripping ends up as part of auto upgrade, all of 
this becomes much simpler to think about.


https://reviews.llvm.org/D38042



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

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[Chandler Carruth via Phabricator via cfe-commits]

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

Marking as needing changes to clear dashboard.


https://reviews.llvm.org/D38042



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

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

I would just change this to have the module pass loop over the functions -- 
that seems like it'll be much cleaner.

As it is, I'm not seeing where the loop actually happens. But rather than 
trying to figure that out, just seems better to turn it into an explicit loop. 
That way you can get rid of all the check analysis overhead I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D62225: [clang][NewPM] Fixing -O0 tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

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

I think this ultimately needs to be split up into smaller patches. A bunch of 
these things can be landed independently. Here is my first cut at things to 
split out, each one into its own patch.

1. the LLVM change to the always inliner
2. the Clang change to how we build the always inliner
3. the PGO pipeline changes (which I have to admit I still don't fully 
understand)
4. The additions of `-fno-experimental-new-pass-manager` for test cases that 
are explicitly testing legacy PM behavior
5. switching tests to be resilient to changes in attribute group numbering (and 
adding a RUN line w/ the new PM to ensure we don't regress)

for #5 (or others) where *some* testing needs to be working before they can 
land, just sequence them after whatever they depend on

Other things I think can also be split out, but I suspect into *different* 
changes from what you have here:

6. Instead of passing `-fno-experimental-new-pass-manager` for tests that use 
`-O` but don't specify a number, Clang should pick a consistent value for the 
level I think

I'd be interested to then see what is left here.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1104-1105
   // which is just that always inlining occurs.
-  MPM.addPass(AlwaysInlinerPass());
+  // We always pass false here since according to the legacy PM logic for
+  // enabling lifetime intrinsics, we should not be compiling with O0.
+  MPM.addPass(AlwaysInlinerPass(/*InsertLifetimeIntrinsics=*/false));

leonardchan wrote:
> serge-sans-paille wrote:
> > echristo wrote:
> > > Can you elaborate more here? We do turn on the always inliner at O0 which 
> > > makes this comment a bit confusing.
> > I guess he means 
> > 
> > We always pass false here since according to the legacy PM logic for 
> > enabling lifetime intrinsics, they are not required with O0
> > 
> Yup, my bad. This is what I meant with this comment. Always inlining is used. 
> It's the lifetime intrinsics that aren't always used.
I don't think explaining it in terms of one pass manager or another is the righ 
thing to do.

Instead, I'd say what the desired result is:

```
Build a minimal pipeline based on the semantics required by Clang,
which is just that always inlining occurs. Further, disable generating
lifetime intrinsics to avoid enabling further optimizations during
code generation.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62225



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

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

This was a lot easier for me to understand too, thanks. Somewhat minor code 
changes below.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232
+  MPM.addPass(ModuleSanitizerCoveragePass(SancovOpts));
+  MPM.addPass(
+  
createModuleToFunctionPassAdaptor(SanitizerCoveragePass(SancovOpts)));
+}

Is there an existing function pass pipeline we can put this in, maybe by using 
an extension point? That would make it much faster to run I suspect.



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

Need to use the new file header.



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:14
+//===--===//
+#ifndef LLVM_TRANSFORMS_INSTRUMENTATION_SANITIZERCOVERAGE_H
+#define LLVM_TRANSFORMS_INSTRUMENTATION_SANITIZERCOVERAGE_H

I feel like we usually have whitespace here too?



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:24
+
+/// This is a wrapper for SanitizerCoverage usage in the new pass manager. The
+/// pass instruments functions for coverage.

Not really a wrapper, just the pass itself...

`SanitizerCoverage` is an implementation detail that the reader here doesn't 
really know about.



Comment at: llvm/include/llvm/Transforms/Instrumentation/SanitizerCoverage.h:37
+
+/// This is a wrapper for the ModuleSanitizerCoverage. This adds initialization
+/// calls to the module for trace PC guards and 8bit counters if they are

Same comment as above.



Comment at: llvm/lib/Passes/PassRegistry.def:248
 FUNCTION_PASS("tsan", ThreadSanitizerPass())
+FUNCTION_PASS("sancov-func", SanitizerCoveragePass())
 #undef FUNCTION_PASS

Consistency would suggest just `sancov` here? I don't feel strongly though.



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:228-229
+  : Options(OverrideFromCL(Options)) {
+initializeModuleSanitizerCoverageLegacyPassPass(
+*PassRegistry::getPassRegistry());
   }

This should be in the legacy pass below? (actually, I tihnk it already is, so 
this can likely be removed)



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:262-269
+bool ShouldEmitInitCalls = false;
+for (const Function &F : M) {
+  if (canInstrumentWithSancov(F)) {
+ShouldEmitInitCalls = true;
+break;
+  }
+}

```
if (!llvm::any_of(M, [](const Function &F) { return can 
InstrumentWithSancov(F); }))
  return false;
```



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:442
+  const PostDominatorTree *PDT = &AM.getResult(F);
+  SanitizerCoverage Sancov(*F.getParent(), Options);
+  if (Sancov.instrumentFunction(F, DT, PDT))

I completely misread this the first time through thinking you used a common 
object for state

Maybe change the constructor to accept a function to make it more obvious that 
this is intended to be built per-function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

This really confused me. We shouldn't be seeing this kind of difference in the 
new PM.

But I think I figured it out.

Both PMs have to run *some* inliner at -O0. This is because we need to inline 
`always_inline` functions. The new PM has a *super* simple (and fast 
compile-time wise) inliner designed for this. It's really good at its job. The 
old PM runs the normal inliner. This thing isn't simple at all. It does complex 
stuff to incrementally simplify code while inlining because that can have a 
huge effect on the overall efficiency of the compiler *when doing full 
optimization passes*. But we're not doing that.

The result is that because all of these tests check code that is emitted by 
`always_inline` functions in our builtin headers, and then *inlined* into the 
test functions, the inliner in the old pass manager did a bunch of "cleanup" of 
the code that the new PM doesn't do.

Honestly, I think the new PM's behavior is correct for -O0, but I think it 
makes these tests less easy to understand. I think we should instead change the 
command running here, and when we do I think we will find a set of checks that 
work for both PMs.

When we are willing to do something like run `opt` in addition to clang (the 
`convergent.cl` case), instead of just doing `-instnamer` we should also do 
`-instcombine`. I suspect this will remove the differences.

When we're just directly using `%clang_cc1` I suspect that rather than checking 
the completely unoptimized code we should check with `-O1` to make the IR much 
more stable and easy to inspect. It should also result in identical results 
from the two PMs.




Comment at: clang/test/CodeGenOpenCL/convergent.cl:2
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm %s -o - 
-fno-experimental-new-pass-manager | opt -instnamer -S | FileCheck 
-enable-var-scope %s --check-prefixes=CHECK,CHECK-LEGACY
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm %s -o - 
-fexperimental-new-pass-manager | opt -instnamer -S | FileCheck 
-enable-var-scope %s --check-prefixes=CHECK,CHECK-NEWPM
 

Probably want to use `opt -passes=instnamer` or some such to use the new PM in 
the `opt` invocation as well...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

[PATCH] D63153: [clang][NewPM] Fix broken -O0 test from the AlwaysInliner

[Chandler Carruth via Phabricator via cfe-commits]

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

Code change LGTM. Can you update at least one of the tests to explicitly run 
both PMs so that we'll notice if this breaks in some weird way? Feel free to 
submit with that change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63153



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

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

Let's update the test to explicitly run w/ both PMs to make sure this keeps 
working. LGTM with that change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63155



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

I understand the change to explicitly say `-O2`. I also understand the change 
to add an explicit `-fno-experimental-new-pass-manager` to a `RUN` line when we 
have another `RUN` line that explicitly uses `-fexperiemntal-new-pass-manager`.

But for many of these cases, there is no new-PM `RUN` line in the test. If 
we're going to fix one `RUN` line to a particular pass manager, we should fix 
both IMO unless there is something quite odd about the IR being tested that 
makes the result of optimizinng be weirdly different between the two pass 
managers. That would be somewhat surprising, so I kinda want to know if we 
actually see that so often to understand what is causing this divergence.

In general, Clang tests shouldn't be checking such complex optimizations that 
the differences between the pass managers really manifests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156



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

[PATCH] D63168: [clang][NewPM] Fix split debug test

[Chandler Carruth via Phabricator via cfe-commits]

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

Code change LGTM, but again, let's update the test to check both ways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63168



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

[PATCH] D63170: [clang][NewPM] Fix broken -O0 test from missing assumptions

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM. Bit annoying that we need to do this at O0, but fine...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63170



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63156#1543937 , @leonardchan wrote:

> I did some more digging and found the following differences between PMs for 
> each test, and they seem to all differ and can be fixed for different reasons.


Awesome, and sorry this is such a big effort, but I think these are all going 
to end up being pretty interesting root causes. I guess yay, Clang's testing is 
doing its job?

> **CodeGen/aggregate-assign-call.c**: The new PM on -O1 codegen produces the 
> do/while loop differently but still emits the lifetime start/end intrinsics 
> around temporary variables, which is what the test checks for. Here we can 
> just check for the instructions while ignoring the branches generated.

Weird, but makes sense.

> **CodeGen/arm_acle.c**: A bunch of instructions seem to have been combined 
> into a call to `llvm.fshl.i32`, so we just check for those under the new PM.

Same.

> **CodeGen/available-externally-suppress.c**: `available_externally` was not 
> emitted during `-O2 -flto` runs when it should still be retained for link 
> time inlining purposes. This can be fixed by checking that we aren't 
> LTOPrelinking when adding the `EliminateAvailableExternallyPass`.

Yikes, this is a good bug to fix!

> **CodeGen/pgo-sample.c [No new fix]**: The test checks that `PruneEH` runs, 
> but there doesn’t seem to be a corresponding new PM pass for it. Should there 
> be? If there is one then we can just check for that in the debug PM dump.

The analog would be `function-attrs` and `function(simplify-cfg)` in some 
combination. Maybe this should just check that some specific thing is optimized 
away at `-O2` rather than checking the specific details of pass pipeline 
construction?

> **CodeGen/x86_64-instrument-functions.c [No new fix]**: This one's a bit 
> complicated. The initial problem seems to be that `EntryExitInstrumenterPass` 
> was not added into the pipeline to emit `__cyg_profile_func_enter/exit()` 
> calls. Adding that to the pipeline seems to instrument correctly now and add 
> these calls, but we run into a problem with inlining. `root()` expects 2 
> calls to `__cyg_profile_func_enter` and 2 calls to `__cyg_profile_func_exit` 
> in its body,
> 
>   ; Function Attrs: nounwind
>   define i32 @root(i32 returned %x) #0 {
>   entry:
> %0 = tail call i8* @llvm.returnaddress(i32 0)
> tail call void @__cyg_profile_func_enter(i8* bitcast (i32 (i32)* @root to 
> i8*), i8* %0) #2
> tail call void @__cyg_profile_func_enter(i8* bitcast (i32 (i32)* @leaf to 
> i8*), i8* %0) #2
> tail call void @__cyg_profile_func_exit(i8* bitcast (i32 (i32)* @leaf to 
> i8*), i8* %0) #2
> tail call void @__cyg_profile_func_exit(i8* bitcast (i32 (i32)* @root to 
> i8*), i8* %0) #2
> ret i32 %x
>   }
> 
> 
> but we get only 1 call
> 
>   ; Function Attrs: norecurse nounwind readnone
>   define i32 @root(i32 returned %x) local_unnamed_addr #0 {
>   entry:
> %0 = call i8* @llvm.returnaddress(i32 0)
> call void @__cyg_profile_func_enter(i8* bitcast (i32 (i32)* @root to 
> i8*), i8* %0)
> %1 = call i8* @llvm.returnaddress(i32 0)
> call void @__cyg_profile_func_exit(i8* bitcast (i32 (i32)* @root to i8*), 
> i8* %1)
> ret i32 %x
>   }
> 
> 
> I suspect this is due to the `leaf()` being inlined into `root()` even though 
> inlining should happen after it. The legacy `EntryExitInstrumenter` pass 
> seems to be added to the legacy FunctionPassManager first before all others 
> passes. Under the legacy PM, when this pass runs, `root()` should look like:
> 
>   ; Function Attrs: nounwind
>   define i32 @root(i32 %x) #1 {
>   entry:
> %x.addr = alloca i32, align 4
> store i32 %x, i32* %x.addr, align 4, !tbaa !2
> %0 = load i32, i32* %x.addr, align 4, !tbaa !2
> %call = call i32 @leaf(i32 %0)  ; this call is not yet inlined
> ret i32 %call
>   }
> 
> 
> but `root()` looks like this in the new PM pass:
> 
>   ; Function Attrs: norecurse nounwind readnone
>   define i32 @root(i32 returned %x) local_unnamed_addr #1 {
>   entry:
> ret i32 %x
>   }

Yeah, I think the entry/exit pass really wants to come *before* any other pass. 
That should be possible even in the new PM by using an extension point?

> **CodeGenCXX/auto-var-init.cpp**: Auto var initialization is performed 
> differently under new PM than legacy. With initialization for structs with 
> default values, the legacy PM will store the stream of 0xAAs, then initialize 
> the members with default values in a constructor, whereas the new PM will 
> just store the correct value for the whole struct right away without 
> initialization. In one case, it seems that ctor function was also inlined. 
> Fixed by check accounting for these checks separately.

Cool, makes sense.

> **CodeGenCXX/conditional-temporaries.cpp**: The new pm seems to be unable to 
> statically determine the return value of some functions. For now I just add 
> separate checks for the new PM IR.

In

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

OMG, I'm so sorry, I had no idea that the tests would explode like that... 
Yeah, I don't think that's useful

Maybe a better approach is to just explicitly run the code through `opt 
-passes=instsimplify` before handing it to FileCheck? That should produce 
almost identical results to the old PM's inliner?

Feel free to just try this out and report back -- if it isn't looking good, 
don't invest a bunch of time in it, we should talk to Craig and figure out what 
the right long-term strategy here is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

[PATCH] D62225: [clang][NewPM] Fixing remaining -O0 tests that are broken under new PM

[Chandler Carruth via Phabricator via cfe-commits]

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

FWIW, this LGTM. We may want to tweak the behavior around flattening, but that 
can happen as a follow-up, and this seems to get us to a better state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62225



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232
+  MPM.addPass(ModuleSanitizerCoveragePass(SancovOpts));
+  MPM.addPass(
+  
createModuleToFunctionPassAdaptor(SanitizerCoveragePass(SancovOpts)));
+}

leonardchan wrote:
> chandlerc wrote:
> > Is there an existing function pass pipeline we can put this in, maybe by 
> > using an extension point? That would make it much faster to run I suspect.
> I tried this earlier, but when I use `registerOptimizerLastEPCallback()` 
> before the default module pipeline is made, I end up getting this linker 
> error for `-O2` runs:
> 
> ```
> fuzzer.c:(.text.sancov.module_ctor_8bit_counters[sancov.module_ctor_8bit_counters]+0x11):
>  undefined reference to `__start___sancov_pcs'
> fuzzer.c:(.text.sancov.module_ctor_8bit_counters[sancov.module_ctor_8bit_counters]+0x16):
>  undefined reference to `__stop___sancov_pcs'
> ```
> 
> I don't understand how I get this because the symbols are declared in the IR 
> dump as:
> 
> ```
> @__start___sancov_pcs = external hidden global i64*
> @__stop___sancov_pcs = external hidden global i64*
> ```
> 
> for both the optimized cases if I use the adaptor or EP. I'm still linking 
> with the same compiler-rt in each case.
> 
> The only difference I think of using the adaptor after 
> `buildPerModuleDefaultPipeline()` vs using the extension point callback 
> before is that the order in which the pass will be called changes, although I 
> still don't see how it could lead to this undefined reference if I still link 
> against compiler-rt.
> 
> Additionally, the only difference between the IR between using the adaptor vs 
> the EP is a few extra globals/declarations seem to be missing when using the 
> EP, which makes me think another pass that ran after this one removed them, 
> although these don't seem to be related to the error.
> 
Hmm...

Maybe those missing globals / declarations are relevant. Maybe they are 
necessary to get the linking against the sancov runtime to work correctly?

You could try adding these globals / declarations to the 'used' set so they 
don't get removed by later passes:
https://llvm.org/docs/LangRef.html#the-llvm-compiler-used-global-variable

Otherwise, I have no idea, maybe makes sense to loop in the sanitizer folks 
explicitly for help debugging this...



Comment at: llvm/lib/Passes/PassRegistry.def:248
 FUNCTION_PASS("tsan", ThreadSanitizerPass())
+FUNCTION_PASS("sancov-func", SanitizerCoveragePass())
 #undef FUNCTION_PASS

leonardchan wrote:
> chandlerc wrote:
> > Consistency would suggest just `sancov` here? I don't feel strongly though.
> Ah, so the reason why I don't use `sancov` here is because in the tests when 
> using `-passes='function(sancov)', I get the following error:
> 
> ```
> /usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-b133876586/bin/opt:
>  unknown function pass 
> '/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-3-b133876586/bin/sancov'
> ```
> 
> I think this is because lit tries to substitute `sancov` with the one under 
> `bin/sancov` in my build dir. I'd be up for changing it if there's a way 
> around this.
That's... really entertaining.

I suspect the tool should really be `llvm-sancov` to avoid this kind of thing.

But anyways, i'm OK with this or calling it something else. There is probably a 
way to hide a thing from `lit`'s substitution as well, I just don't know what 
it is


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D63577: [clang][NewPM] Move EntryExitInstrumenterPass to the start of the pipeline

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM, nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63577



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

[PATCH] D63580: [clang][NewPM] Do not eliminate available_externally durng `-O2 -flto` runs

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM, again, really nice. Tiny tweak below.




Comment at: llvm/test/Other/available-externally-lto.ll:1
+; Ensure that we don't emit available_externally functions at -O2, unless 
-flto is present in which case we should preserve them for link-time inlining 
decisions.
+; RUN: opt < %s -S -passes='default' | FileCheck %s

Wrap to 80-columns?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63580



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

just a minor comment on one of these...




Comment at: clang/test/CodeGen/pgo-sample.c:10
+
+// The new pass manager analog to PrunEH is a combination of 'function-attrs'
+// and 'function(simplify-cgf)'.

s/PrunEH/PruneEH/



Comment at: clang/test/CodeGen/pgo-sample.c:12-14
+// NEWPM-DAG: PostOrderFunctionAttrsPass
+// NEWPM-DAG: SimplifyCFGPass
+// NEWPM-DAG: SampleProfileLoaderPass

The DAG worries me a bit ... The point here is to check that we remove EH 
before attaching sample profile data, and with the DAG it isn't clear that this 
happens *before*.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM, thanks so much for sticking through this, I know it was ... nontrivial!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

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

Sorry for the delay here.

It'd be nice to land the LLVM patch first and with its own testing -- we should 
have testing for the pass builder independent of Clang (IE, in the LLVM tests).

One option would be to test it with a unittest, not sure we have pass builder 
unittests at the moment.

Probably a better option would be to define a pseudo pass name like our 
`default`, maybe `pgo`. Then you can parse the `O0` the same way we do 
for the `default` thing. when it is O0 you can call the new routine I've 
suggested below. When it is higher, you can call the existing routine much like 
the default pipeline code does. This would all be implemented inside the pass 
builder so no issues w/ using the utility code there.

This would let us much more nicely write tests for the pipeline fragment 
generated for PGO both at O0 and above -- we have tests that specifically print 
out the pipeline sequence. This makes it very easy to visualize changes to the 
pipeline. And it would let you test the O0 code path here.

The clang part of the patch (once adapted to the narrower API) seems likely to 
be good then.

I'm happy for this to be two patches (first llvm, then Clang), or one patch. 
Whatever is easier for you.




Comment at: llvm/lib/Passes/PassBuilder.cpp:576
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }

xur wrote:
> I moved this pass under the condition when Early inline is enabled. I'm under 
> the impression that the intention is to clean up the code for the inline.
> Chandler: you added this pass. If you don't like this move, I can pull it out 
> and put it under another (Level > 0) condition.
This makes sense to me. It was probably just transcription error on my part.



Comment at: llvm/lib/Passes/PassBuilder.cpp:579-602
   if (RunProfileGen) {
 MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+if (Level > 0) {
+  FunctionPassManager FPM;
+  FPM.addPass(
+  createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));

Rather than moving the entire routine that is really only intended for the 
innards of the pass manager to be public, I'd do something a bit more narrow...

Maybe just add pipeline method to the pass builder to generate a minimal 
pipeline suitable for O0 usage?

I think this code will be simpler to read w/o having to have the O0 conditions 
plumbed all the way through it, and the duplication is tiny.

If you want, could pull out the use bits which are common and use early exit to 
make the code more clear:

```
if (!RunProfileGen) {
  addPGOUsePasses(...);
  return;
}

...
```

(Not sure what name to use for this, but you see the idea.) Then there would be 
almost no duplication between the O0 and this code, w/o having to have the 
conditions threaded throughout.


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

https://reviews.llvm.org/D64029



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

[PATCH] D63638: [clang][NewPM] Add new pass manager RUN lines to avx512f-builtins.c

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Just to make sure we're on the same page (and sorry I didn't jump in sooner)...

With the old PM, *anything* that is `always_inline` *gets* `instsimplify` run 
on it, even at -O0, even if you didn't want that. So using `-instsimplify` 
explicitly is, IMO, not any more scary of a reliance on LLVM's behavior than 
the old PM already subjected us to...

That said, if the x86 maintainers are comfortable with *only* using the new PM 
(because it has an always inliner that literally does nothing else and thus has 
an absolute minimum amount of LLVM transformations applied), I certainly don't 
have any objections. =D


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

https://reviews.llvm.org/D63638



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

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM with two nits addressed, thanks!




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1125
+PB.addPGOInstrPassesForO0(MPM, CodeGenOpts.DebugPassManager,
+  /* RunProfileGen */ PGOOpt->Action ==
+  PGOOptions::IRInstr,

Minor nit, here and else where with the comment-named bools I'd use the style: 
`/*RunProfileGen=*/PGOOpt->Action == PGOOPtions::IRInstr`

This was suggested as more consistent w/ Clang style, and it seems a bit 
cleaner. Also, clang-tidy will recognize and check that the comment uses the 
same name as the parameter.



Comment at: llvm/lib/Passes/PassBuilder.cpp:1839
+addPGOInstrPassesForO0(MPM, DebugLogging,
+   /* RunProfileGen */
+   PGOOpt->Action == PGOOptions::IRInstr,

Same nit about the parameter comments here.


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

https://reviews.llvm.org/D64029



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

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

One high level point that is at least worth clarifying, and maybe others will 
want to suggest a different approach:

The overall approach here is to have as small of a difference between the O1 
 and O2 
 pipelines as possible.

An alternative approach that we could take would be to design a focused O1 
 pipeline without regard to how 
much it diverges from the O2  
pipeline.

Which approach is used somewhat depends on the goals. I feel like the goal here 
is to get as close to the level of optimization at O2 
 as possible without losing compile 
time or coherent backtraces for test / assertion failures. For that goal, the 
approach taken makes sense. But it seems important to clarify that goal as 
otherwise I think we'd want to go in very different directions.

In D65410#1613555 , @hfinkel wrote:

> Thanks for starting on this. Can you go ahead and replace the sroa calls with 
> mem2reg calls for `O1` and then see what that does to the performance? That 
> strikes me as a major change, but certainly one that potentially makes sense, 
> so I'd rather we go ahead and test it now before we make decisions about 
> other adjustments.


I really think we need mem2reg at least at -O1... In fact, I really think we 
need SROA at O1 . If it is actually 
a compile time problem, I'd like to fix that in SROA. I don't really expect it 
to be though.

> FWIW, I thought that we might run InstCombine less often (or maybe replace it 
> with InstSimplify, in some places). Did you try that?

I think the biggest thing to do would be to avoid repeated runs of instcombine 
over the same code. I suspect we want at least one run after inliner and inside 
the CGSCC walk for canonicalization. But it'd be great to limit it to exactly 
one or mybe one before and one after the loop pipeline.




Comment at: llvm/lib/Passes/PassBuilder.cpp:407-414
   }
 
   // Speculative execution if the target has divergent branches; otherwise nop.
-  FPM.addPass(SpeculativeExecutionPass());
+  if (Level > O1)
+FPM.addPass(SpeculativeExecutionPass());
 
   // Optimize based on known information about branches, and cleanup afterward.

I think you can merge all of these?



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:362
 
+  // TODO: Investigate the cost/benefit of tail call elimination on debugging.
   MPM.add(createTailCallEliminationPass()); // Eliminate tail calls

hfinkel wrote:
> By definition, this loses information from the call stack, no?
Yeah, I'd have really expected this to be skipped.



Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:432
 
+  // TODO: Investigate if this is too expensive at O1.
   MPM.add(createAggressiveDCEPass()); // Delete dead instructions

hfinkel wrote:
> Yes, I'd fall back to using regular DCE.
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65410



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

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added reviewers: davidxl, tejohnson.
chandlerc added a comment.

See inline comment, but I think we should just drop the testing of the function 
attribute bit here rather than adjusting the pipeline.




Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668
+// We must also remove exception handling before attaching sample profile
+// data.
+MPM.addPass(
+createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass()));

Does the this need to go before the EarlyFPM as well as before the sample 
profile loader?

Also, this is ... a pretty expensive thing to do... Do we really need this? 
We've been using ThinLTO and sample PGO with the new PM for a long time and 
haven't seen any real issue. I think we can probably just leave this difference 
between the two pass managers and remove the test for this pass running rather 
than adding it.

CC-ing some others just to double check, but I'm not aware of any issues we've 
seen with PGO that were because of this discrepancy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Chandler Carruth via Phabricator via cfe-commits]

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

Eh, this seems close enough now. I'd like a better approach for the x86 
builtins, but no idea what it will end up being.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

FWIW, I think we can wait for a bug to be filed or report come in with some 
functional test case before we change any behavior here.

I'm just suggesting how to make the test better below.




Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668
+// We must also remove exception handling before attaching sample profile
+// data.
+MPM.addPass(
+createModuleToPostOrderCGSCCPassAdaptor(PostOrderFunctionAttrsPass()));

leonardchan wrote:
> tejohnson wrote:
> > chandlerc wrote:
> > > Does the this need to go before the EarlyFPM as well as before the sample 
> > > profile loader?
> > > 
> > > Also, this is ... a pretty expensive thing to do... Do we really need 
> > > this? We've been using ThinLTO and sample PGO with the new PM for a long 
> > > time and haven't seen any real issue. I think we can probably just leave 
> > > this difference between the two pass managers and remove the test for 
> > > this pass running rather than adding it.
> > > 
> > > CC-ing some others just to double check, but I'm not aware of any issues 
> > > we've seen with PGO that were because of this discrepancy.
> > We don't tend to use exception handling so might not be hitting the issue 
> > described in the headline.
> > 
> > If this is fixing an issue with SamplePGO and EH cleanup interactions, can 
> > you add a test that tests for that (i.e. is there a code optimization issue 
> > currently?).
> I'm not entirely familiar with the expected codegen for SamplePGO or PruneEH 
> and don't really know if not adding these 2 passes breaks anything.
> 
> Under the legacy PM, exception handling was removed via `PruneEH` before 
> creating the sample profile loader, so this change was meant to match the 
> behavior since `function-attrs` and `function(simplify-cfg)` (the new PM 
> equivalent for `-prune-eh`) did not initially run before 
> `SampleProfileLoaderPass` was added to the pipeline.
> 
> The original patch that added `CodeGen/pgo-sample.c` (D21197) doesn't seem to 
> have any codegen tests either and only checks that PruneEH runs before sample 
> profile loader creation, so I'm also unsure of what code optimizations are 
> meant to happen.
Going back to this original issue, I think I understand more what is going on...

Specifically, I can imagine that having `invoke` still in place caused some 
issues.

So I would just keep the testing that (in the new PM) SimplifyCFG is run before 
the profile loading pass, and ignore function attrs.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63626



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63155#1563229 , @xur wrote:

> This patch does not make sense to me.
>
> The reason of failing with -fexperimental-new-pass-manager is because we 
> don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> instrumentation/use passes are under 
> PassBuilder::buildPerModuleDefaultPipeline.
>
> This patch add a pass
>
>   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
>
> which only gives you the wrong signal  of instrumentation is done.
>
> I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions 
> for thinlto under ldd for CSPGO.
>  Regular FDO should not use it.
>
> The right fix is to enable PGO instrumentation and use in pass builder for O0.
>
> I would like to request to revert this patch.


I mean, I'm happy for the patch to be reverted, but I still really don't 
understand why this fixes the test to work *exactly* the same as w/ the old 
pass manager and doesn't break any other tests if it is completely wrong? It 
seems like there must be something strange with the test coverage if this is so 
far off of correct without any failures...

I also don't understand what fix you are suggesting instead, but maybe you can 
show a patch?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63155



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63155#1563240 , @leonardchan wrote:

> In D63155#1563229 , @xur wrote:
>
> > This patch does not make sense to me.
> >
> > The reason of failing with -fexperimental-new-pass-manager is because we 
> > don't do PGO instrumentation at -O0. This is due to the fact that PGO 
> > instrumentation/use passes are under 
> > PassBuilder::buildPerModuleDefaultPipeline.
> >
> > This patch add a pass
> >
> >   MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
> >
> > which only gives you the wrong signal  of instrumentation is done.
> >
> > I wrote pass PGOInstrumentationGenCreateVar only for some trick 
> > interactions for thinlto under ldd for CSPGO.
> >  Regular FDO should not use it.
> >
> > The right fix is to enable PGO instrumentation and use in pass builder for 
> > O0.
> >
> > I would like to request to revert this patch.
>
>
> As an alternative, could I instead request that we remove the BackendUtil 
> changes and just mark the runs in gcc-flag-compatibility.c with 
> `-fno-experimental-new-pass-manager`. This way we could clarify that under 
> the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.


No, I think we should be doing PGO at O0 in the new PM if we do so in the old 
PM.

I think Rong is saying that the *way* you're enabling PGO at O0 isn't correct 
to fix this test case. That seems plausible to me at least, and I think 
reverting and figuring out what the right way to do it is a fine approach.

I just think we also need to understand why *no other test failed*, and why the 
only test we have for doing PGO at O0 is this one and it could be "fixed" but 
such a deeply unrelated change


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63155



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

[PATCH] D63155: [clang][NewPM] Fix broken profile test

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

In D63155#1563275 , @xur wrote:

> In D63155#1563266 , @chandlerc wrote:
>
> > I just think we also need to understand why *no other test failed*, and why 
> > the only test we have for doing PGO at O0 is this one and it could be 
> > "fixed" but such a deeply unrelated change
>
>
> We have special code to do PGO at O0 in old pass manager. This is not done in 
> the new pass manager.


I'm not sure how this addresses my question about test coverage.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63155



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

[PATCH] D62888: [NewPM] Port Sancov

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

The used thing still seems like there is an underlynig bug here. See below.

(Also a tiny nit, but that isn't the interesting part.)




Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:206
+
+std::string getSectionStart(const Triple &TargetTriple,
+const std::string &Section) {

Maybe call these `getFooBarImpl` so that you don't have to rely on name lookup 
trickery below to refer to them unambiguously?



Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:750
   Constant::getNullValue(ArrayTy), "__sancov_gen_");
+  appendToCompilerUsed(*CurModule, {Array});
 

There is a `GlobalsToAppendToCompilerUsed` vector that we append `Array` to 
below (line 759). Why isn't that sufficient instead of this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62888



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

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

[Chandler Carruth via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D61617



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Sorry I've been a bit slow to respond here...

In D61634#1503089 , @hfinkel wrote:

> In D61634#1502201 , @tejohnson wrote:
>
> > In D61634#1502138 , @hfinkel wrote:
> >
> > > In D61634#1502043 , @efriedma 
> > > wrote:
> > >
> > > > > I have a related patch that turns -fno-builtin* options into module 
> > > > > flags
> > > >
> > > > Do you have any opinion on representing -fno-builtin using a module 
> > > > flag vs. a function attribute in IR?  It seems generally more flexible 
> > > > and easier to reason about a function attribute from my perspective.  
> > > > But I might be missing something about the semantics of -fno-builtin 
> > > > that would make that representation awkward.  Or I guess it might just 
> > > > be more work to implement, given we have some IPO passes that use 
> > > > TargetLibraryInfo?
> > >
> > >
> > > I think that a function attribute would be better. We generally use these 
> > > flags only in the context of certain translation units, and when we use 
> > > LTO, it would be sad if we had to take the most-conservative settings 
> > > across the entire application. When we insert new function call to a 
> > > standard library, we always do it in the context of some function. We'd 
> > > probably need to block inlining in some cases, but that's better than a 
> > > global conservative setting.
> >
>


FWIW, I definitely agree here. This really is the end state we're going to find 
ourselves in and we should probably go directly there.

>> Using function level attributes instead of module flags does provide finer 
>> grained control and avoids the conservativeness when merging IR for LTO. The 
>> downsides I see, mostly just in terms of the engineering effort to get this 
>> to work, are:
>> 
>> - need to prevent inlining with different attributes
> 
> I think that this should be relatively straightforward. You just need to 
> update `AttributeFuncs::areInlineCompatible` and/or 
> `AttributeFuncs::mergeAttributesForInlining` by adding a new MergeRule in 
> include/llvm/IR/Attributes.td and writing a function like 
> adjustCallerStackProbeSize.

+1

> 
> 
>> - currently the TargetLibraryInfo is constructed on a per-module basis. 
>> Presumably it would instead need to be created per Function - this one in 
>> particular seems like it would require fairly extensive changes.
> 
> Interesting point. The largest issue I see is that we need TLI available from 
> loop passes, etc., and we can't automatically recompute a function-level 
> analysis there. We need to make sure that it's always available and not 
> invalidated. TLI is one of those analysis passes, being derived only from 
> things which don't change (i.e., the target triple), or things that change 
> very rarely (e.g., function attributes), that we probably don't want to 
> require all passes to explicitly say that they preserve it (not that the 
> mechanical change to all existing passes is hard, but it's easy to forget), 
> so I think we'd like something like the CFG-only concept in the current 
> passes, but stronger and perhaps turned on by default, for this kind of 
> "attributes-only" pass. (@chandlerc , thoughts on this?).

Yep, this makes sense.

The new PM makes this quite easy. The analysis itself gets to implement the 
invalidation hook, and say "nope, I'm not invalidated". In fact, in the new PM, 
`TargetLibraryInfo` already works this way. We build an instance per function 
and say it is never invalidated.

However, they are just trivial wrappers around shared implementations, so it 
will still require some non-trivial changes. Will need to remove the 
module-based access and move clients over to provide a function when they query 
it, etc.

IIRC, `TargetTransformInfo` already basically works exactly this way in both 
old and new PMs and we should be able to look at exactly the techniques it uses 
in both pass managers to build an effective way to manage these per-function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added inline comments.
This revision now requires changes to proceed.



Comment at: lib/CodeGen/CGCall.cpp:1739
   FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-  FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
 }

This seems like an unrelated change?



Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+   options::OPT_fomit_frame_pointer))
+return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+   mustUseFramePointerForTarget(Triple);

This doesn't correctly handle "last-flag-wins". Consider the case of 
`-mno-omit-leaf-frame-pointer -fomit-frame-pointer`. That should omit the leaf 
frame pointer, but if I read this correctly the logic here will use a leaf 
frame pointer.



Comment at: test/Driver/frame-pointer-elim.c:92
+
+// RUN: %clang -### -S -Os -fno-omit-frame-pointer 
-mno-omit-leaf-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=NO-OMIT-ALL2 %s

As indicated above, you need a test of the reverse order as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55915



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

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested changes to this revision.
chandlerc added reviewers: hans, rnk.
chandlerc added a comment.
This revision now requires changes to proceed.

The number of negations and such in the CC1 interface here and the attributes 
makes all of these tests super confusing. Wonder if we should fix that before 
making changes here.

Also adding folks to comment on the `cl.exe` driver mode and how it should 
behave.




Comment at: lib/CodeGen/CGCall.cpp:1739
   FuncAttrs.addAttribute("no-frame-pointer-elim", "true");
-  FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
 }

tabloid.adroit wrote:
> chandlerc wrote:
> > This seems like an unrelated change?
> The only user of "no-frame-pointer-elim-non-leaf" is 
> TargetOptions::DisableFramePointerElim where "no-frame-pointer-elim-non-leaf" 
> matters only if "no-frame-pointer-elim" is "false".  This is to make it less 
> confusing.
Yes, but that's kind of my point. This change is unrelated to the rest of the 
patch.

I would go ahead and land *just* this change and explain that it doesn't change 
behavior. Then the actual behavior change  can be landed independently.



Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+   options::OPT_fomit_frame_pointer))
+return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+   mustUseFramePointerForTarget(Triple);

tabloid.adroit wrote:
> chandlerc wrote:
> > This doesn't correctly handle "last-flag-wins". Consider the case of 
> > `-mno-omit-leaf-frame-pointer -fomit-frame-pointer`. That should omit the 
> > leaf frame pointer, but if I read this correctly the logic here will use a 
> > leaf frame pointer.
> Updated test with this case along with some other cases.
> 
> // RUN: %clang -### -S -Os -mno-omit-leaf-frame-pointer -fomit-frame-pointer 
> %s 2>&1 | \
> // RUN:   FileCheck --check-prefix=OMIT-ALL5 %s
> // OMIT-ALL5-NOT: "-mdisable-fp-elim"
> // OMIT-ALL5-NOT: "-momit-leaf-frame-pointer"
> 
> This falls into lib/CodeGen/CGCall.cpp:1733, which causes 
> TargetOptions::DisableFramePointerElim returns false for all frames.
> 
> 
Then I don't understand what this change is doing.

This function, when called with arguments `-mno-omit-leaf-frame-pointer 
-fomit-frame-pointer` will not hit the code you've added here, and will instead 
return `true`. That doesn't seem like a sensible result given the desired 
change to these flags. If something *else* is causing us to still not use leaf 
frame pointers, that doesn't make the code here correct, it makes me question 
how this works at all (and how we are testing it).



Comment at: test/Driver/cl-options.c:177
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /Oy- /O2 -### -- %s 2>&1 | 
FileCheck -check-prefix=Oy_2 %s
-// Oy_2: -momit-leaf-frame-pointer
+// Oy_2: -mdisable-fp-elim
 // Oy_2: -O2

Do we want to also change behavior for the CL options? We should discuss this 
w/ the Windows folks at least


Repository:
  rC Clang

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

https://reviews.llvm.org/D55915



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