[PATCH] D109531: [CSSPGO] Enable pseudo probe instrumentation in O0 mode.

wmi accepted this revision. wmi added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109531/new/ https://reviews.llvm.org/D109531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D109531: [CSSPGO] Enable pseudo probe instrumentation in O0 mode.

wmi added a comment. In D109531#2993484 , @hoy wrote: > In D109531#2993394 , @wmi wrote: > >> In D109531#2992721 , @hoy wrote: >> >>> In D109531#2992702

[PATCH] D109531: [CSSPGO] Enable pseudo probe instrumentation in O0 mode.

wmi added a comment. In D109531#2992721 , @hoy wrote: > In D109531#2992702 , @wenlei wrote: > >> The change makes sense given instr PGO also happens for O0. But practically, >> if a file is being built with O0,

[PATCH] D107876: [CSSPGO] Allow the use of debug-info-for-profiling and pseudo-probe-for-profiling together

wmi accepted this revision. wmi added a comment. In D107876#2939691 , @hoy wrote: > In D107876#2939624 , @wmi wrote: > >> Could you remind me the discriminator difference between debug info based >> AFDO and

[PATCH] D107876: [CSSPGO] Allow the use of debug-info-for-profiling and pseudo-probe-for-profiling together

wmi added a comment. Could you remind me the discriminator difference between debug info based AFDO and pseudo probe based AFDO? I forgot it. I am sure it is described in some patch but I havn't found it. Give me a pointer is good enough. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D99554: [ThinLTO] During module importing, close one source module before open another one for distributed mode.

This revision was automatically updated to reflect the committed changes. Closed by commit rGd535a05ca1a6: [ThinLTO] During module importing, close one source module before open (authored by wmi). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99554: [ThinLTO] During module importing, close one source module before open another one for distributed mode.

