[PATCH] D101918: [clang][Driver] Add -fintegrate-as to debug-pass-structure test

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 accepted this revision.
evgeny777 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/D101918/new/

https://reviews.llvm.org/D101918

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

[PATCH] D99599: [NewPM] Add an option to dump pass structure

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

> Could you explain why -debug-pass-manager doesn't fit your use case?

I wanted to have something similar to -debug-pass=Structure, because the above 
is too verbose and lacks identation. What's the problem with this one, anyway?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99599

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

[PATCH] D99599: [NewPM] Add an option to dump pass structure

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

> I've already run into having to update these two golden file tests twice

I think you can just reduce the tests. No need to revert the entire change, 
unless there is a replacement for it (D101797 
 is not)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99599

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

[PATCH] D99599: [NewPM] Add an option to dump pass structure

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

Should be fixed with `c81ec19fba27ec`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99599

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

[PATCH] D99599: [NewPM] Add an option to dump pass structure

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

Hm ... I see BarrierNoop pass being added before `annotation-remarks`. Why's 
that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99599

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

[PATCH] D99599: [NewPM] Add an option to dump pass structure

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

@haowei What are LLVM configuration options? Also please send output from

  /opt/s/w/ir/x/w/staging/llvm_build/bin/clang -flegacy-pass-manager 
-fdebug-pass-structure -O0 -S -emit-llvm 
/opt/s/w/ir/x/w/llvm-project/clang/test/Driver/debug-pass-structure.c -o 
/dev/null 2>&1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99599

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

[PATCH] D99599: [NewPM] Add an option to dump pass structure

