[PATCH] D72331: OpaquePtr: add type to inalloca attribute.

dblaikie accepted this revision. dblaikie added a subscriber: rnk. dblaikie added a comment. This revision is now accepted and ready to land. Looks pretty good to me - couple of minor things. If you want to check with @rnk (best point of contact on the inalloca feature in general), that'd probab

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1467-1468 if (Method->isStatic()) return cast_or_null( getOrCreateType(QualType(Func, 0), Unit)); + return getOrCreateInstanceMethodType(Method->getThisType(), Func, Unit, decl);

[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

dblaikie added a comment. What's the plan for this? Is it still in an experimental stage, with the intent to investigate the types that are no longer emitted unedr the flag & explain why they're missing (& either have a justification for why that's acceptable, or work on additional heuristics t

[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

dblaikie added a comment. In D72427#1812954 , @akhuang wrote: > > What's the plan for this? Is it still in an experimental stage, with the > > intent to investigate the types that are no longer emitted unedr the flag & > > explain why they're missing (&

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70524/new/ https://reviews.llvm.org/D70524 ___ cfe-commits mailin

[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3692 + Args.getLastArg(options::OPT_flimit_debug_info_constructor), Args, D, TC) && +DebugInfoKind == codegenoptions::LimitedDebugInfo) + DebugInfoKind = codegenoptions::Debug

[PATCH] D72565: adding __has_feature support for left c++ type_traits

dblaikie added a comment. Probably needs test coverage (perhaps check the commits that added other feature tests - there's probably somewhere you can plugin tests in a similar way to plugging in the feature tests themselves) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D69779: -fmodules-codegen should not emit extern templates

dblaikie added inline comments. Comment at: clang/test/PCH/codegen-extern-template.cpp:1-9 +// Build the PCH with extern template. +// RUN: %clang -x c++-header %S/codegen-extern-template.h -o %t.pch -Xclang -building-pch-with-obj -Xclang -fmodules-codegen +// Build the PCH's ob

[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

dblaikie added a comment. Looks pretty good to me - if you could commit the debug info level refactoring separately/up-front, and maybe the test case could be simplified a bit. Looking forward to seeing what comes of this option, analysis of missing types, etc. Comment at: cl

[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

dblaikie added a comment. In D72427#1818322 , @rnk wrote: > > Total object file size on Windows, compiling with RelWithDebInfo: > > > > before: 4,257,448 kb > > after: 2,104,963 kb > > > > And on Linux > > > > before: 9,225,140 kb > > after: 4,387,4

[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72427/new/ https://reviews.llvm.org/D72427 ___

[PATCH] D69779: -fmodules-codegen should not emit extern templates

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks for your patience! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69779/new/ https://reviews.llvm.org/D69779 ___

[PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

dblaikie added a comment. (just a general comment that this code review should be only in service of the design discussion happening on llvm-dev - please don't approve/commit this without closing out the design discussion there if there are actionable conclusions from this review) Repository:

[PATCH] D71451: Support to emit extern variables debuginfo with "-fstandalone-debug"

dblaikie added a comment. In D71451#1822497 , @Jac1494 wrote: > >> I'd be curious to the answer to David's questions. If the size increase is > >> because of unused extern variables coming in from libc or something then > >> it doesn't seem worth the cos

[PATCH] D86926: FormatTest: Provide real line number in failure messages

dblaikie added a comment. Could this be done with a custom matcher, perhaps? (might be a bit tidier than macros with __FILE__ and __LINE__ manually handled, etc? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86926/new/ https://reviews.llvm.org/D86

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

dblaikie added a subscriber: rsmith. dblaikie added a comment. @rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the o

[PATCH] D87837: [Driver] Add disabled-by-default -Wuse-ld-non-word for the deprecation warning for -fuse-ld=/abs/path

dblaikie added a comment. Rather than "non-word" maybe "path" would be more legible (that's the terminology the message text uses, and seems shorter/clearer in the warning name/constant name/etc), ie: -Wuse-ld-path (how do other warnings about flags do naming regarding the 'f' (or 'g', etc) pr

[PATCH] D87837: [Driver] Add disabled-by-default -Wuse-ld-path for the deprecation warning for -fuse-ld=/abs/path

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D87837#2283784 , @chandlerc wrote: > Good to confirm with Dave of course, but this looks pretty great to me, and > thanks so much! Works for me

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

dblaikie added a comment. In D87808#2286664 , @rsmith wrote: > In D87808#2280197 , @dblaikie wrote: > >> @rsmith What's the deal with these anonymous structs/unions? Why do they >> have copy/move constructors (are

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2292-2300 + bool hasCtor = false; + for (const auto *Ctor : RD->ctors()) { if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor()) return false; +if (!Ctor->isCopyOrMoveConstructor(

[PATCH] D87673: [clangd] Don't use zlib when it's unavailable.

dblaikie added a comment. In D87673#2275940 , @sammccall wrote: > Thanks, this seems better than crashing. > The practical result isn't wonderful: the two are going to fight over index > files to some extent. But this can happen with different clangd vers

[PATCH] D87673: [clangd] Don't use zlib when it's unavailable.

dblaikie added a comment. In D87673#2290926 , @sammccall wrote: > In D87673#2290838 , @dblaikie wrote: > >> In D87673#2275940 , @sammccall >> wrote: >> >>> Thanks, this seem

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

dblaikie added a comment. In D80531#2284388 , @kkleine wrote: > Hi @dblaikie . I did run `ninja check-all` and `/bin/llvm-lit -av > ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp` > on this very new rev

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Generally looks good to me! Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2287 + // don't have trivial or constexpr constructors, or can be created from + // aggregate

[PATCH] D81865: [clang] Use string tables for static diagnostic descriptions

dblaikie added a comment. In D81865#2293059 , @MaskRay wrote: > @froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 > appears > to be very different from the revi

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2224297 , @nhaehnle wrote: >> Not sure that's the best place to be designing this fairly integral and >> complicated piece of infrastructure from, but hoping we can find some good >> places/solutions/etc. > > I sent an

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. (side note: this code review is a bit hard to follow with all the linting messages about naming - might be a bit more readable if it conformed to the naming conventions?) In D83088#2227151 , @nhaehnle wrote: > In D83088#2225415

[PATCH] D81347: Make ASTFileSignature an array of 20 uint8_t instead of 5 uint32_t

dblaikie added a comment. In D81347#2080217 , @aprantl wrote: > I was going to ask why make this change, but looking at the patch, it's > pretty obvious :-) Might be worth writing it down for everyone else - isn't exactly obvious to me (though not a par

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Thanks for testing/catching this (what sort of testing have you been doing that discovered this?)! & yeah, the way shouldOmitDefinition is now written (with this fix) is right - that ever

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

dblaikie accepted this revision. dblaikie added a comment. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86491/new/ https://reviews.llvm.org/D86491 ___ cfe-commits mailing list cfe-co

[PATCH] D81865: [clang] Use string tables for static diagnostic descriptions

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Herald added a subscriber: danielkiss. Sounds good to me - thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81865/new/ https://reviews.llvm.org/D81865 __

[PATCH] D87049: Exploratory patch - capture DebugInfo for constexpr variables used within lambda

dblaikie added a comment. I think it'd be helpful to discuss what the DWARF does and should look like in this case (I wonder if, maybe, the better solution is in the DWARF consumer - to do name lookup out through the lambda) - could you provide some summarized llvm-dwarfdump output comparisons

[PATCH] D87062: [DebugInfo] Add size to class declarations in debug info.

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1035 + const RecordDecl *D = RD->getDefinition(); + if (D && D->isCompleteDefinition()) +Size = CGM.getContext().getTypeSize(Ty); When is a definition not a complete definition? (

[PATCH] D87062: [DebugInfo] Add size to class declarations in debug info.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87062/new/ https://reviews.llvm.org/D87062 __

[PATCH] D87103: [test] Use %t instead of %T to remove race conditions between config-file3.c and target-override.c

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87103/new/ https://reviews.llvm.org/D87103 _

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

dblaikie added subscribers: sammccall, dblaikie. dblaikie added a comment. In D80531#2069637 , @kwk wrote: > In D80531#2069383 , @njames93 wrote: > >> LGTM with 2 small nits, but I'd still give a few days see if any

[PATCH] D78938: Make LLVM build in C++20 mode

dblaikie added a comment. In D78938#2262713 , @jhenderson wrote: > In D78938#2261411 , @jfb wrote: > >> On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then >> it's easier to un-rot with Barry

[PATCH] D85408: Let -basic-block-sections=labels emit basicblock metadata in a new .bb_addr_map section, instead of emitting special unary-encoded symbols.

dblaikie added a comment. In D85408#2262134 , @rahmanl wrote: > @efriedma Would you please chime in specially with respect to @MaskRay 's > comment about the clang test involving assembly? I'll chime in, perhaps @efriedma will/can too: This patch has no

[PATCH] D92809: [Driver] Add -gno-split-dwarf which can disable debug fission

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92809/new/ https://reviews.llvm.org/D92809 __

[PATCH] D92888: ADT: Allow IntrusiveRefCntPtr construction from std::unique_ptr, NFC

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good with one minor tweak Comment at: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:180 + template + IntrusiveRefCntPtr(std::unique_ptr &&S) : Obj(S.release()) { +

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie added inline comments. Comment at: clang/test/Driver/cuda-unsupported-debug-options.cu:18 +// RUN: %clang -### -target x86_64-linux-gnu -c %s -gdwarf-5 -gembed-source 2>&1 | FileCheck %s --check-prefix=DWARF-CLAMP +// CHECK: debug information option '{{-gz|-fdebug-info

[PATCH] D92617: [DWARF] Allow toolchain to adjust specified DWARF version.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Fair enough - thanks for your patience/explanations! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92617/new/ https://reviews.llvm.org/D9261

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added a subscriber: rnk. dblaikie added a comment. This should have a LLVM test coverage for the LLVM changes. (I realize they're also tested by the Clang test, because there's no way to test Clang's pass manager creation short of testing the effect of running the pass manager (hmm - /

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2056-2059 + void replaceRawLinkageName(MDString *LinkageName) { +replaceOperandWith(3, LinkageName); + } + hoy wrote: > tmsriram wrote: > > dblaikie wrote: > > > The need t

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added a comment. In D93656#2469485 , @hoy wrote: > In D93656#2468841 , @aeubanks wrote: > >> In D93656#2468834 , @hoy wrote: >> >>> In D93656#2468821

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

dblaikie added a comment. Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang) I'd still be curious if you could look around to see whether there are other cases of function

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49 +// LPIPELINE: Unique Internal Linkage Names +// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass // PLAIN: @_ZL4glob = internal global Does this test

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49 +// LPIPELINE: Unique Internal Linkage Names +// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass // PLAIN: @_ZL4glob = internal global hoy wrote: > d

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49 +// LPIPELINE: Unique Internal Linkage Names +// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass // PLAIN: @_ZL4glob = internal global hoy wrote: > a

[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

dblaikie added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49 +// LPIPELINE: Unique Internal Linkage Names +// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass // PLAIN: @_ZL4glob = internal global aeubanks wrote

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie added a comment. Test case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90714/new/ https://reviews.llvm.org/D90714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

dblaikie added a comment. Does Chromium need this fixed in clang? Or if it were fixed in libc++ would that be adequate? (does Chromium's build need to work with old libc++s, or does it always build with a libc++ that matches the compiler? (in the latter case, a fix in libc++ would be as good as

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

dblaikie added a comment. In D90719#2372554 , @rnk wrote: > In D90719#2372463 , @dblaikie wrote: > >> Does Chromium need this fixed in clang? Or if it were fixed in libc++ would >> that be adequate? (does Chromium'

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie added a comment. Since the same code is used to mangle all these things, probably just test one of them? Could use macros to stamp out longer names without having to write them out manually? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

dblaikie added a comment. In D90719#2376388 , @rnk wrote: > In D90719#2372656 , @dblaikie wrote: > >> My understanding is that such code is UB, is that right? > > I guess I'm not convinced it's UB, and need some lan

[PATCH] D90448: [clang] Add type check for explicit instantiation of static data members

dblaikie added a comment. How's this compare to the similar checks for variable templates? Is there some code/checking we could share here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90448/new/ https://reviews.llvm.org/D90448 _

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie added a comment. In D90714#2375911 , @mibintc wrote: > In D90714#2374913 , @dblaikie wrote: > >> Since the same code is used to mangle all these things, probably just test >> one of them? >> >> Could use m

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie added a comment. In D90714#2376667 , @rnk wrote: > lgtm > > I think the test case looks good as is, FWIW. Token pasting might make it > more readable, but it's improving the existing test, and not necessarily in > scope for this patch. Oh, sorr

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie added a comment. In D90714#2377231 , @mibintc wrote: > I'm sorry, I don't see how to build up an identifier with an arbitrary number > of characters using the token pasting, for example an ident with 4095 > characters is not a power of 2. I trie

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

dblaikie added a comment. In D90719#2377324 , @rnk wrote: > I had another thought, which is that even if it is UB, perhaps we really > shouldn't be using UB as the basis for debug info emission. All programs have > bugs, and most bugs invoke some form of

[PATCH] D90874: [test] Properly test -Werror-implicit-function-declaration and -Wvec-elem-size

dblaikie added a comment. Thanks for taking a look at this! Comment at: clang/test/Sema/implicit-decl.c:2 +// RUN: %clang_cc1 %s -verify=expected,implicit -fsyntax-only -Werror=implicit-function-declaration +// RUN: %clang_cc1 %s -verify -fsyntax-only -Wno-implicit-function-de

[PATCH] D90874: [test] Properly test -Werror-implicit-function-declaration and -Wvec-elem-size

dblaikie added inline comments. Comment at: clang/test/Sema/vecshift.c:3-4 +// RUN: %clang_cc1 -fsyntax-only -DEXT -DERR -verify=expected,vecelemsize %s +// RUN: %clang_cc1 -fsyntax-only -DERR -verify %s -Wno-vec-elem-size +// RUN: %clang_cc1 -fsyntax-only -DEXT -DERR -verify %s

[PATCH] D90874: [test] Properly test -Werror-implicit-function-declaration and -Wvec-elem-size

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90874/new/ https://reviews.llvm.org/D90874 _

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

dblaikie added a comment. In D90719#2377883 , @rsmith wrote: > Perhaps we could address both the UB and the debug info homing issue by > adding a new attribute on the libc++ types that says it's valid to create an > instance of the type without calling a

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie accepted this revision. dblaikie added a comment. Generally looks good, thanks! The inline comment trying to understand the `4095 - 4088 == 7` math I think should be answered (possibly in the form of the test/CHECK rephrasing I mentioned to clarify what the extra 7 characters are, and

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

dblaikie added a comment. In D90714#2379785 , @mibintc wrote: > you're right about the regex syntax. i think the 4088 is over the max > repetition count and there's also a problem with balanced. not sure what i > did wrong here. if you have a chance to l

[PATCH] D90448: [clang] Add type check for explicit instantiation of static data members

dblaikie added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:2339-2353 /// Determine whether the given types \p T1 and \p T2 are equivalent. - bool hasSameType(QualType T1, QualType T2) const { -return getCanonicalType(T1) == getCanonicalType(T2); + /

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

dblaikie added a comment. In D90719#2387379 , @akhuang wrote: > + @ldionne for libc++ input? > > To summarize, this constructor homing debug info optimization makes the > assumption that if a class is being used then its constructor must have been > call

[PATCH] D91229: [FileCheck] Flip -allow-unused-prefixes to false by default

dblaikie added a comment. > because FileCheck won't accept more than one occurrence of > --allow-unused-prefixes Perhaps this is a fixable issue? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91229/new/ https://reviews.llvm.org/D91229 __

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

dblaikie added a comment. > Performing the mandatory inlinings first simplifies the problem the full > inliner needs to solve That confuses me a bit - is that suggesting that we don't run the AlwaysInliner when we are running the Inliner (ie: we only run the AlwaysInliner at -O0, and use the I

[PATCH] D90507: Adding DWARF64 clang flag

dblaikie added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:35 CODEGENOPT(AsmVerbose, 1, 0) ///< -dA, -fverbose-asm. +CODEGENOPT(Dwarf64 , 1, 0) ///< -gdwarf64. CODEGENOPT(PreserveAsmComments, 1, 1) ///< -dA, -fno-preserve-as-comme

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

dblaikie added a comment. In D91567#2398461 , @mtrofin wrote: > In D91567#2398440 , @dblaikie wrote: > >>> Performing the mandatory inlinings first simplifies the problem the full >>> inliner needs to solve >> >> T

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. In D91311#2400775 , @ldionne wrote: > In D91311#2398526 , @rsmith wrote: > >> [...] > > Thanks for your detailed explanation. I did not understand the philosophy of > the attribute, and it

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. In D91311#2400926 , @ldionne wrote: > In D91311#2400917 , @dblaikie wrote: > >> How would that work for users - they would get error messages from the >> compiler using type names that don

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

dblaikie added a comment. In D91567#2398637 , @mtrofin wrote: > In D91567#2398623 , @dblaikie wrote: > >> In D91567#2398461 , @mtrofin wrote: >> >>> In D91567#2398440

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

dblaikie added a comment. Thanks for the walkthroughs/help. Also stared at the code a bit. I think I get it now. Some of the confusion also came from having both LPM and NPM versions of the always inliner in the same file, though they seem to share no code. I'll leave the more nuanced review to

[PATCH] D90507: Adding DWARF64 clang flag: -gdwarf64

dblaikie added inline comments. Comment at: clang/include/clang/Driver/Options.td:2145-2146 HelpText<"Generate source-level debug information with dwarf version 5">; +def gdwarf64 : Flag<["-"], "gdwarf64">, Group, Flags<[CC1Option]>, + HelpText<"Generate DWARF64 debug informa

[PATCH] D90507: Adding DWARF64 clang flag: -gdwarf64

dblaikie added inline comments. Comment at: clang/include/clang/Driver/Options.td:2145-2146 HelpText<"Generate source-level debug information with dwarf version 5">; +def gdwarf64 : Flag<["-"], "gdwarf64">, Group, Flags<[CC1Option]>, + HelpText<"Generate DWARF64 debug informa

[PATCH] D90507: Adding DWARF64 clang flag: -gdwarf64

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4023 + D.Diag(diag::err_drv_argument_only_allowed_with) + << A->getAsString(Args) << "DWARVv3 or greater"; +else if (!RawTriple.isArch64Bit())

[PATCH] D90507: Adding DWARF64 clang flag: -gdwarf64

dblaikie added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:35 CODEGENOPT(AsmVerbose, 1, 0) ///< -dA, -fverbose-asm. +CODEGENOPT(Dwarf64 , 1, 0) ///< -gdwarf64. CODEGENOPT(PreserveAsmComments, 1, 1) ///< -dA, -fno-preserve-as-comme

[PATCH] D90507: [Driver] Add DWARF64 flag: -gdwarf64

dblaikie added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:35 CODEGENOPT(AsmVerbose, 1, 0) ///< -dA, -fverbose-asm. +CODEGENOPT(Dwarf64 , 1, 0) ///< -gdwarf64. CODEGENOPT(PreserveAsmComments, 1, 1) ///< -dA, -fno-preserve-as-comme

[PATCH] D90507: [Driver] Add DWARF64 flag: -gdwarf64

dblaikie added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:35 CODEGENOPT(AsmVerbose, 1, 0) ///< -dA, -fverbose-asm. +CODEGENOPT(Dwarf64 , 1, 0) ///< -gdwarf64. CODEGENOPT(PreserveAsmComments, 1, 1) ///< -dA, -fno-preserve-as-comme

[PATCH] D78938: Make LLVM build in C++20 mode

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/BasicBlock.h:324-325 +template ::value>> phi_iterator_impl(const phi_iterator_impl &Arg) BRevzin wrote: > dblaikie wrote: > > What tripped over/required this SFINAE? > There's somewhere whi

[PATCH] D78938: Make LLVM build in C++20 mode

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/BasicBlock.h:324-325 +template ::value>> phi_iterator_impl(const phi_iterator_impl &Arg) Quuxplusone wrote: > dblaikie wrote: > > BRevzin wrote: > > > dblaikie wrote: > > > > What tripped o

[PATCH] D78938: Make LLVM build in C++20 mode

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/BasicBlock.h:324-325 +template ::value>> phi_iterator_impl(const phi_iterator_impl &Arg) dblaikie wrote: > Quuxplusone wrote: > > BRevzin wrote: > > > dblaikie wrote: > > > > BRevzin wrote:

[PATCH] D88522: [DebugInfo] Add types from constructor homing to the retained types list.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88522/new/ https://reviews.llvm.org/D88522 _

[PATCH] D88603: [WebAssembly] Add support for DWARF type units

dblaikie added inline comments. Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:962 + case Triple::Wasm: +return Ctx->getWasmSection(Name, SectionKind::getMetadata(), utostr(Hash), + ~0); dschuff wrote: > dschuff wrote: > > I may a

[PATCH] D88603: [WebAssembly] Add support for DWARF type units

dblaikie added inline comments. Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:962 + case Triple::Wasm: +return Ctx->getWasmSection(Name, SectionKind::getMetadata(), utostr(Hash), + ~0); dschuff wrote: > dblaikie wrote: > > dschuf

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

dblaikie added a comment. I'll probably leave the llvm/CodeView parts of this for @rnk - but for the clang parts, they should have corresponding clang test coverage & I'd be curious to see the test for that to understand what the IR was before/after this change. (& to understand if/how either o

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-static-member.cpp:144-147 +// For some reason, const_va is not emitted when the target is MS. +// NOT-MS: !DIDerivedType(tag: DW_TAG_member, name: "const_va", +// NOT-MS-SAME: line: [[@LINE-3]]

[PATCH] D78658: [clang][Frontend] Add missing error handling

dblaikie added a comment. Perhaps this'd be more robust with ScopeExit? ( https://llvm.org/doxygen/ScopeExit_8h_source.html ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78658/new/ https://reviews.llvm.org/D78658 ___ cfe-commits mailing l

[PATCH] D89286: [DebugInfo] Check for templated static data member when adding constant to record static fields

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1421-1425 + else if (auto *TemplateDecl = Var->getInstantiatedFromStaticDataMember()) { +// Inline static data members might not have an initialization. +if (TemplateDecl->getInit()) + Val

[PATCH] D78658: [clang][Frontend] Add missing error handling

dblaikie added a comment. In D78658#2327615 , @LemonBoy wrote: >> Perhaps this'd be more robust with ScopeExit? > > Not really, `OnError` is not executed when/if the function succeeds. Ah, sorry - ScopeExit has a 'release()' function intended to disengage

[PATCH] D78658: [clang][Frontend] Add missing error handling

dblaikie accepted this revision. dblaikie added inline comments. Comment at: clang/lib/Frontend/ASTUnit.cpp:1158-1160 + if (!Clang->hasTarget()) { return true; + } You can remove these braces now - since it's a superfluous change here now there's no need

[PATCH] D91840: OpaquePtr: Require byval on x86_intrcc parameter 0

dblaikie added a comment. (generally I'd suggest splitting the clangc and llvm patches if at all possible - in this case I'm guessing the clang patch could go first (adding the attribute which would be redundant/unused initially), then the LLVM one?) CHANGES SINCE LAST ACTION https://reviews

[PATCH] D92512: ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC

dblaikie accepted this revision. dblaikie added a comment. Yeah, looks good to me - I'd probably go with your suggestion of committing the cleanup pre-emptively, then the alias change. But dealer's choice. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92512/new/ https://reviews.llvm.or

[PATCH] D92516: ADT: Migrate users of AlignedCharArrayUnion to std::aligned_union_t, NFC

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92516/new/ https://reviews.llvm.org/D92516

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:75-76 RefCountedBase() = default; + // Copy and move constructors/assignments are no-ops as the RefCount isn't + // dictated by the class directly. RefCountedBase(const RefCountedBase &

[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

dblaikie added inline comments. Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:190-196 +// Use a custom deleter for the TrueMatcherInstance ManagedStatic. This prevents +// an assert firing when the refcount is nonzero while running its destructor. +struct DynMatcherI

[PATCH] D92606: Clean up debug locations for logical-and/or expressions

dblaikie added a comment. Probably worth separating these changes/fixes/tests (looks like 3 different changes?) - at least it'd help me understand what each one of the changes does, separately (I'm especially curious about the EmitScalarConversion one and would like to better understand why the

<    1   2   3   4   5   6   7   8   9   10   >