[PATCH] D119040: Fix LookupTest where it was missing an assertion

probinson added a reviewer: kadircet. probinson added a subscriber: kadircet. probinson added a comment. Ping; +@kadircet CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119040/new/ https://reviews.llvm.org/D119040 ___ cfe-commits mailing list

[PATCH] D119040: Fix LookupTest where it was missing an assertion

probinson added a subscriber: bkramer. probinson added a comment. Ping; + @bkramer who reviewed the original patch that added this test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119040/new/ https://reviews.llvm.org/D119040 ___ cfe-commit

[PATCH] D121299: [NVPTX] Disable DWARF .file directory for PTX

probinson added a comment. I'm okay with doing it this way. My impression over the years (as a debug-info guy, not as a ptxas user) is that ptxas evolves slowly and this is not really "busy work." Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121

[PATCH] D114115: [Driver] Support for compressed debug info on Fuchsia

probinson added a comment. Lit is aware of zlib's presence or absence, you could use `REQUIRES: zlib` (or you might factor that bit out into its own test with the REQUIRES). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114115/new/ https://reviews

[PATCH] D129404: Change default C dialect for PS5 to gnu17/gnu18.

probinson accepted this revision. probinson 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/D129404/new/ https://reviews.llvm.org/D129404 _

[PATCH] D130493: Disable stack-sizes section by default for PS4.

probinson accepted this revision. probinson 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/D130493/new/ https://reviews.llvm.org/D130493 _

[PATCH] D126224: Add DWARF string debug to clang release notes.

probinson added a comment. Unnamed variables are an oddity, sure; we've had to patch a downstream test or two that wasn't being careful enough. But not providing a name is entirely defensible, and consumers should be willing to cope with DWARF that doesn't fully meet their expectations. Repo

[PATCH] D126224: Add DWARF string debug to clang release notes.

probinson added a comment. In D126224#3532769 , @dblaikie wrote: > While the string would be deduplicated, the offset in the DIEs (or index in > DWARFv5, plus offset in .debug_str_offsets) for each of these strings would > not. But perhaps that would st

[PATCH] D126224: Add DWARF string debug to clang release notes.

probinson added a comment. I see some unrelated whitespace changes, we generally don't like mixing those with "real" changes. But the description seems fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126224/new/ https://reviews.llvm.org

[PATCH] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

probinson added inline comments. Comment at: test/CodeGenCXX/debug-info-atexit-stub.cpp:14 + +// CHECK: define internal void @"??__Ff@?1??d@@YAPEAU?$c@UBXZ@YAXXZ"() +// CHECK-SAME: !dbg ![[SUBPROGRAM:[0-9]+]] { Do these Windows-mangled names work on Linux?

[PATCH] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

probinson added a comment. I don't see a test for the __cxx_global_array_dtor case? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66328/new/ https://reviews.llvm.org/D66328 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lis

[PATCH] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

probinson added a comment. In D66328#1647094 , @aganea wrote: > In D66328#1647062 , @probinson wrote: > > > I don't see a test for the __cxx_global_array_dtor case? > > > It is actually the `arraydestroy.*` loop tha

[PATCH] D64672: [X86] Prevent passing vectors of __int128 as in llvm IR

probinson added a comment. It looks to me like the patch does things the New Way only for Linux and NetBSD, so for PS4 backward compatibility I am okay with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64672/new/ https://reviews.llvm.org/D64672 _

[PATCH] D36411: Restore previous structure ABI for bitfields with 'packed' attribute for PS4 targets

probinson added a comment. FTR, from a PS4 perspective this is all good, but we'd like somebody from outside our team to take a look. https://reviews.llvm.org/D36411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

probinson created this revision. Make sure all temporary MD nodes have been replaced with uniqued or distinct nodes before we clone a function. Fixes PR33930. https://reviews.llvm.org/D37038 Files: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h lib/CodeGen/CGVTables.cpp test/Co

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

probinson added a comment. I mentioned this in the PR but I should have restated it here, sorry... Wolfgang authored this patch. He is away for a month so I volunteered to post this, in case it was okay for resolving the PR as the crash is present in the 5.0 branch. I do know Wolfgang was not r

[PATCH] D36501: add flag to undo ABI change in r310401

probinson added a comment. Locally we have a couple different tactics for dealing with changes that we can't support. A more coherent approach would be great. For example we defined a new TargetCXXABI::Kind value that is mostly GenericItaniumABI except where it isn't. I personally did not do mo