[Eugene Leviant via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6a0283d0d23c: [NewPM] Add an option to dump pass structure 
(authored by evgeny777).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99599

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/Driver/debug-pass-structure.c
  llvm/include/llvm/Passes/StandardInstrumentations.h
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/test/Other/opt-O3-pipeline.ll

Index: llvm/test/Other/opt-O3-pipeline.ll
===
--- llvm/test/Other/opt-O3-pipeline.ll
+++ llvm/test/Other/opt-O3-pipeline.ll
@@ -1,4 +1,5 @@
 ; RUN: opt -enable-new-pm=0 -mtriple=x86_64-- -O3 -debug-pass=Structure < %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK,%llvmcheckext %s
+; RUN: opt -enable-new-pm=1 -mtriple=x86_64-- -O3 -debug-pass-structure < %s -o /dev/null 2>&1 | FileCheck --check-prefixes=NEWPM,%llvmcheckext %s
 
 ; REQUIRES: asserts
 
@@ -335,6 +336,178 @@
 ; CHECK-NEXT: Branch Probability Analysis
 ; CHECK-NEXT: Block Frequency Analysis
 
+; NEWPM:  VerifierPass on [module]
+; NEWPM-NEXT:   VerifierAnalysis analysis on [module]
+; NEWPM-NEXT: Annotation2MetadataPass on [module]
+; NEWPM-NEXT: ForceFunctionAttrsPass on [module]
+; NEWPM-NEXT: InferFunctionAttrsPass on [module]
+; NEWPM-NEXT:   InnerAnalysisManagerProxy<{{.*}}> analysis on [module]
+; NEWPM-NEXT: ModuleToFunctionPassAdaptor on [module]
+; NEWPM-NEXT:   PassManager<{{.*}}> on f
+; NEWPM-NEXT: PreservedCFGCheckerAnalysis analysis on f
+; NEWPM-NEXT: LowerExpectIntrinsicPass on f
+; NEWPM-NEXT: SimplifyCFGPass on f
+; NEWPM-NEXT:   TargetIRAnalysis analysis on f
+; NEWPM-NEXT:   AssumptionAnalysis analysis on f
+; NEWPM-NEXT: SROA on f
+; NEWPM-NEXT:   DominatorTreeAnalysis analysis on f
+; NEWPM-NEXT: EarlyCSEPass on f
+; NEWPM-NEXT:   TargetLibraryAnalysis analysis on f
+; NEWPM-NEXT: CallSiteSplittingPass on f
+; NEWPM-NEXT: OpenMPOptPass on [module]
+; NEWPM-NEXT: IPSCCPPass on [module]
+; NEWPM-NEXT: CalledValuePropagationPass on [module]
+; NEWPM-NEXT: GlobalOptPass on [module]
+; NEWPM-NEXT: ModuleToFunctionPassAdaptor on [module]
+; NEWPM-NEXT:   InnerAnalysisManagerProxy<{{.*}}> analysis on [module]
+; NEWPM-NEXT:   PromotePass on f
+; NEWPM-NEXT: PreservedCFGCheckerAnalysis analysis on f
+; NEWPM-NEXT: DominatorTreeAnalysis analysis on f
+; NEWPM-NEXT: AssumptionAnalysis analysis on f
+; NEWPM-NEXT: DeadArgumentEliminationPass on [module]
+; NEWPM-NEXT: ModuleToFunctionPassAdaptor on [module]
+; NEWPM-NEXT:   PassManager<{{.*}}> on f
+; NEWPM-NEXT: InstCombinePass on f
+; NEWPM-NEXT:   TargetLibraryAnalysis analysis on f
+; NEWPM-NEXT:   OptimizationRemarkEmitterAnalysis analysis on f
+; NEWPM-NEXT:   TargetIRAnalysis analysis on f
+; NEWPM-NEXT:   AAManager analysis on f
+; NEWPM-NEXT: BasicAA analysis on f
+; NEWPM-NEXT:   OuterAnalysisManagerProxy<{{.*}}> analysis on f
+; NEWPM-NEXT: SimplifyCFGPass on f
+; NEWPM-NEXT: ModuleInlinerWrapperPass on [module]
+; NEWPM-NEXT:   InlineAdvisorAnalysis analysis on [module]
+; NEWPM-NEXT:   RequireAnalysisPass<{{.*}}> on [module]
+; NEWPM-NEXT: GlobalsAA analysis on [module]
+; NEWPM-NEXT:   CallGraphAnalysis analysis on [module]
+; NEWPM-NEXT:   RequireAnalysisPass<{{.*}}> on [module]
+; NEWPM-NEXT: ProfileSummaryAnalysis analysis on [module]
+; NEWPM-NEXT:   ModuleToPostOrderCGSCCPassAdaptor on [module]
+; NEWPM-NEXT: InnerAnalysisManagerProxy<{{.*}}> analysis on [module]
+; NEWPM-NEXT:   LazyCallGraphAnalysis analysis on [module]
+; NEWPM-NEXT: FunctionAnalysisManagerCGSCCProxy analysis on (f)
+; NEWPM-NEXT:   OuterAnalysisManagerProxy<{{.*}}> analysis on (f)
+; NEWPM-NEXT: DevirtSCCRepeatedPass on (f)
+; NEWPM-NEXT:   PassManager<{{.*}}> on (f)
+; NEWPM-NEXT: InlinerPass on (f)
+; NEWPM-NEXT: InlinerPass on (f)
+; NEWPM-NEXT: PostOrderFunctionAttrsPass on (f)
+; NEWPM-NEXT: ArgumentPromotionPass on (f)
+; NEWPM-NEXT: OpenMPOptCGSCCPass on (f)
+; NEWPM-NEXT: CGSCCToFunctionPassAdaptor on (f)
+; NEWPM-NEXT:   PassManager<{{.*}}> on f
+; NEWPM-NEXT: PreservedCFGCheckerAnalysis analysis on f
+; NEWPM-NEXT: SROA on f
+; NEWPM-NEXT:   DominatorTreeAnalysis analysis on f
+; NEWPM-NEXT: EarlyCSEPass on f
+; NEWPM-NEXT:   MemorySSAAnalysis analysis on f
+; NEWPM-NEXT: AAManager analysis on f
+; NEWPM-NEXT:   BasicAA analysis on f
+; NEWPM-NEXT: SpeculativeExecutionPass on f
+; NEWPM-NEXT: JumpThreadingPass on f
+; NEWPM-NEXT:   

[PATCH] D91812: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends

[Eugene Leviant via Phabricator via cfe-commits]

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

Thanks, LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91812

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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 accepted this revision.
evgeny777 added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

This needs to be rebased




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

tejohnson wrote:
> Here is the fix for the issue with the multi-stage clang bootstrap test 
> failures described in D75201. I also restructured this code to try to make it 
> clearer (rather than an early continue when we don't need to remove type test 
> assumes, make it an explicit removal when needed).
> 
> Added new test llvm/test/ThinLTO/X86/type_test_noindircall.ll for this.
Can you please also test the case when LTO unit is split?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

[PATCH] D75655: [Docs] Document -lto-whole-program-visibility

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

@pcc

> That case seems somewhat questionable to me. If the symbols are being 
> exported, it is presumably for the purpose of allowing the symbols to be used 
> outside of the defining DSO. But LTO visibility based optimizations could 
> make any such use of the symbols unsafe. For example with WPD it's unsafe to 
> derive outside of the defining DSO and with dead virtual function elimination 
> it's unsafe to call virtual functions outside of the defining DSO.

True, but still direct (cross DSO) calls and globals accesses are possible and 
do not require explicitly setting visibility everywhere. That was my point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75655



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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 accepted this revision.
evgeny777 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/D73242/new/

https://reviews.llvm.org/D73242



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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+// breaks any uses on assumes.
+if (TypeIdMap.count(TypeId))
+  continue;

tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > tejohnson wrote:
> > > > > evgeny777 wrote:
> > > > > > I don't think, I understand this.
> > > > > > It looks like that if (a) we have type.test in the module and (b) 
> > > > > > we don't have vtable definition with corresponding type metadata in 
> > > > > > the same module then we'll remove type.test/assume sequence before 
> > > > > > even getting to ICP. This seems strange in the context of previous 
> > > > > > discussion because virtual function may be called in different 
> > > > > > module from one where vtable is defined, like so:
> > > > > > ```
> > > > > > struct A { virtual int foo() { return 42; } };
> > > > > > 
> > > > > > // calls pA->foo
> > > > > > extern int callFoo(A *pA);
> > > > > > int main() { A a; return callFoo(); }
> > > > > > ```
> > > > > > 
> > > > > We will only be able to do ICP this way if we have the target vtable 
> > > > > in the module (similar to how we currently can only do ICP if the 
> > > > > target function is in the module). I anticipate doing something 
> > > > > similar for vtable defs as to what we do for virtual functions, where 
> > > > > a fake call edge is added to the summary based on the value profile 
> > > > > results to ensure they are imported before the LTO backend ICP.
> > > > > We will only be able to do ICP this way if we have the target vtable 
> > > > > in the module (similar to how we currently can only do ICP if the 
> > > > > target function is in the module).
> > > > 
> > > > I was thinking of slightly different approach: it seems you have 
> > > > required type information in combined index together with type name in 
> > > > the module, so you probably can add external declarations of required 
> > > > vtables (this may require promotion) to the module in the ICP pass and 
> > > > run ICP over them. Do you think this can work?
> > > Possibly, but we'd still need to have type metadata on those vtable 
> > > declarations, because the type metadata provides the address point offset 
> > > which is needed in the comparison sequence.
> > > Possibly, but we'd still need to have type metadata on those vtable 
> > > declarations, because the type metadata provides the address point offset 
> > > which is needed in the comparison sequence.
> > 
> > I think it's stored in the index in VFuncId structures. Isn't it?
> > I think it's stored in the index in VFuncId structures. Isn't it?
> 
> No, that holds the offset from the address point to the virtual function 
> within the vtable def, not the address point offset itself, which is what we 
> need from the type metadata. But actually, we need both offsets:
> 
> Consider the following example:
> 
> ```
> class A {
>virtual void foo();
> }
> 
> void bar(A *a) {
>a->foo();
> }
> ```
> 
> The indirect call sequence will look like (not showing the type test for 
> simplicity):
> 
> ```
>  %0 = bitcast %class.A* %a to i32 (%class.A*)***
>   %vtable = load i32 (%class.A*)**, i32 (%class.A*)*** %0
>   %1 = load i32 (%class.A*)*, i32 (%class.A*)** %vtable
>   %call = tail call i32 %1(%class.A* %a)
> ```
> 
> With type profiling we will profile the value of %vtable, and figure out that 
> it is associated with the symbol for A's vtable (the profiling support uses 
> symbol table info for this), which is:
> 
> ```
> @_ZTV1A = dso_local unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* 
> null, i8* bitcast ({ i8*, i8* }* @_ZTI1A to i8*), i8* bitcast (i32 
> (%class.A*)* @_ZN1A3fooEv to i8*)] }, align 8, !type !0
> 
> !0 = !{i64 16, !"_ZTS1A"}
> ```
> 
> When we promote the call using the vtable profile results, we need to add the 
> address point offset (16) from the type metadata to the profiled result which 
> will be the symbol @_ZTV1A, before comparing against %vtable.
> 
> We then need to look at the IR and compute the offset to the function address 
> used there (0 in the above IR), and add it to the address point offset, 
> resulting in 16 here, looking in the vtable def to see what function is at 
> the combined offset (_ZN1A3fooEv here), so that we insert a direct call to 
> that function.
> 
> For the address point offset, we only have it in the summary for index-only 
> WPD (TypeIdCompatibleVtableMap). However, I don't want to restrict the ICP 
> improvements to that build mode.
> 
> For the latter, the VFuncId does summarize the offsets, but we can compute it 
> from the IR as I described above (it's the amount added to %vtable before 
> loading the virtual function pointer) and don't need the summary. And in any 
> case the VFuncId doesn't tell us which function is at the aggregate offset, 
> we need the vtable def to tell us that (or the vTableFuncs array saved on the 
> 

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+// breaks any uses on assumes.
+if (TypeIdMap.count(TypeId))
+  continue;

tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > I don't think, I understand this.
> > > > It looks like that if (a) we have type.test in the module and (b) we 
> > > > don't have vtable definition with corresponding type metadata in the 
> > > > same module then we'll remove type.test/assume sequence before even 
> > > > getting to ICP. This seems strange in the context of previous 
> > > > discussion because virtual function may be called in different module 
> > > > from one where vtable is defined, like so:
> > > > ```
> > > > struct A { virtual int foo() { return 42; } };
> > > > 
> > > > // calls pA->foo
> > > > extern int callFoo(A *pA);
> > > > int main() { A a; return callFoo(); }
> > > > ```
> > > > 
> > > We will only be able to do ICP this way if we have the target vtable in 
> > > the module (similar to how we currently can only do ICP if the target 
> > > function is in the module). I anticipate doing something similar for 
> > > vtable defs as to what we do for virtual functions, where a fake call 
> > > edge is added to the summary based on the value profile results to ensure 
> > > they are imported before the LTO backend ICP.
> > > We will only be able to do ICP this way if we have the target vtable in 
> > > the module (similar to how we currently can only do ICP if the target 
> > > function is in the module).
> > 
> > I was thinking of slightly different approach: it seems you have required 
> > type information in combined index together with type name in the module, 
> > so you probably can add external declarations of required vtables (this may 
> > require promotion) to the module in the ICP pass and run ICP over them. Do 
> > you think this can work?
> Possibly, but we'd still need to have type metadata on those vtable 
> declarations, because the type metadata provides the address point offset 
> which is needed in the comparison sequence.
> Possibly, but we'd still need to have type metadata on those vtable 
> declarations, because the type metadata provides the address point offset 
> which is needed in the comparison sequence.

I think it's stored in the index in VFuncId structures. Isn't it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/lib/Passes/PassBuilder.cpp:1380
+  // in ICP (which is performed earlier than this in the regular LTO pipeline).
+  MPM.addPass(LowerTypeTestsPass(nullptr, nullptr, true));
 

tejohnson wrote:
> evgeny777 wrote:
> > Can we get rid of two identical passes here (and in other places also)? For 
> > instance 
> > ```
> > MPM.addPass(LowerTypeTestsPass(ExportSummary, nullptr, true));
> > ```
> > can both lower type metadata and do final cleanup.
> The problem is that currently under DropTypeTests=true, LTT will drop the 
> type tests up front and return without doing any other lowering. Which is 
> what we want in the already existing callers of LTT with DropTypeTests=true. 
> Here, we want LTT to perform its usual lowering, and only afterwards do the 
> dropping. We want to do this only during regular LTO, but unfortunately we 
> can't just key off of the ExportSummary (e.g. drop at the start if 
> ExportSummary is nullptr, and drop at the end if ExportSummary is 
> non-nullptr), since in some cases here ExportSummary may be nullptr (i.e. 
> regular LTO invoked via the old LTO API). So I would need to introduce 
> another option to essentially say "drop them after lowering". I can certainly 
> do this if you think it would be better (I was thinking about that before, 
> but thought it might be more confusing). WDYT?
> So I would need to introduce another option to essentially say "drop them 
> after lowering". I can certainly do this if you think it would be better (I 
> was thinking about that before, but thought it might be more confusing). WDYT?