wmi added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1583 ModuleToDefinedGVSummaries[M->getModuleIdentifier()], - ModuleMap, CGOpts.CmdArgs)) { + nullptr, CGOpts.CmdArgs)) {

[PATCH] D99554: [ThinLTO] During module importing, close one source module before open another one for distributed mode.

wmi updated this revision to Diff 334240. wmi added a comment. Address Teresa's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99554/new/ https://reviews.llvm.org/D99554 Files: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D99554: [ThinLTO] During module importing, close one source module before open another one for distributed mode.

wmi created this revision. wmi added a reviewer: tejohnson. Herald added subscribers: steven_wu, hiraditya, inglorion. wmi requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber: cfe-commits. Currently during module importing, ThinLTO opens all the source

[PATCH] D95271: [CSSPGO] Passing the clang driver switch -fpseudo-probe-for-profiling to the linker.

wmi accepted this revision. wmi added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95271/new/ https://reviews.llvm.org/D95271 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D95271: [CSSPGO] Passing the clang driver switch -fpseudo-probe-for-profiling to the linker.

wmi added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:609-610 + // Pass an option to enable pseudo probe emission. + if (Args.hasFlag(options::OPT_fpseudo_probe_for_profiling, + options::OPT_fno_pseudo_probe_for_profiling)) +

[PATCH] D93264: [CSSPGO] Introducing distribution factor for pseudo probe.

wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land. LGTM. Thanks. Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:363 const FunctionSamples *CalleeSamples; uint64_t CallsiteCount; + // Call site distribution factor to

[PATCH] D93264: [CSSPGO] Introducing distribution factor for pseudo probe.

wmi added inline comments. Comment at: llvm/include/llvm/IR/PseudoProbe.h:41 // [18:3] - probe id - // [25:19] - reserved + // [25:19] - probe distribution factor // [28:26] - probe type, see PseudoProbeType hoy wrote: > wmi wrote: > > hoy wrote: > >

[PATCH] D93264: [CSSPGO] Introducing distribution factor for pseudo probe.

wmi added inline comments. Comment at: llvm/include/llvm/IR/PseudoProbe.h:41 // [18:3] - probe id - // [25:19] - reserved + // [25:19] - probe distribution factor // [28:26] - probe type, see PseudoProbeType hoy wrote: > wmi wrote: > > hoy wrote: > >

[PATCH] D93264: [CSSPGO] Introducing distribution factor for pseudo probe.

wmi added inline comments. Comment at: llvm/include/llvm/IR/PseudoProbe.h:41 // [18:3] - probe id - // [25:19] - reserved + // [25:19] - probe distribution factor // [28:26] - probe type, see PseudoProbeType hoy wrote: > hoy wrote: > > wmi wrote: > >

[PATCH] D93264: [CSSPGO] Introducing distribution factor for pseudo probe.

wmi added inline comments. Comment at: llvm/include/llvm/IR/PseudoProbe.h:41 // [18:3] - probe id - // [25:19] - reserved + // [25:19] - probe distribution factor // [28:26] - probe type, see PseudoProbeType The bits in discriminator is a scare

[PATCH] D91756: [CSSPGO] Pseudo probes for function calls.

wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land. In D91756#2427795 , @hoy wrote: > In D91756#2427759 , @wmi wrote: > >> Another question. Sorry for not bringing it

[PATCH] D91756: [CSSPGO] Pseudo probes for function calls.

wmi added a comment. Another question. Sorry for not bringing it up earlier. When a call with probe metadata attached is inlined, the probe will be gone or it will be kept somehow? I think you want to keep the probe especially for inline instance to reconstruct the context but I didn't figure

[PATCH] D91756: [CSSPGO] Pseudo probes for function calls.

wmi added inline comments. Comment at: llvm/include/llvm/IR/PseudoProbe.h:33-34 + static uint32_t packProbeData(uint32_t Index, uint32_t Type) { +assert(Index <= 0x); +assert(Type <= 0x7); +return (Index << 3) | (Type << 26) | 0x7; Add assertion

[PATCH] D91756: [CSSPGO] Pseudo probes for function calls.

wmi added inline comments. Comment at: llvm/lib/CodeGen/PseudoProbeInserter.cpp:50 +for (MachineBasicBlock : MF) { + MachineInstr *FirstInstr = nullptr; + for (MachineInstr : MBB) { What is the usage of FirstInstr? Comment at:

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

wmi accepted this revision. wmi added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

wmi added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; +

[PATCH] D86502: [CSSPGO] A Clang switch -fpseudo-probe-for-profiling for pseudo-probe instrumentation.

wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:978-982 Opts.DebugInfoForProfiling = Args.hasFlag( OPT_fdebug_info_for_profiling,

[PATCH] D86502: [CSSPGO] A Clang switch -fpseudo-probe-for-profiling for pseudo-probe instrumentation.

wmi added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:978-982 Opts.DebugInfoForProfiling = Args.hasFlag( OPT_fdebug_info_for_profiling, OPT_fno_debug_info_for_profiling, false); + Opts.PseudoProbeForProfiling = +

[PATCH] D86193: [CSSPGO] Pseudo probe instrumentation for basic blocks.

wmi added a comment. Herald added a subscriber: ecnelises. The patches split from the main one look good to me. Please see if David has further comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86193/new/ https://reviews.llvm.org/D86193

[PATCH] D86502: [CSSPGO] A Clang switch -fpseudo-probe-for-profiling for pseudo-probe instrumentation.

wmi added a comment. In D86502#2245578 , @hoy wrote: > In D86502#2245460 , @wmi wrote: > >>> The early instrumentation also allows the inliner to duplicate probes for >>> inlined instances. When a probe along with

[PATCH] D86502: [CSSPGO] A Clang switch -fpseudo-probe-for-profiling for pseudo-probe instrumentation.

wmi added a comment. Herald added a subscriber: danielkiss. > The early instrumentation also allows the inliner to duplicate probes for > inlined instances. When a probe along with the other instructions of a callee > function are inlined into its caller function, the GUID of the callee >

[PATCH] D86193: [CSSPGO] Pseudo probe instrumentation for basic blocks.

wmi added a comment. In D86193#2240502 , @hoy wrote: > In D86193#2240353 , @wmi wrote: > >>> There are some optimizations such as if-convert, tail call elimination, >>> that were initially blocked by the pseudo

[PATCH] D86193: [CSSPGO] Pseudo probe instrumentation for basic blocks.

wmi added a comment. > There are some optimizations such as if-convert, tail call elimination, that > were initially blocked by the pseudo probe intrinsic but is now unblocked by > fixes included in this change. With the current change we do not see perf > degradation out of SPEC and one of

[PATCH] D86193: [CSSPGO] Pseudo probe instrumentation for basic blocks.

wmi added a comment. Thanks for the patch! A few questions: > probe blocks some CFG transformations that can mess up profile correlation. Can you enumerate some CFG transformations which be blocked? Is it possible that some CFG transformations being blocked are actually beneficial for later

[PATCH] D79959: [SampleFDO] Add use-sample-profile function attribute

This revision was automatically updated to reflect the committed changes. Closed by commit rG7a6c89427c9b: [SampleFDO] Add use-sample-profile function attribute. (authored by wmi). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed prior to commit:

[PATCH] D77989: Allow disabling of vectorization using internal options

wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land. There is a comment about the bitwise operator, otherwise LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77989/new/

[PATCH] D77989: Allow disabling of vectorization using internal options

wmi added a comment. In D77989#1979725 , @wmi wrote: > The patch makes the reasoning about when the internal flags will take effect > much easier. > > One question is, with the change, the internal flags can only be used to > disable loop

[PATCH] D77989: Allow disabling of vectorization using internal options

wmi added a comment. The patch makes the reasoning about when the internal flags will take effect much easier. One question is, with the change, the internal flags can only be used to disable loop vectorization/slp vectorization/loop interleaving, but not to enable them. Can we treat them in

[PATCH] D73307: Unique Names for Functions with Internal Linkage

wmi added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:5 +// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE + +static int glob; MaskRay

[PATCH] D72538: [ThinLTO] Add additional ThinLTO pipeline testing with new PM

wmi accepted this revision. wmi 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/D72538/new/ https://reviews.llvm.org/D72538 ___

[PATCH] D72538: [ThinLTO] Add additional ThinLTO pipeline testing with new PM

wmi added a comment. The additional pipeline testing will catch any future pass change to the pipeline. A related but separate question is do we have a way to check whether there is any other missing pass in thinlto newpm similar as that in D72386 ?

[PATCH] D72386: [ThinLTO] pass UnrollLoops/VectorizeLoop/VectorizeSLP in CGOpts down to pass builder in ltobackend

This revision was automatically updated to reflect the committed changes. Closed by commit rG21a4710c67a9: [ThinLTO] Pass CodeGenOpts like UnrollLoops/VectorizeLoop/VectorizeSLP down to… (authored by wmi). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed prior to

[PATCH] D71485: [profile] Fix a crash when -fprofile-remapping-file= triggers an error

wmi accepted this revision. wmi 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/D71485/new/ https://reviews.llvm.org/D71485 ___

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi added a comment. In D36562#1642403 , @chill wrote: > In D36562#1641930 , @wmi wrote: > > > In D36562#1639441 , @chill wrote: > > > > > Shouldn't we disable

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi added a comment. In D36562#1639441 , @chill wrote: > Shouldn't we disable `OPT_ffine_grained_bitfield_accesses` only if TSAN is > active? I don't remember why it is disabled for all sanitizer modes. Seems you are right that the disabling the

[PATCH] D42154: Don't generate inline atomics for i386/i486

This revision was automatically updated to reflect the committed changes. Closed by commit rL323281: Adjust MaxAtomicInlineWidth for i386/i486 targets. (authored by wmi, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D39053: [Bitfield] Add more cases to making the bitfield a separate location

wmi added a comment. Thanks for the size evaluation. I regarded the change as a natural and limited extension to the current fine-grain bitfield access mode, so I feel ok with the change. Hal, what do you think? https://reviews.llvm.org/D39053

[PATCH] D42154: Don't generate inline atomics for i386/i486

wmi added a comment. In https://reviews.llvm.org/D42154#983840, @rjmccall wrote: > In https://reviews.llvm.org/D42154#977991, @wmi wrote: > > > In https://reviews.llvm.org/D42154#977975, @efriedma wrote: > > > > > The LLVM backend currently assumes every CPU is Pentium-compatible. If > > >

[PATCH] D42154: Don't generate inline atomics for i386/i486

wmi added inline comments. Comment at: lib/Basic/Targets/X86.h:472 +CPUKind Kind = getCPUKind(Opts.CPU); +if (Kind >= CK_i586 || Kind == CK_Generic) + MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64; wmi wrote: > craig.topper wrote: > >

[PATCH] D42154: Don't generate inline atomics for i386/i486

wmi added a comment. In https://reviews.llvm.org/D42154#977975, @efriedma wrote: > The LLVM backend currently assumes every CPU is Pentium-compatible. If we're > going to change that in clang, we should probably fix the backend as well. With the patch, for i386/i486 targets, clang will

[PATCH] D42154: Don't generate inline atomics for i386/i486

wmi created this revision. wmi added reviewers: rjmccall, eli.friedman, rsmith. Herald added subscribers: cfe-commits, eraman, sanjoy. This is to fix the bug reported in https://bugs.llvm.org/show_bug.cgi?id=34347#c6 Currently, all MaxAtomicInlineWidth of x86-32 targets are set to 64. However,

[PATCH] D39053: [Bitfield] Add more cases to making the bitfield a separate location

wmi added a comment. I think it may be hard to fix the problem in backend. It will face the same issue of store-to-load forwarding if at some places the transformation happens but at some other places somehow it doesn't. But I am not sure whether it is acceptable to add more finegrain bitfield

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

This revision was automatically updated to reflect the committed changes. Closed by commit rL315915: [Bitfield] Add an option to access bitfield in a fine-grained manner. (authored by wmi). Changed prior to commit: https://reviews.llvm.org/D36562?vs=118181=119170#toc Repository: rL LLVM

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi updated this revision to Diff 117947. wmi added a comment. Address Hal's comment. Repository: rL LLVM https://reviews.llvm.org/D36562 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.def

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi marked an inline comment as done. wmi added inline comments. Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:444 // Add bitfields to the run as long as they qualify. -if (Field != FieldEnd && Field->getBitWidthValue(Context) != 0 && -Tail ==

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi updated this revision to Diff 116896. wmi added a comment. Address Hal's comment. Separate bitfields to shards separated by the naturally-sized-and-aligned fields. Repository: rL LLVM https://reviews.llvm.org/D36562 Files: include/clang/Basic/DiagnosticDriverKinds.td

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi added a comment. In https://reviews.llvm.org/D36562#880808, @hfinkel wrote: > You seem to be only changing the behavior for the "separatable" fields, but I > suspect you want to change the behavior for the others too. The bitfield > would be decomposed into shards, separated by the

[PATCH] D38046: [Atomic][X8664] set max atomic inline/promote width according to the target

This revision was automatically updated to reflect the committed changes. Closed by commit rL313992: [Atomic][X8664] set max atomic inline width according to the target (authored by wmi). Changed prior to commit: https://reviews.llvm.org/D38046?vs=116107=116362#toc Repository: rL LLVM

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi updated this revision to Diff 116232. wmi added a comment. Changes following the discussion: - Put the bitfield split logic under an option and off by default. - When sanitizer is enabled, the option for bitfield split will be ignored and a warning message will be emitted. In addition, a

[PATCH] D38046: [Atomic][X8664] set max atomic inline/promote width according to the target

wmi updated this revision to Diff 116107. wmi added a comment. Herald added a subscriber: eraman. Address Eli's comments. Repository: rL LLVM https://reviews.llvm.org/D38046 Files: include/clang/Basic/TargetInfo.h lib/Basic/Targets.cpp lib/Basic/Targets/X86.h

[PATCH] D38046: [Atomic][X8664] set max atomic inline/promote width according to the target

wmi added inline comments. Comment at: lib/Basic/Targets/X86.h:898 + MaxAtomicPromoteWidth = 64; + MaxAtomicInlineWidth = 64; +} efriedma wrote: > wmi wrote: > > efriedma wrote: > > > I don't think we need to mess with MaxAtomicPromoteWidth? > > >

[PATCH] D38046: [Atomic][X8664] set max atomic inline/promote width according to the target

wmi added inline comments. Comment at: lib/Basic/Targets/X86.h:898 + MaxAtomicPromoteWidth = 64; + MaxAtomicInlineWidth = 64; +} efriedma wrote: > I don't think we need to mess with MaxAtomicPromoteWidth? > > Probably more intuitive to check "if

[PATCH] D38046: [Atomic][X8664] set max atomic inline/promote width according to the target

wmi updated this revision to Diff 115945. wmi added a comment. Address Eli's comment. Repository: rL LLVM https://reviews.llvm.org/D38046 Files: include/clang/Basic/TargetInfo.h lib/Basic/Targets.cpp lib/Basic/Targets/X86.h test/OpenMP/atomic_capture_codegen.cpp

[PATCH] D37310: [Atomic] Merge alignment information from Decl and from Type when emit atomic expression.

This revision was automatically updated to reflect the committed changes. Closed by commit rL312801: Use EmitPointerWithAlignment to get alignment information of the pointer used… (authored by wmi). Changed prior to commit: https://reviews.llvm.org/D37310?vs=114300=114384#toc Repository: rL

[PATCH] D37310: [Atomic] Merge alignment information from Decl and from Type when emit atomic expression.

wmi updated this revision to Diff 114300. wmi added a comment. Address John's comment. Repository: rL LLVM https://reviews.llvm.org/D37310 Files: lib/CodeGen/CGAtomic.cpp test/CodeGenCXX/atomic-align.cpp Index: test/CodeGenCXX/atomic-align.cpp

[PATCH] D37310: [Atomic] Merge alignment information from Decl and from Type when emit atomic expression.

wmi added a comment. Ping Repository: rL LLVM https://reviews.llvm.org/D37310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37310: [Atomic] Merge alignment information from Decl and from Type when emit atomic expression.

wmi created this revision. Herald added a subscriber: sanjoy. This is to fix PR34347. EmitAtomicExpr will only use alignment information from Type, instead of Decl, so when the declaration of an atomic variable is marked to have the alignment equal as its size, EmitAtomicExpr don't know about

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi updated this revision to Diff 112271. wmi added a comment. Try another idea suggested by David. All the bitfields in a single run are still wrapped inside of a large integer according to CGBitFieldInfo. For the bitfields with legal integer types and aligned, change their access manner when

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi added a comment. I limit the bitfield separation in the last update to only happen at the beginning of a run so no bitfield combine will be blocked. I think I need to explain more about why I change the direction and start to work on the problem in frontend. Keeping the information by

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi updated this revision to Diff 110641. wmi added a comment. Don't separate bitfield in the middle of a run because it is possible to hinder bitfields accesses combine. Only separate bitfield at the beginning of a run. Repository: rL LLVM https://reviews.llvm.org/D36562 Files:

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

wmi added a comment. In https://reviews.llvm.org/D36562#837594, @chandlerc wrote: > This has been discussed before and I still pretty strongly disagree with it. > > This cripples the ability of TSan to find race conditions between accesses to > consecutive bitfields -- and these bugs have