[PATCH] D86965: Do not emit "-tune-cpu generic" for PS4 platform

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Couple of nits and LGTM. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2078 - // Default to "generic" unless -march is present. + // Default to "generic" unless

[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

probinson added a comment. One thing this patch does, is make decisions about default behavior static. Meaning, the option behavior cannot depend on other options; specifically, it can't be based on the triple, which allows target-specific customization. PS4 certainly has cases where our defa

[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

probinson added a comment. Thanks Duncan! I (or someone) will play around with that and see what we need to do. May take a little while, as we're about to freeze a release and then go on break for two weeks, but good to know there's a straightforward path. Repository: rG LLVM Github Monore

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

probinson created this revision. probinson added reviewers: dblaikie, rnk. probinson added a project: debug-info. probinson requested review of this revision. Herald added a project: clang. Attaches a more appropriate debug location to the PHIs used for the short-circuit evaluations, and makes log

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

probinson added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4293 + { +// There is no need to emit line number for unconditional branch. +auto NL = ApplyDebugLocation::CreateEmpty(CGF); rnk wrote: > I see this is consistent with LAnd h

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

probinson added a comment. In D103125#2781239 , @jhenderson wrote: > In D103125#2780936 , @dblaikie > wrote: > >> Can't say I'm super enthusiastic about this (I assume the build already >> supports prefixes and

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2780997 , @dblaikie wrote: > Looks like GCC emits aliases as a `DW_TAG_variable` without a location, not > as a `DW_TAG_imported_declaration` and marks it external; this works only because gdb will look up the ELF s

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

probinson added a comment. In D103125#2782096 , @echristo wrote: > I'm also not a fan of this change. From a project perspective the binary is > clang and while people may wish to change the name for their own product > teams it seems like that onus sho

[PATCH] D103131: support debug info for alias variable

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4957 + auto Loc = getLineNumber(D->getLocation()); + DBuilder.createGlobalVariableExpression( + DContext, Name, StringRef(), Unit, Loc, Ty, I wasn't saying that gcc did the righ

[PATCH] D103131: support debug info for alias variable

probinson added a comment. > Mixed feelings - somewhat in favor of "do the thing that's probably already > fairly tested/known to work" (GCC's thing). But open to the idea that that > approach has problems, for sure. "Known to work" for whom? gdb, sure, because gcc/gdb cooperate on many things

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2789493 , @probinson wrote: >> Mixed feelings - somewhat in favor of "do the thing that's probably already >> fairly tested/known to work" (GCC's thing). But open to the idea that that >> approach has problems, for

[PATCH] D103131: support debug info for alias variable

probinson added a comment. Of course, if it turns out that gdb can't handle the imported_declaration, we might end up having to do this two different ways under the tuning option. I'd *really* prefer not to do that though, and I'd argue it's a gdb bug if it cannot understand imported_declarati

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2791881 , @dblaikie wrote: > In D103131#2789493 , @probinson > wrote: > >>> Mixed feelings - somewhat in favor of "do the thing that's probably already >>> fairly tested/kno

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2792364 , @dblaikie wrote: > I will say, as the person who implemented DW_TAG_imported_declaration support > in Clang - it's a bit not great/unfortunate (in part because we currently > have to emit them for all usin

[PATCH] D110129: [DebugInfo] Support typedef with btf_tag attributes

probinson added inline comments. Comment at: clang/test/CodeGen/attr-btf_tag-typedef.c:2 +// REQUIRES: x86-registered-target +// RUN: %clang -target x86_64 -g -S -emit-llvm -o - %s | FileCheck %s + Outside of clang/test/Driver, tests should use `%clang_cc1` to ru

[PATCH] D110129: [DebugInfo] Support typedef with btf_tag attributes

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. In D110129#3013946 , @yonghong-song wrote: > - The only thing left is for llvm/test/DebugInfo/X86/attr-btf_tag-typedef.ll > for which I didn't

[PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Seems like a good simplification. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110455/new/ https://reviews.llvm.org/D110455 __

[PATCH] D96274: [clang][cli] Generate and round-trip Diagnostic options

probinson added a comment. I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here. Possibly I am misunderstanding something fundamental

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

probinson created this revision. probinson added reviewers: hoy, wmi. probinson requested review of this revision. Herald added a project: clang. After D93264 , using both -fdebug-info-for-profiling and -fpseudo-probe-for-profiling will cause the compiler to cras

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5ea2d4fa4811: Avoid conflicts between debug-info and pseudo-probe profiling (authored by probinson). Changed prior to commit: https://reviews.llvm

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

probinson added a comment. > the driver had a redundant pass-through of the option I could've sworn it worked to remove that, but it didn't when I rebased, so that's gone from the final patch (i.e, the explicit pass-through in the driver is still there). It's a functionally separate topic anyw

[PATCH] D96838: Add GNU attribute 'retain'

probinson added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:98 +This support is available in GNU ``ld`` and ``gold`` as of binutils 2.36, as +well as in ``ld.lld`` 13. + }]; As a user, this seems like a complicated thing to figure out. Als

[PATCH] D96838: Add GNU attribute 'retain'

probinson added a comment. > For ELF, `used` does not retain the entity, regardless of this patch. That statement does not match my experience. Note that there are two separate variables documented in the LangRef: `llvm.used` (which corresponds to source attribute `used` and is documented as a

[PATCH] D96838: Add GNU attribute 'retain'

probinson added a comment. Aha; attribute `used` *by itself* is not sufficient to preserve sections in the output. But the `__start_/__stop_` symbols implicitly create a reference to each of the named sections, and that implicit reference can preserve them in the output (assuming gc roots etc)

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

probinson added a comment. > GNU ld has a rule "start_/stop_ references from a live input section retain > the associated C identifier name sections", which LLD may change in the future (D96914 ). The phrasing "may change" implies LLD could eliminate the rule; i

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

probinson added a comment. In D97446#2587996 , @MaskRay wrote: > In D97446#2587806 , @probinson wrote: > >>> GNU ld has a rule "start_/stop_ references from a live input section retain >>> the associated C identifi

[PATCH] D74436: Change clang option -ffp-model=precise to select ffp-contract=on

probinson added a comment. I see that the UserManual has been updated, but this behavior change should probably also be mentioned in the Release Notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74436/new/ https://reviews.llvm.org/D74436 _

[PATCH] D97187: [Clang][Sema] Warn when function argument is less aligned than parameter

probinson added a comment. Looks like this caused PR49534 (https://bugs.llvm.org/show_bug.cgi?id=49534) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97187/new/ https://reviews.llvm.org/D97187 ___ cfe-co

[PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

probinson added a comment. > Currently when we have a member function that has an auto return type and the > definition is out of line we generate two DWARF DIE. > One for the declaration and a second one for the definition, the definition > holds the deduced type but does not contain a DW_AT_na

[PATCH] D97566: [WIP][RGT] Rotten Green Test checking for LLVM and Clang unittests

probinson created this revision. probinson added reviewers: MaskRay, dblaikie. Herald added a subscriber: mgrang. probinson requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber: cfe-commits. This is an enhancement of the 'googletest' support code, to rep

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

probinson added a comment. In D97411#2598625 , @akhuang wrote: > I started looking into some diffs of debug info in libc++ tests, but it's > pretty hard to tell what's different - as far as I can see, there are just a > bunch of `__hash_value_type`s and

[PATCH] D98514: [RGT] Fix ASTMatchersTest so all assertions are executed

probinson created this revision. probinson added a reviewer: Prazek. probinson requested review of this revision. Herald added a project: clang. Some 'if' conditions turn out always to be false, so change test expectations to match. Found by the Rotten Green Tests project. Repository: rG LLVM

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

probinson added inline comments. Comment at: llvm/lib/IR/Verifier.cpp:3470 + // (Interposable functions are not inlinable as are functions w/o + // declarations). if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() && (Interposable functions

[PATCH] D136188: Update docs for -fuse-ctor-homing

probinson added a subscriber: jmorse. probinson added a comment. +jmorse who is closer to this topic than I am. We've had a few complaints from licensees about ctor homing, and debug-info size in general is something of a sensitive topic. But if we can come to a place where `-fstandalone-debug

[PATCH] D136187: [clang][AIX] Omitting Explicit Debugger Tuning Option

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4374 + ? llvm::DebuggerKind::Default + : DebuggerTuning); Seems like you should be able to return `DebuggerKind::Default

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

probinson added inline comments. Comment at: clang/test/Driver/as-options.s:121 +// Test that -g is passed through to GAS. +// RUN: %clang -fno-integrated-as -g %s -### 2>&1 | \ +// RUN: FileCheck --check-prefix=CHECK-DEBUG %s Please use an explicit triple here

[PATCH] D133998: [HIP][test] Avoid %T

probinson added subscribers: lamb-j, probinson. probinson added a comment. @MaskRay I see that you restored the `clang-driver` keyword in hip-link-bc-to-bc.hip, and of course that keyword is not defined anywhere so the test is never being run. Is there an issue filed to track this? Or is the or

[PATCH] D133998: [HIP][test] Avoid %T

probinson added a comment. I filed #58711 to get the test fixed; in the meantime, removed `clang-driver` and added `XFAIL: *` in rG7af01fe4 Repository: rG LLVM Github Mon

[PATCH] D128934: [X86] Add RDPRU instruction

probinson added a comment. In D128934#3630800 , @probinson wrote: > In D128934#3629653 , @pengfei wrote: > >> > > Done, thanks for the reminder! > >>> 2. Update `clang/lib/Headers/cpuid.h` and `llvm/lib/Support/H

[PATCH] D131919: Move googletest to the third-party directory

probinson added inline comments. Comment at: clang/CMakeLists.txt:106 AND EXISTS ${UNITTEST_DIR}/CMakeLists.txt) add_subdirectory(${UNITTEST_DIR} utils/unittest) endif() Should this be `third-party/unittest` ? Looking at how the lldb cm

[PATCH] D131919: Move googletest to the third-party directory

probinson added a comment. In D131919#3749850 , @Meinersbur wrote: > Semi-OT: `polly\lib\External` has 3 more third-party libraries. Two of them > have been heavily modified in-tree, the third has just a custom > CMakeLists.txt. > Should these eventual

[PATCH] D134507: [Clang] add missing ClangABICompat check

probinson added a comment. Anytime there's a Clang ABI version check, I have to wonder whether PS4/PS5 need an exception. (And I'm sorry I missed the earlier patches in this series.) We are pretty much locked to however things worked for Clang 3.2. Is this specific to C++20 (or later)? In th

[PATCH] D134507: [Clang] add missing ClangABICompat check

probinson added a comment. Hi @ychen I saw you updated the patch description, thanks. IIUC, the change causes some difference in the interpretation of particular template stuff. Would this be clearer if the BEFORE-15/AFTER-15 test cases used the same template construct? Currently they are diff

[PATCH] D134544: [clang-cl] Implement /ZH: flag

probinson added a project: debug-info. probinson added a comment. Probably we should reject the -gsrc... option if we're emitting DWARF, which requires MD5 (at least so far). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134544/new/ https://reviews.llvm.org/D134544 ___

[PATCH] D98438: Clang: Allow selecting the hash algorithm for file checksums in debug info.

probinson added a comment. So this one can be marked abandoned? You might need to commandeer it first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98438/new/ https://reviews.llvm.org/D98438 ___ cfe-co

[PATCH] D134468: [Driver] Ignore -fmsc-version= -fms-compatibility-version= values smaller than 19

probinson added a comment. These kinds of compatibility changes generally should get a Discourse post for better visibility. Comment at: clang/docs/ReleaseNotes.rst:85 + a version smaller than ``19.0`` is now unsupported. + Or words to that effect. "19.0"

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

probinson added a comment. > The number of people likely to notice that PS4 is different from the other > targets seems far smaller than the number of people noticing the same for > Windows. But I've got no data to back that up, either. heh. There's a PS4 bot that notices. But the PS4 situati

[PATCH] D134544: [clang-cl] Implement /ZH: flag

probinson added inline comments. Comment at: clang/test/CodeGen/debug-info-file-checksum.c:3 +// RUN: %S/Inputs/debug-info-file-checksum.c -o - | FileCheck %s +// RUN: %clang -emit-llvm -S -g -gcodeview -gsrc-hash-algorithm=md5 -x c \ +// RUN: %S/Inputs/debug-info-file-ch

[PATCH] D134673: [Driver][PS4] pass -fcrash-diagnostics-dir to LTO

probinson added a comment. I'm guessing the non-PS4 targets would also want this, so a similar change is needed in tools::addLTOOptions. This can be a separate patch if you wish. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134673/new/ https://r

[PATCH] D134507: [Clang] add missing ClangABICompat check

probinson added a subscriber: rjmccall. probinson added a comment. It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise. @aaron.ballman or @rjmccall would probably be the right people to weigh in on this. Repository:

[PATCH] D134673: [Driver][PS4] pass -fcrash-diagnostics-dir to LTO

probinson accepted this revision. probinson 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/D134673/new/ https://reviews.llvm.org/D134673 _

[PATCH] D134507: [Clang] add missing ClangABICompat check

probinson added a comment. In D134507#3818765 , @ychen wrote: > In D134507#3817928 , @probinson > wrote: > >> It feels odd to use a ClangABI check for something that is affecting what >> source is accepted, but

[PATCH] D134880: [clang] [test] Use %clang_cc1 substitution consistently

probinson added inline comments. Comment at: clang/test/Driver/lit.local.cfg:8 -('%clang_cc1', - """*** Do not use 'clang -cc1' in Driver tests. ***""") ) But we _don't_ want to use `%clang_cc1` in Driver tests; if we do, then we're not testing the dri

[PATCH] D111000: [clang-format] allow clang-format to be passed a file of filenames so we can add a regression suite of "clean clang-formatted files" from LLVM

probinson added a comment. In D111000#3822354 , @ychen wrote: > It is possible to just use `clang-format @response.txt`? That should also > invoke the executable once. It's true that whatever file you're handing to `--files` could just as easily be spe

[PATCH] D111000: [clang-format] allow clang-format to be passed a file of filenames so we can add a regression suite of "clean clang-formatted files" from LLVM

probinson added a comment. Response files should work for all our tools, I think? Keeping the option is fine with me, but the description needs some improvement. @ychen Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111000/new/ https://reviews.ll

[PATCH] D135115: [clang-format] update --files help description

probinson added a comment. The option handling in clang-format itself would need to be updated as well. Comment at: clang/docs/ClangFormat.rst:29 USAGE: clang-format [options] [ ...] MyDeveloperDay wrote: > if response files are so common place why not s

[PATCH] D135115: [clang-format] update --files help description

probinson added inline comments. Comment at: clang/docs/ClangFormat.rst:72-73 Used only with --dry-run or -n ---files= - Provide a list of files to run clang-format +--files= - Accept a list of response fi

[PATCH] D135115: [clang-format] update --files help description

probinson added a comment. The help text in ClangFormat.cpp for the `--files` option needs to be updated the same way. Note using `cl::value_desc("filename")` is what you need to change the meta-variable in the help output. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D135115: [clang-format] update --files help description

probinson added a comment. In D135115#3839547 , @HazardyKnusperkeks wrote: > It's too long ago for me. Does the `--files` option take multiple file names > on the command line, or a file containing the file names? If the latter the > patch generally lo

[PATCH] D135488: [codegen][WIP] Display stack layouts in console

probinson added inline comments. Comment at: clang/test/Frontend/frame-diags.c:1 +// TODO: This is not really a functional test yet, and needs to be rewritten. +// currently its just a copy of backend-diagnostics.c, and all of the run/test I know this is a draf

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

probinson added a comment. dblaikie: > In the same way that an assert says "This condition should never be false" - > I use "should" in the same sense in both unreachable and assert, and I > believe that's the prevailing opinion of LLVM developers/the LLVM style guide. aaronballman: > I belie

[PATCH] D135097: Remove redundant option '-menable-unsafe-fp-math'.

probinson added subscribers: wristow, probinson. probinson added a comment. +wristow who has been tracking this kind of stuff for Sony. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135097/new/ https://reviews.llvm.org/D135097 ___ cfe-commits

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

probinson added a comment. Herald added subscribers: jsetoain, JDevlieghere, ormris. Worth trying on MSVC, which is the other more likely place to fail. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140332/new/ https://reviews.llvm.org/D140332 ___

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

probinson added a comment. This causes codegen to be different depending on whether debug-info is generated. Please don't do that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141381/new/ https://reviews.llvm.org/D141381

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

probinson added a comment. I should expand on my previous comment. Extending the lifetime of these variables by saving the pointers into local variables, that's fine; just make sure it's not conditional on -g. The only difference between -g and not -g should be metadata and dbg.* instructions

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

probinson added a subscriber: wolfgangp. probinson added a comment. Basically you're homing a pointer to the parameter, rather than the parameter itself, which makes sense in this context. I suspect the code size/perf impact on -O0 will not be huge, as this sounds like it's only for byval-but-n

[PATCH] D137287: [Test] Fix CHECK typo.

probinson added a comment. Nice work! have you verified that all the modified tests pass? naively it looks like you'd need at least 3 different targets to run them all (windows, webassembly, powerpc) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1

[PATCH] D131919: Move googletest to the third-party directory

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

[PATCH] D137287: [Test] Fix CHECK typo.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM, but you'll want to be ready to jump on any bot failures. That's just the nature of these kinds of changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D137437: [lit][AIX] Convert clang tests to use 'target={{.*}}aix{{.*}}'

probinson edited subscribers, added: cfe-commits; removed: clang. probinson added a comment. Try again to add the commits list. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137437/new/ https://reviews.llvm.org/D137437 ___ cfe-commits mailing

[PATCH] D137437: [lit][AIX] Convert clang tests to use 'target={{.*}}aix{{.*}}'

probinson added inline comments. Comment at: clang/test/ClangScanDeps/modules-no-undeclared-includes.c:3 // section in XCOFF yet. -// UNSUPPORTED: aix +// UNSUPPORTED: target={{.*}}aix{{.*}} hubert.reinterpretcast wrote: > Perhaps we should normalize on `-aix`

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596 + DefaultedSMFArePOD = false; + } + Does this mean `-fclang-abi-compat` will override the PS4/Darwin special case? I think we don't want to do that. Repository: rG

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596 + DefaultedSMFArePOD = false; + } + dblaikie wrote: > probinson wrote: > > Does this mean `-fclang-abi-compat` will override the PS4/Darwin special > > case? I think w

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596 + DefaultedSMFArePOD = false; + } + dblaikie wrote: > probinson wrote: > > dblaikie wrote: > > > probinson wrote: > > > > Does this mean `-fclang-abi-compat` will overri

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added a comment. I'm in the middle of upstreaming PS5 support and I still didn't think of this... doh! Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5588 + bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin(); + `isPS4()` =>

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

probinson added a comment. Hi @rsmith, > these two different templates would have the same mangling: template T returnit() {return I;}; template T returnit() { return I; } I tried compiling `long foo() { return returnit(); }` with these two templates, and got different manglings. `_Z8re

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

probinson added a comment. > You need to do `returnit`. Doh! Thanks. Those two instantiations could have different function bodies, but would have the same mangled name. Got it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147655/new/ https://re

[PATCH] D155081: Specify the developer policy around links to external resources

probinson added inline comments. Comment at: llvm/docs/DeveloperPolicy.rst:359 + If the patch fixes a bug in GitHub Issues, we encourage adding + "Fixes https://github.com/llvm/llvm-project/issues/12345"; to automate closing + the issue in GitHub. If the patch has been reviewe

[PATCH] D155859: [Headers][doc] Add misc non-AVX2 intrinsic descriptions

probinson created this revision. probinson added reviewers: pengfei, RKSimon, goldstein.w.n, craig.topper. Herald added a project: All. probinson requested review of this revision. Adds descriptions for adxintrin.h, bmi2intrin.h, clflushoptintrin.h, rdseedintrin.h, and xsavecintrin.h. Revises clze

[PATCH] D155861: [Headers][doc] Add SHA1/SHA256 intrinsic descriptions

probinson created this revision. probinson added reviewers: pengfei, RKSimon, goldstein.w.n, craig.topper. Herald added a project: All. probinson requested review of this revision. I didn't include pseudo-code, because it would be long and complicated, probably not tell the whole story, and these

[PATCH] D155991: [DWARF] Make sure file entry for artificial functions has an MD5 checksum

probinson created this revision. probinson added reviewers: dblaikie, aprantl. probinson added a project: debug-info. Herald added a project: All. probinson requested review of this revision. The DIFile cache was keyed on a string pointer instead of string content, which was causing misses and res

[PATCH] D155991: [DWARF] Make sure file entry for artificial functions has an MD5 checksum

probinson added a comment. The fix is straightforward but the test was surprisingly tricky; I had to add the -main-file-name option to keep one of the DIFile's from ending up as ``. I'm still befuddled about why there are two DIFile entries, when the DIFileCache clearly has only one. The metada

[PATCH] D155861: [Headers][doc] Add SHA1/SHA256 intrinsic descriptions

probinson added a comment. Thanks! The pre-merge check caught mistakes in the \param commands, which I fixed before pushing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155861/new/ https://reviews.llvm.org/D155861 ___ cfe-commits mailing

<    1   2   3   4   5   6   >