Ok, I see. I don't think having this another option is significantly better. So 
let's leave it as is for now



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+// breaks any uses on assumes.
+if (TypeIdMap.count(TypeId))
+  continue;

tejohnson wrote:
> evgeny777 wrote:
> > I don't think, I understand this.
> > It looks like that if (a) we have type.test in the module and (b) we don't 
> > have vtable definition with corresponding type metadata in the same module 
> > then we'll remove type.test/assume sequence before even getting to ICP. 
> > This seems strange in the context of previous discussion because virtual 
> > function may be called in different module from one where vtable is 
> > defined, like so:
> > ```
> > struct A { virtual int foo() { return 42; } };
> > 
> > // calls pA->foo
> > extern int callFoo(A *pA);
> > int main() { A a; return callFoo(); }
> > ```
> > 
> We will only be able to do ICP this way if we have the target vtable in the 
> module (similar to how we currently can only do ICP if the target function is 
> in the module). I anticipate doing something similar for vtable defs as to 
> what we do for virtual functions, where a fake call edge is added to the 
> summary based on the value profile results to ensure they are imported before 
> the LTO backend ICP.
> We will only be able to do ICP this way if we have the target vtable in the 
> module (similar to how we currently can only do ICP if the target function is 
> in the module).

I was thinking of slightly different approach: it seems you have required type 
information in combined index together with type name in the module, so you 
probably can add external declarations of required vtables (this may require 
promotion) to the module in the ICP pass and run ICP over them. Do you think 
this can work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

This patch is to be rebased against D73094 




Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:830
   enum Kind {
+Unknown,   ///< Unknown (analysis not performed, don't lower)
 Unsat, ///< Unsatisfiable type (i.e. no global has this type metadata)

I don't think such implicit conversion of Unsat to Unknown is good idea. I 
guess problem will arise for legacy index files: for those ByteArray resolution 
will become Unsat and so on. I suggest moving Unknown the end of the list and 
bumping index version respectively (parseTypeIdSummaryRecord doesn't verify 
TheKind).



Comment at: llvm/lib/Passes/PassBuilder.cpp:1380
+  // in ICP (which is performed earlier than this in the regular LTO pipeline).
+  MPM.addPass(LowerTypeTestsPass(nullptr, nullptr, true));
 

Can we get rid of two identical passes here (and in other places also)? For 
instance 
```
MPM.addPass(LowerTypeTestsPass(ExportSummary, nullptr, true));
```
can both lower type metadata and do final cleanup.



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1678
+// breaks any uses on assumes.
+if (TypeIdMap.count(TypeId))
+  continue;

I don't think, I understand this.
It looks like that if (a) we have type.test in the module and (b) we don't have 
vtable definition with corresponding type metadata in the same module then 
we'll remove type.test/assume sequence before even getting to ICP. This seems 
strange in the context of previous discussion because virtual function may be 
called in different module from one where vtable is defined, like so:
```
struct A { virtual int foo() { return 42; } };

// calls pA->foo
extern int callFoo(A *pA);
int main() { A a; return callFoo(); }
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

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

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

> This is an enabler for upcoming enhancements to indirect call promotion, for 
> example streamlined promotion guard sequences that compare against vtable 
> address instead of the target function

Can you please describe the whole approach in more detail? At the moment ICP is 
capable to do (a sort of) devirtualization is replacing indirect vtbl call with 
sequence of function address comparisons and direct calls.
Are you going to speedup this by means of comparing vtable pointers instead of 
function pointers (thus eliminating a single load per each vtbl call) or there 
is also something else in mind? If that's true, what's the next
step? Make ICP pass analyze type test intrinsics?




Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1660
+cast(CI->getArgOperand(1))->getMetadata();
 // If we found any, add them to CallSlots.
 if (!Assumes.empty()) {

This change seems to be unrelated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

[PATCH] D73418: [WPD] Emit vcall_visibility metadata for MicrosoftCXXABI

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 accepted this revision.
evgeny777 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/D73418/new/

https://reviews.llvm.org/D73418



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

[PATCH] D71907: [WPD/VFE] Always emit vcall_visibility metadata for -fwhole-program-vtables

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 accepted this revision.
evgeny777 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/D71907/new/

https://reviews.llvm.org/D71907



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

[PATCH] D71911: [ThinLTO] Summarize vcall_visibility metadata

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 accepted this revision.
evgeny777 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/D71911/new/

https://reviews.llvm.org/D71911



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

Looks good so far. See remaining comment in D71907 





Comment at: clang/lib/CodeGen/CGVTables.cpp:1050
 
-  if (getCodeGenOpts().LTOVisibilityPublicStd) {
-const DeclContext *DC = RD;
-while (1) {
-  auto *D = cast(DC);
-  DC = DC->getParent();
-  if (isa(DC->getRedeclContext())) {
-if (auto *ND = dyn_cast(D))
-  if (const IdentifierInfo *II = ND->getIdentifier())
-if (II->isStr("std") || II->isStr("stdext"))
-  return false;
-break;
-  }
-}
-  }
+  if (HasLTOVisibilityPublicStd(RD))
+return false;

nit: return !HasLTOVisibilityPublicStd(RD)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71907: [WPD/VFE] Always emit vcall_visibility metadata for -fwhole-program-vtables

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/lib/Transforms/IPO/GlobalSplit.cpp:116
+if (GV.hasMetadata(LLVMContext::MD_vcall_visibility))
+  SplitGV->setVCallVisibilityMetadata(GV.getVCallVisibility());
   }

I think this needs a test. Removal of this code doesn't break anything


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71907



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > What caused this and other changes in this file?
> > > Because we now will insert type tests for non-hidden vtables. This is 
> > > enabled by the changes to LTO to interpret these based on the 
> > > vcall_visibility metadata.
> > The results of this test case
> > ```
> > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions 
> > -fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | 
> > FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
> > ```
> > look not correct to me. I think you shouldn't generate type tests for 
> > standard library classes with  `-flto-visibility-public-std`. Currently if 
> > this flag is given, clang doesn't do this either even with 
> > `-fvisibility=hidden`
> The associated vtables would get the vcall_visibility public metadata, so the 
> type tests themselves aren't problematic. I tend to think that an application 
> using such options should simply stick with -fvisibility=hidden to get WPD 
> and not use the new LTO option to convert all public vcall_visibility 
> metadata to hidden.
> The associated vtables would get the vcall_visibility public metadata, so the 
> type tests themselves aren't problematic. I tend to think that an application 
> using such options should simply stick with -fvisibility=hidden to get WPD 
> and not use the new LTO option to convert all public vcall_visibility 
> metadata to hidden.

I see two issues here:
1) It's not always good option to force hidden visibility for everything. For 
instance I work on proprietary platform which demands public visibility for 
certain symbols in order for dynamic loader to work properly. In this context 
your patch does a great job.

2) Standard library is almost never LTOed so in general we can't narrow std::* 
vtables visibility to linkage unit

Is there anything which prevents from implementing the same functionality with 
new -lto-whole-program-visibility option (i.e without forcing hidden 
visibility)? In other words the following looks good to me:

```
# Compile with lto/devirtualization support
clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c *.cpp

# Link: everything is devirtualized except standard library classes virtual 
methods
clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71907: [WPD/VFE] Always emit vcall_visibility metadata for -fwhole-program-vtables

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:676
+  bool ShouldEmitWPDInfo = CGM.getCodeGenOpts().WholeProgramVTables &&
+   CGM.HasHiddenLTOVisibility(RD);
   llvm::Value *VirtualFn = nullptr;

Why are we checking for hidden visibility here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71907



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

[PATCH] D71911: [ThinLTO] Summarize vcall_visibility metadata

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:762
+bool ReadOnly, bool WriteOnly, bool Constant,
+GlobalObject::VCallVisibility Vis = 
GlobalObject::VCallVisibilityPublic)
 : MaybeReadOnly(ReadOnly), MaybeWriteOnly(WriteOnly),

I think using default parameter is error prone here. Can you please get rid of 
it and update all places of usage respectively?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71911



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> evgeny777 wrote:
> > What caused this and other changes in this file?
> Because we now will insert type tests for non-hidden vtables. This is enabled 
> by the changes to LTO to interpret these based on the vcall_visibility 
> metadata.
The results of this test case
```
%clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions 
-fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | 
FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
```
look not correct to me. I think you shouldn't generate type tests for standard 
library classes with  `-flto-visibility-public-std`. Currently if this flag is 
given, clang doesn't do this either even with `-fvisibility=hidden`



Comment at: clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp:7
+// as expected.
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux 
-fvisibility hidden -fwhole-program-vtables -emit-llvm-bc -o %t.o %s
+// RUN: llvm-dis %t.o -o - | FileCheck --check-prefix=TT %s

I think, we no longer need `-fvisibility hidden` here, do we?



Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:976
+  // via the internal option. Must be done before WPD below.
+  updateVCallVisibilityInIndex(*Index);
+

evgeny777 wrote:
> ditto
updateVCallVisibilityInIndex(*Index, /* WholeProgramVisibilityEnabledInLTO */ 
false) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll:2
 ; Test that we correctly import an indir resolution for type identifier 
"typeid1".
-; RUN: opt -S -wholeprogramdevirt -wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility 
-wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t

tejohnson wrote:
> evgeny777 wrote:
> > Why do you need `-whole-program-visibility` here? Correct me if I'm wrong, 
> > but AFAIK module scanning doesn't happen during import and GV visibility 
> > should be taken from imported summary.
> Before my patch, type tests were only inserted for vtables with hidden LTO 
> visibility. Therefore, the very presence of type tests communicated the 
> hidden visibility and enabled the WPD.
> 
> With this patch, to support enabling WPD aggressively at LTO time, we now 
> insert type tests unconditionally under -fwhole-program-vtables. The 
> vcall_visibility metadata tells LTO how to interpret them. And the new 
> options allow changing those to hidden visibility to get the more aggressive 
> WPD.
> 
> Because these legacy tests have type tests but no vcall_visibility metadata, 
> we now will conservatively treat them as having public LTO visibility. This 
> option is therefore required to convert the summarized (default public) vcall 
> visibility into hidden to get the prior more aggressive behavior they are 
> trying to test.
> 
> Note I could have instead changed the assembly here to add hidden 
> vcall_visibility metadata everywhere. That seemed a little onerous so that's 
> why I just added the option. I could add a comment to that effect if it would 
> help?
I think you can remove this option from this test (and probably others using 
-wholeprogramdevirt-summary-action=import option), because visibility seems to 
be not analyzed on import phase. I just did this btw and test still passes 
flawlessly.



Comment at: llvm/tools/opt/opt.cpp:634
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ 
false);

tejohnson wrote:
> evgeny777 wrote:
> > Hm, looks like I don't fully understand this. I have following concerns:
> > 
> > 1) According to your changes every time I use `opt -wholeprogramdevirt` I 
> > also have to pass `-whole-program-visibility`. Has `-wholeprogramdevirt` 
> > flag become no-op without this additional flag? If so this looks counter 
> > intuitive to me.
> > 
> > 2) When I use `opt -wholeprogramdevirt` default constructor of 
> > `WholeProgramDevirt` class is called and `UseCommandLine` flag is set to 
> > true. Can't I use this flag to effectively lower visibility in module 
> > instead of playing with metadata?
> > 
> > ```
> > if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && 
> > !UseCommandLine)
> >  return false;
> > ```
> > 
> > According to your changes every time I use opt -wholeprogramdevirt I also 
> > have to pass -whole-program-visibility. Has -wholeprogramdevirt flag become 
> > no-op without this additional flag? If so this looks counter intuitive to 
> > me.
> 
> No, it wouldn't be needed if the tests contained !vcall_visibility metadata 
> indicating hidden LTO visibility (see my earlier comment response).
> 
> > When I use opt -wholeprogramdevirt default constructor of 
> > WholeProgramDevirt class is called and UseCommandLine flag is set to true. 
> > Can't I use this flag to effectively lower visibility in module instead of 
> > playing with metadata?
> 
> I could do that. What it would mean though is that we would be unable to use 
> opt for any future testing of vtables intended to have public vcall 
> visibility (either through a lack of that metadata, or explicit 
> vcall_vsibility metadata indicating public). Which might be ok - in fact all 
> my new testing of this behavior is via llvm-lto2 or the linkers. I suppose 
> that would obviate this change as well as all the opt based test changes to 
> pass the flag. Do you think that is better?
> I could do that. What it would mean though is that we would be unable to use 
> opt for any future testing of vtables intended to have public vcall 
> visibility (either through a lack of that metadata, or explicit 
> vcall_vsibility metadata indicating public). Which might be ok - in fact all 
> my new testing of this behavior is via llvm-lto2 or the linkers. I suppose 
> that would obviate this change as well as all the opt based test changes to 
> pass the flag. Do you think that is better?

Thanks for explanation. I think it's okay to have this extra option for 

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

Thanks, I'm still in process of testing (now fixing issue which however is most 
likely related to devirtualization itself, not to this patch). Meanwhile some 
of my comments below.




Comment at: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll:2
 ; Test that we correctly import an indir resolution for type identifier 
"typeid1".
-; RUN: opt -S -wholeprogramdevirt -wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility 
-wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t

Why do you need `-whole-program-visibility` here? Correct me if I'm wrong, but 
AFAIK module scanning doesn't happen during import and GV visibility should be 
taken from imported summary.



Comment at: llvm/tools/opt/opt.cpp:634
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ 
false);

Hm, looks like I don't fully understand this. I have following concerns:

1) According to your changes every time I use `opt -wholeprogramdevirt` I also 
have to pass `-whole-program-visibility`. Has `-wholeprogramdevirt` flag become 
no-op without this additional flag? If so this looks counter intuitive to me.

2) When I use `opt -wholeprogramdevirt` default constructor of 
`WholeProgramDevirt` class is called and `UseCommandLine` flag is set to true. 
Can't I use this flag to effectively lower visibility in module instead of 
playing with metadata?

```
if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && 
!UseCommandLine)
 return false;
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71911: [ThinLTO] Summarize vcall_visibility metadata

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

I think this has to be rebased - I see multiple failures when trying to apply


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71911



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:559
+  if (!CodeGenOpts.ThinLTOIndexFile.empty())
+MPM.add(createLowerTypeTestsPass(/*ExportSummary=*/nullptr,
+ /*ImportSummary=*/nullptr,

Test case?



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

What caused this and other changes in this file?



Comment at: llvm/include/llvm/Transforms/IPO.h:245
+ const ModuleSummaryIndex *ImportSummary,
+ bool StripAll = false);
 

s/StripAll/DropTypeTests/g ?



Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:550
+  // pipeline run below.
+  updateVCallVisibilityInModule(*MergedModule);
+

I'd rather use
```
updateVCallVisibilityInModule(*MergedModule,  /* 
WholeProgramVisibilityEnabledInLTO */ false)
```
and remove default value for second parameter



Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:976
+  // via the internal option. Must be done before WPD below.
+  updateVCallVisibilityInIndex(*Index);
+

ditto



Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1775
+if (TypeTestFunc) {
+  for (auto UI = TypeTestFunc->use_begin(), UE = TypeTestFunc->use_end();
+   UI != UE;) {

Fold identical code blocks



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:145
+/// when enabled via the linker.
+cl::opt DisableWholeProgramVisibility(
+"disable-whole-program-visibility", cl::init(false), cl::Hidden,

Is this tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71907: [WPD/VFE] Always emit vcall_visibility metadata for -fwhole-program-vtables

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

Looks good at first sight. Do you have linker patch for me to try this out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71907



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

[PATCH] D71911: [ThinLTO] Summarize vcall_visibility metadata

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added inline comments.



Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:794
+  }
+  GlobalObject::VCallVisibility vCallVisibility() const {
+return (GlobalObject::VCallVisibility)VarFlags.VCallVisibility;

getVCallVisibility() ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71911



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

[PATCH] D69173: [clang][ThinLTO][Legacy] Don't add -fsplit-lto-unit for legacy thin LTO builds

[Eugene Leviant via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1ae8e8d25fd8: Dont add -fsplit-lto-unit for thin LTO 
builds with PS4 and Darwin toolchains (authored by evgeny777).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D69173?vs=226049=226223#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69173

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/test/Driver/split-lto-unit.c


Index: clang/test/Driver/split-lto-unit.c
===
--- clang/test/Driver/split-lto-unit.c
+++ clang/test/Driver/split-lto-unit.c
@@ -3,6 +3,10 @@
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 
-fno-split-lto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 
-fno-split-lto-unit -fwhole-program-vtables 2>&1 | FileCheck 
--check-prefix=ERROR1 %s
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin 
-fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=ERROR2 %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s 
-fwhole-program-vtables -flto=full 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s 
-fwhole-program-vtables -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -fwhole-program-vtables 
-flto=full 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-scei-ps4 -### %s -fwhole-program-vtables 
-flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
 
 // UNIT: "-fsplit-lto-unit"
 // NOUNIT-NOT: "-fsplit-lto-unit"
Index: clang/lib/Driver/ToolChains/PS4CPU.h
===
--- clang/lib/Driver/ToolChains/PS4CPU.h
+++ clang/lib/Driver/ToolChains/PS4CPU.h
@@ -84,6 +84,10 @@
 
   SanitizerMask getSupportedSanitizers() const override;
 
+  // PS4 toolchain uses legacy thin LTO API, which is not
+  // capable of unit splitting.
+  bool canSplitThinLTOUnit() const override { return false; }
+
 protected:
   Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
Index: clang/lib/Driver/ToolChains/Darwin.h
===
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -258,6 +258,9 @@
 return "";
   }
 
+  // Darwin toolchain uses legacy thin LTO API, which is not
+  // capable of unit splitting.
+  bool canSplitThinLTOUnit() const override { return false; }
   /// }
 };
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5395,7 +5395,9 @@
 CmdArgs.push_back("-fwhole-program-vtables");
   }
 
-  bool DefaultsSplitLTOUnit = WholeProgramVTables || Sanitize.needsLTO();
+  bool DefaultsSplitLTOUnit =
+  (WholeProgramVTables || Sanitize.needsLTO()) &&
+  (D.getLTOMode() == LTOK_Full || TC.canSplitThinLTOUnit());
   bool SplitLTOUnit =
   Args.hasFlag(options::OPT_fsplit_lto_unit,
options::OPT_fno_split_lto_unit, DefaultsSplitLTOUnit);
Index: clang/include/clang/Driver/ToolChain.h
===
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -600,6 +600,10 @@
   virtual SanitizerMask getDefaultSanitizers() const {
 return SanitizerMask();
   }
+
+  /// Returns true when it's possible to split LTO unit to use whole
+  /// program devirtualization and CFI santiizers.
+  virtual bool canSplitThinLTOUnit() const { return true; }
 };
 
 /// Set a ToolChain's effective triple. Reset it when the registration object


Index: clang/test/Driver/split-lto-unit.c
===
--- clang/test/Driver/split-lto-unit.c
+++ clang/test/Driver/split-lto-unit.c
@@ -3,6 +3,10 @@
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit 2>&1 | FileCheck --check-prefix=NOUNIT %s
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fwhole-program-vtables 2>&1 | FileCheck --check-prefix=ERROR1 %s
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -fno-split-lto-unit -fsanitize=cfi 2>&1 | FileCheck --check-prefix=ERROR2 %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -fwhole-program-vtables -flto=full 2>&1 | FileCheck --check-prefix=UNIT %s
+// RUN: %clang -target x86_64-apple-darwin13.3.0 -### %s -fwhole-program-vtables -flto=thin 2>&1 | FileCheck --check-prefix=NOUNIT %s
+// RUN: %clang 

[PATCH] D54905: [AddressSanitizer] Add flag to disable linking with CXX runtime

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

> ASan uses -Wl,-whole-archive to pull all its members

Yep, I forgot about this. Looks like weak new/delete will do the job then.


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

https://reviews.llvm.org/D54905



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

[PATCH] D54905: [AddressSanitizer] Add flag to disable linking with CXX runtime

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

>   don't think so. It's just symbol interposition. Every module in the process 
> gets the same definition.

If I have two weak symbols in lib1.a and lib2.a and doesn't define strong one, 
which one will be chosen at link time?


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

https://reviews.llvm.org/D54905



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

[PATCH] D54905: [AddressSanitizer] Add flag to disable linking with CXX runtime

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

> How do you do this now?

Sometimes with this patch, sometimes simply issuing linker command line manually

> Is that related to https://github.com/google/sanitizers/issues/295 ?

I don't think so. I'm experiencing link error, not C++ standard discrepancy.

> Would turning asan operator new/delete into weak symbols help?

The new operator is already a weak symbol in libcxx, so it looks like an ODR 
violation, doesn't it?


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

https://reviews.llvm.org/D54905



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

[PATCH] D55048: [ThinLTO] Allow importing of multiple symbols with same GUID

[Eugene Leviant via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D55048



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

[PATCH] D54905: [AddressSanitizer] Add flag to disable linking with CXX runtime

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

Unfortunately, this is not an option. I have very custom heap implementation 
(in fact multiple heap types sharing contiguous memory block), so ASAN heap 
can't be a drop-in replacement. I'm using manual poisoning and it works pretty 
well, but have to disable C++ runtime each time I build with ASAN.


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

https://reviews.llvm.org/D54905



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

[PATCH] D54905: [AddressSanitizer] Add flag to disable linking with CXX runtime

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 created this revision.
evgeny777 added reviewers: kcc, samsonov.

This prevents link errors when operators new/delete are overridden by 
application.


https://reviews.llvm.org/D54905

Files:
  include/clang/Driver/Options.td
  lib/Driver/SanitizerArgs.cpp
  test/Driver/sanitizer-ld.c


Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -115,6 +115,12 @@
 // CHECK-ASAN-LINUX-CXX-STATIC: "--whole-archive" 
"{{.*}}libclang_rt.asan-i386.a" "--no-whole-archive"
 // CHECK-ASAN-LINUX-CXX-STATIC: stdc++
 
+// RUN: %clangxx -x c++ %s -### -o /dev/null -fsanitize=address 
-fno-sanitize-link-c++-runtime \
+// RUN: -target i386-unknown-linux -fuse-ld=ld 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ASAN-NO-LINK-CXX-RUNTIME %s
+//
+// CHECK-ASAN-NO-LINK-CXX-RUNTIME-NOT: asan_cxx
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target arm-linux-gnueabi -fuse-ld=ld -fsanitize=address \
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -743,7 +743,8 @@
 
   // Parse -link-cxx-sanitizer flag.
   LinkCXXRuntimes =
-  Args.hasArg(options::OPT_fsanitize_link_cxx_runtime) || D.CCCIsCXX();
+  Args.hasFlag(options::OPT_fsanitize_link_cxx_runtime,
+   options::OPT_fno_sanitize_link_cxx_runtime, D.CCCIsCXX());
 
   // Finally, initialize the set of available and recoverable sanitizers.
   Sanitizers.Mask |= Kinds;
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1015,6 +1015,8 @@
 Group;
 def fsanitize_link_cxx_runtime : Flag<["-"], "fsanitize-link-c++-runtime">,
  Group;
+def fno_sanitize_link_cxx_runtime : Flag<["-"], 
"fno-sanitize-link-c++-runtime">,
+ Group;
 def fsanitize_cfi_cross_dso : Flag<["-"], "fsanitize-cfi-cross-dso">,
   Group,
   HelpText<"Enable control flow integrity (CFI) 
checks for cross-DSO calls.">;


Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -115,6 +115,12 @@
 // CHECK-ASAN-LINUX-CXX-STATIC: "--whole-archive" "{{.*}}libclang_rt.asan-i386.a" "--no-whole-archive"
 // CHECK-ASAN-LINUX-CXX-STATIC: stdc++
 
+// RUN: %clangxx -x c++ %s -### -o /dev/null -fsanitize=address -fno-sanitize-link-c++-runtime \
+// RUN: -target i386-unknown-linux -fuse-ld=ld 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ASAN-NO-LINK-CXX-RUNTIME %s
+//
+// CHECK-ASAN-NO-LINK-CXX-RUNTIME-NOT: asan_cxx
+
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: -target arm-linux-gnueabi -fuse-ld=ld -fsanitize=address \
 // RUN: --sysroot=%S/Inputs/basic_android_tree/sysroot \
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -743,7 +743,8 @@
 
   // Parse -link-cxx-sanitizer flag.
   LinkCXXRuntimes =
-  Args.hasArg(options::OPT_fsanitize_link_cxx_runtime) || D.CCCIsCXX();
+  Args.hasFlag(options::OPT_fsanitize_link_cxx_runtime,
+   options::OPT_fno_sanitize_link_cxx_runtime, D.CCCIsCXX());
 
   // Finally, initialize the set of available and recoverable sanitizers.
   Sanitizers.Mask |= Kinds;
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1015,6 +1015,8 @@
 Group;
 def fsanitize_link_cxx_runtime : Flag<["-"], "fsanitize-link-c++-runtime">,
  Group;
+def fno_sanitize_link_cxx_runtime : Flag<["-"], "fno-sanitize-link-c++-runtime">,
+ Group;
 def fsanitize_cfi_cross_dso : Flag<["-"], "fsanitize-cfi-cross-dso">,
   Group,
   HelpText<"Enable control flow integrity (CFI) checks for cross-DSO calls.">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits