[PATCH] D153600: Implement -frecord-command-line for XCOFF

scott.linder accepted this revision. scott.linder added a comment. This revision is now accepted and ready to land. Thank you for the patch and the changes! This LGTM, and AFAICT all of the comments have been addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D153600: Implement -frecord-command-line for XCOFF

scott.linder added inline comments. Comment at: llvm/lib/MC/MCAsmStreamer.cpp:993 + // Metadata needs to be padded out to an even word size. + uint32_t PaddedSize = alignTo(std::max((int)MetadataSize, 1), 4); + uint32_t PaddingSize = PaddedSize - MetadataSize;

[PATCH] D153600: Implement -frecord-command-line for XCOFF

scott.linder added a comment. Sorry for the delay in review, I am not too familiar with XCOFF so I was hoping someone else would take a look first. If my understanding of the COFF docs I could find is correct then this LGTM save for some nits/suggestions Comment at:

[PATCH] D146023: [AMDGPU] Remove Code Object V2

scott.linder added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:520 - assert(Func.getCallingConv() == CallingConv::AMDGPU_KERNEL || - Func.getCallingConv() == CallingConv::SPIR_KERNEL); + if (Func.getCallingConv() !=

[PATCH] D146023: [AMDGPU] Remove Code Object V2

scott.linder added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:520 - assert(Func.getCallingConv() == CallingConv::AMDGPU_KERNEL || - Func.getCallingConv() == CallingConv::SPIR_KERNEL); + if (Func.getCallingConv() !=

[PATCH] D150282: [Driver] -ftime-trace: derive trace file names from -o and -dumpdir

scott.linder added inline comments. Comment at: clang/include/clang/Driver/Compilation.h:279 + void addTimeTraceFile(const char *Name, const JobAction *JA) { +TimeTraceFiles[JA] = Name; + } MaskRay wrote: > scott.linder wrote: > > If this is overriding

[PATCH] D149986: AMDGPU: Force sc0 and sc1 on stores for gfx940 and gfx941

scott.linder added inline comments. Comment at: llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp:517-529 + bool tryForceStoreSC0SC1(const SIMemOpInfo , + MachineBasicBlock::iterator ) const override { +if (ST.hasForceStoreSC0SC1() && +

[PATCH] D150282: [Driver] -ftime-trace: derive trace file names from -o and -dumpdir

scott.linder added a comment. Does GCC have the same `-ftime-trace=` option? It seems like it doesn't, as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92396 is still open? If so, I am happy with unifying more of the dump/aux handling, and I imagine when/if GCC adds the option it will behave

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

scott.linder added a comment. In D149193#4316293 , @dblaikie wrote: > I guess my main question is: What's the motivation for implementing this? Do > you have a need/use for this? (it doesn't seem to be motivated by GCC > compatibility - as discussed,

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

scott.linder added a comment. I am OK to give LGTM, assuming the other reviewers don't still have reservations? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149193/new/ https://reviews.llvm.org/D149193

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

scott.linder added a comment. In D149193#4300885 , @MaskRay wrote: > I think the patch as-is implements all the useful parts of GCC's complex > rules and in the absence of `-dumpbase` (we deliberately don't implement), > the rule almost exactly matches

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

scott.linder added inline comments. Comment at: clang/lib/Driver/Driver.cpp:3884 + nullptr, getOpts().getOption(options::OPT_dumpdir), + Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-")); + Arg->claim(); MaskRay wrote: >

[PATCH] D149193: [Driver] -gsplit-dwarf: derive .dwo names from -o for link actions

scott.linder added a comment. I'm a bit confused after trying to work out the rules for the GCC version of `-dumpdir` over at https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-dumpdir but it at least seems like our version is a subset of theirs. Do we support any of the other

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

scott.linder accepted this revision. scott.linder added a comment. This revision is now accepted and ready to land. LGTM, thank you! Does this warrant a release note, as it is changing the behavior in a backwards-incompatible manner? I do think changing to match GCC is worthwhile, even if it

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

scott.linder added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:209 - std::map DebugPrefixMap; + llvm::SmallVector, 0> DebugPrefixMap; std::map CoveragePrefixMap; MaskRay wrote: > scott.linder wrote: > > What benefit does

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

scott.linder added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:209 - std::map DebugPrefixMap; + llvm::SmallVector, 0> DebugPrefixMap; std::map CoveragePrefixMap; What benefit does forcing allocation have? Why not use the

[PATCH] D147279: [HeterogeneousDWARF] Implement AMDGPU CFI, DebugInfo

scott.linder created this revision. Herald added subscribers: kosarev, foad, kerbowa, arphaman, hiraditya, tpr, dstuttard, yaxunl, jvesely, kzhuravl, emaste, arsenm, qcolombet, MatzeB. Herald added a project: All. scott.linder requested review of this revision. Herald added subscribers:

[PATCH] D145770: [clang-offload-bundler] Standardize TargetID field for bundler

scott.linder added a comment. Is there a corresponding document update to go along with this? If consumers of the bundles will begin to rely on this it should be documented that it is guaranteed by the producer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

This revision was automatically updated to reflect the committed changes. Closed by commit rG97ba3c2bec48: [Clang][AMDGPU] Set LTO CG opt level based on Clang option (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

scott.linder added a comment. Are there any thoughts on whether this is too ugly to live? It will be awkward to teach users the current default behavior without this change, but if we can accept it as a historical quirk that may be OK. The primary driver for wanting the 1:1 mapping is an odd

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

scott.linder updated this revision to Diff 494435. scott.linder added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142499/new/ https://reviews.llvm.org/D142499 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

scott.linder updated this revision to Diff 492205. scott.linder added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142499/new/ https://reviews.llvm.org/D142499 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

scott.linder added a comment. In D142499#4078177 , @arsenm wrote: > Making this target specific doesn’t make sense I don't know how else to resolve others wanting the default to be the `CGOptLevel = clamp(ltoOptLevel, 2, 3)` behavior and AMDGPU wanting

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

scott.linder added reviewers: arsenm, yaxunl, scchan, pcc, MaskRay, mehdi_amini, respindola, ruiu, aheejin, sbc100. scott.linder added a comment. (Just adding everyone from the parent review, feel free to remove yourself as reviewer if you aren't interested!) It isn't ideal, but as only AMDGCN

[PATCH] D142499: [Clang][AMDGPU] Set LTO CG opt level based on Clang option

scott.linder created this revision. Herald added subscribers: kosarev, inglorion, tpr, dstuttard, yaxunl, kzhuravl. Herald added a project: All. scott.linder requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, wdng. Herald added a project: clang. For AMDGCN default

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder added inline comments. Comment at: flang/lib/Frontend/FrontendActions.cpp:590 + std::optional OptLevelOrNone = + CodeGenOpt::getLevel(CGOpts.OptimizationLevel); + assert(OptLevelOrNone && "Invalid optimization level!"); aaronpuchert wrote: >

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder added a comment. I also ended up changing the underlying type to `int` to resolve https://lab.llvm.org/buildbot/#/builders/61/builds/38754 , and I removed the `getID` function (with the implicit conversions it would never get used anyway). Repository: rG LLVM Github Monorepo

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder added inline comments. Comment at: llvm/include/llvm/Support/CodeGen.h:67 + inline std::optional getLevel(IDType ID) { +if (ID < 0 || ID > 3) + return std::nullopt; mstorsjo wrote: > scott.linder wrote: > > barannikov88 wrote: > > > As I

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG25c0ea2a5370: [NFC] Consolidate llvm::CodeGenOpt::Level handling (authored by scott.linder). Changed prior to commit:

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder added inline comments. Comment at: llvm/include/llvm/Support/CodeGen.h:57 + /// Code generation optimization level. + enum Level : IDType { +None = 0, ///< -O0 barannikov88 wrote: > arsenm wrote: > > scott.linder wrote: > > > This is ABI

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder updated this revision to Diff 490321. scott.linder marked 2 inline comments as done. scott.linder added a comment. - Fix return type of `getID` - Fix mistakenly updated option help text - Add back in missing `assert`s Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder added inline comments. Comment at: llvm/include/llvm/Support/CodeGen.h:57 + /// Code generation optimization level. + enum Level : IDType { +None = 0, ///< -O0 This is ABI breaking, so maybe we don't want/need to define the underlying

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder added a comment. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. I wasn't sure whether to include `getID` and unit-test it, or just drop it until it is used. If anyone has a preference let me know and I'll update the review. Repository: rG LLVM Github

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

scott.linder created this revision. Herald added subscribers: ormris, steven_wu, hiraditya. Herald added projects: Flang, All. scott.linder requested review of this revision. Herald added subscribers: llvm-commits, openmp-commits, cfe-commits, jdoerfert. Herald added projects: clang, OpenMP, LLVM.

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder abandoned this revision. scott.linder added a comment. I'll open a new review as part of upstreaming all of the debug info work Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88978/new/ https://reviews.llvm.org/D88978

[PATCH] D131717: [ADT] Replace STLForwardCompat.h's C++17 equivalents

scott.linder accepted this revision. scott.linder added a comment. LGTM, thank you! I have some pending changes which would add C++20 "compat" types, but removing it until those land sounds fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D123878: [AMDGPU] Add remarks to output some resource usage

scott.linder added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1207 + auto EmitResourceUsageRemark = [&](StringRef RemarkName, + StringRef RemarkLabel, auto &) { +// Add an indent for every line besides the

[PATCH] D123878: [AMDGPU] Add remarks to output some resource usage

scott.linder added a comment. I will let others comment, but I think this is a perfectly reasonable alternative, and I much prefer using indentation over the delimiter-remark approach. LGTM, assuming nobody else objects Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127923: [Diagnostics] Accept newline and format diag opts on first line

scott.linder added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:99-103 + size_t NewlinePos = PD->getShortDescription().find_first_of('\n'); + if (NewlinePos != std::string::npos) +OutStr =

[PATCH] D127923: [Diagnostics] Accept newline and format diag opts on first line

scott.linder added a comment. Thank you for the patch! I will leave it to others with more knowledge of the history of diagnostics to comment on whether the premise is acceptable, but I have a few technical comments Comment at:

[PATCH] D123878: [AMDGPU] Add remarks to output some resource usage

scott.linder added a comment. In D123878#3507378 , @vangthao wrote: > I am not sure if allowing clang to accept newlines is a good idea. It seems > like clang wants to know what type of message is being outputted. For example > whether this is a

[PATCH] D123878: [AMDGPU] Add remarks to output some resource usage

scott.linder added a comment. In D123878#3506500 , @afanfa wrote: > If possible, I would like to keep some kind of delimiter. I like the idea of > having it at the beginning and at the end of the section. The best option > would be to convince clang to

[PATCH] D123878: [AMDGPU] Add remarks to output some resource usage

scott.linder added a comment. Even with newlines forced via extra remarks, I'm not a big fan of the "---" remark; it doesn't interact well with other random remarks in the output, for example when I enable all remarks using the pattern '.*' I see: remark: foo.cl:27:0:

[PATCH] D123878: [AMDGPU] Add remarks to output some resource usage

scott.linder added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1178-1266 + +void AMDGPUAsmPrinter::emitResourceUsageRemarks( +const MachineFunction , const SIProgramInfo , +bool isModuleEntryFunction, bool hasMAIInsts) { + if (!ORE) +

[PATCH] D121951: [AMDGPU][OpenCL] Remove "printf and hostcall" diagnostic

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG09f33a430b72: [AMDGPU][OpenCL] Remove printf and hostcall diagnostic (authored by scott.linder). Changed prior to commit:

[PATCH] D121951: [AMDGPU][OpenCL] Remove "printf and hostcall" diagnostic

scott.linder added a comment. In D121951#3414271 , @sameerds wrote: > In D121951#3411856 , @scott.linder > wrote: > >> @yaxunl Does excluding device-libs via COV_None make sense? >> >> @sameerds Would you still

[PATCH] D121951: [AMDGPU][OpenCL] Remove "printf and hostcall" diagnostic

scott.linder updated this revision to Diff 418922. scott.linder retitled this revision from "[AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5" to "[AMDGPU][OpenCL] Remove "printf and hostcall" diagnostic". scott.linder edited the summary of this revision. scott.linder

[PATCH] D122669: [AMDGPU][OpenCL] Remove "printf and hostcall" diagnostic

scott.linder created this revision. Herald added subscribers: hsmhsm, foad, Naghasan, ldrumm, kerbowa, hiraditya, t-tye, Anastasia, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl, arsenm. Herald added a project: All. scott.linder requested review of this revision. Herald added subscribers:

[PATCH] D121951: [AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5

scott.linder added a comment. @yaxunl Does excluding device-libs via COV_None make sense? @sameerds Would you still rather we just not add this attribute in the frontend at all? I'm OK with it if that is the consensus Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D121951: [AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5

scott.linder updated this revision to Diff 418635. scott.linder added a comment. Do not add the no-hostcall attribute for "COV_None" modules, to allow for the OpenCL implementation of hostcall in the device-libs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D121951: [AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5

scott.linder added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9381 + M.getTarget().getTargetOpts().CodeObjectVersion != 500) { +F->addFnAttr("amdgpu-no-hostcall-ptr"); + } arsenm wrote: > sameerds wrote: > > The frontend does not

[PATCH] D121951: [AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5

scott.linder updated this revision to Diff 418135. scott.linder retitled this revision from "[AMDGPU] Only warn when mixing printf and hostcall" to "[AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5". scott.linder edited the summary of this revision. scott.linder added a

[PATCH] D121951: [AMDGPU] Only warn when mixing printf and hostcall

scott.linder added a comment. In D121951#3393928 , @sameerds wrote: > In D121951#3392470 , @scott.linder > wrote: > >> In D121951#3391472 , @sameerds >> wrote: >> >>>

[PATCH] D121951: [AMDGPU] Only warn when mixing printf and hostcall

scott.linder added a comment. In D121951#3391472 , @sameerds wrote: > The check for "__ockl_hostcall_internal" is not longer relevant with the new > hostcall attribute. Can we simply remove this check? What is the situation > where the warning is still

[PATCH] D121951: [AMDGPU] Only warn when mixing printf and hostcall

scott.linder added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:567 - if (auto HostcallFunction = M.getFunction("__ockl_hostcall_internal")) { + if (auto *HostcallFunction = M.getFunction("__ockl_hostcall_internal")) { for (auto :

[PATCH] D121951: [AMDGPU] Only warn when mixing printf and hostcall

scott.linder added reviewers: sameerds, arsenm, rampitec, b-sumner, kzhuravl, yaxunl. scott.linder added a comment. For reference, the error I'm changing to a warning was added as part of https://reviews.llvm.org/D70038, and the issue that made me consider this approach appears when building

[PATCH] D121951: [AMDGPU] Only warn when mixing printf and hostcall

scott.linder created this revision. Herald added subscribers: foad, kerbowa, hiraditya, t-tye, Anastasia, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl, arsenm. Herald added a project: All. scott.linder requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits,

[PATCH] D110276: Clean up large copies of binaries copied into temp directories in tests

scott.linder added a comment. Is it possible to use soft links instead of copies? It would still be good to clean up afterwards, but if the host won't allow that cleanup in some cases at least we aren't leaving a big file around. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D107190: [AMDGPU][HIP] Switch default DWARF version to 5

This revision was automatically updated to reflect the committed changes. Closed by commit rG635c5ba45bae: [AMDGPU][HIP] Switch default DWARF version to 5 (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107190/new/

[PATCH] D107190: [AMDGPU][HIP] Switch default DWARF version to 5

scott.linder created this revision. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. scott.linder requested review of this revision. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. Another attempt at changing this default,

[PATCH] D106734: Eliminate clang man page generation warning for missing .rst files

scott.linder accepted this revision. scott.linder added a comment. This revision is now accepted and ready to land. LGTM, I see no reason why this shouldn't apply for all generators. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106734/new/

[PATCH] D100671: [ADT] Factor out in_place_t and expose in Optional ctor

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGaf5247c9347d: [ADT] Factor out in_place_t and expose in Optional ctor (authored by scott.linder). Changed prior to commit:

[PATCH] D100671: [ADT] Factor out in_place_t and expose in Optional ctor

scott.linder updated this revision to Diff 345475. scott.linder added a comment. Add constexpr convenience variables Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100671/new/ https://reviews.llvm.org/D100671 Files:

[PATCH] D101542: [NFC] Refactor ExecuteAssembler in cc1as_main.cpp

This revision was automatically updated to reflect the committed changes. Closed by commit rGcab19d84ce85: [NFC] Refactor ExecuteAssembler in cc1as_main.cpp (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101542/new/

[PATCH] D101542: [NFC] Refactor ExecuteAssembler in cc1as_main.cpp

scott.linder added reviewers: MaskRay, dineshkb-amd, aykevl. scott.linder added a comment. I'm not sure who else to add, please feel free to update the reviewers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101542/new/

[PATCH] D101542: [NFC] Refactor ExecuteAssembler in cc1as_main.cpp

scott.linder created this revision. scott.linder requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Introduce an extra scope (another static function) to replace calls to `unique_ptr::reset` with implicit destructors via RAII. Repository:

[PATCH] D100671: [ADT] Factor out in_place_t and expose in Optional ctor

scott.linder added a comment. In D100671#2695923 , @dblaikie wrote: > This usage doesn't seem to quite match the standard - which provides an > existing instance of in_place_t for callers to use: > > std::optional o4(std::in_place, {'a', 'b', 'c'}); >

[PATCH] D100671: [ADT] Factor out in_place_t and expose in Optional ctor

scott.linder created this revision. Herald added a subscriber: dexonsmith. scott.linder requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D100671 Files:

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder added a comment. In D88978#2660274 , @arsenm wrote: > Is this still needed? Yes, I just got a little bogged down in the OMP code and haven't gotten back to it to finish it up. I anticipate needing to do this to soon, though. Repository:

[PATCH] D94338: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs

This revision was automatically updated to reflect the committed changes. Closed by commit rGc15b0e2229ea: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94338/new/

[PATCH] D94338: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs

scott.linder created this revision. scott.linder requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D94338 Files: clang/docs/ClangOffloadBundler.rst Index:

[PATCH] D93721: [NFC][AMDGPU] Do not override GetDefaultDwarfVersion

scott.linder added reviewers: t-tye, laurentm0, yaxunl. scott.linder added a comment. No other target overrides `GetDefaultDwarfVersion` with the same implementation, and in the future when we make changes we can add them at the points in the hierarchy where they make sense semantically.

[PATCH] D93721: [NFC][AMDGPU] Do not override GetDefaultDwarfVersion

scott.linder created this revision. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. scott.linder requested review of this revision. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. Use the implementation inhereted from

[PATCH] D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5"

This revision was automatically updated to reflect the committed changes. Closed by commit rGffba47df7646: Revert [AMDGPU][HIP] Switch default DWARF version to 5 (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93648/new/

[PATCH] D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5"

scott.linder created this revision. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. scott.linder requested review of this revision. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. This reverts commit

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

scott.linder added inline comments. Comment at: clang/test/Driver/debug-options.c:364-366 +// GEMBED_2: warning: debug information option '-gembed-source' is not supported for target // NOGEMBED_5-NOT: "-gembed-source" +// NOGEMBED_2-NOT: warning: debug information option

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

scott.linder added inline comments. Comment at: clang/test/Driver/debug-options.c:364-366 +// GEMBED_2: warning: debug information option '-gembed-source' is not supported for target // NOGEMBED_5-NOT: "-gembed-source" +// NOGEMBED_2-NOT: warning: debug information option

[PATCH] D90419: [AMDGPU] Add gfx90c target

scott.linder added inline comments. Comment at: llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml:161 # RUN: yaml2obj --docnum=41 %s > %t.o.41 # RUN: llvm-readobj -s -file-headers %t.o.41 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX1031 %s Heads up, I just

[PATCH] D89484: [AMDGPU][HIP] Switch default DWARF version to 5

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc4d10e7e9bb4: [AMDGPU][HIP] Switch default DWARF version to 5 (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D89484: [AMDGPU][HIP] Switch default DWARF version to 5

scott.linder created this revision. Herald added subscribers: cfe-commits, kerbowa, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. Herald added a project: clang. scott.linder requested review of this revision. Herald added a subscriber: wdng. Another attempt at this, see D59008

[PATCH] D88976: [clang] Use correct address space for global variable debug info

scott.linder added inline comments. Comment at: clang/lib/Basic/Targets/NVPTX.h:47 -1, // Default, opencl_private or opencl_generic - not defined -5, // opencl_global +-1, // opencl_global -1, Does anyone have any thoughts on this change

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder added a comment. In D88978#2325991 , @ABataev wrote: > In D88978#2325982 , @scott.linder > wrote: > >> @ABataev Sorry if I'm pulling you in without enough context/work on my end, >> but I wanted to

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder added a comment. @ABataev Sorry if I'm pulling you in without enough context/work on my end, but I wanted to ask how the Clang codegen for OpenMP locals works at a high level? Is the idea that instead of an `alloc` the frontend can insert calls into the runtime in some cases, like

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1579 if (EmitDebugInfo && HaveInsertPoint()) { -Address DebugAddr = address; +Address DebugAddr = AllocaAddr.isValid() ? AllocaAddr : address; bool UsePointerValue = NRVO &&

[PATCH] D88976: [clang] Use correct address space for global variable debug info

scott.linder added inline comments. Comment at: clang/test/CodeGenHIP/debug-info-address-class.hip:8 + +// CHECK-DAG: ![[FILEVAR0:[0-9]+]] = distinct !DIGlobalVariable(name: "FileVar0", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}}, isLocal: false,

[PATCH] D88976: [clang] Use correct address space for global variable debug info

scott.linder updated this revision to Diff 297309. scott.linder added a comment. Replace uses of CHECK-DAG, use more meaningful names in test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88976/new/ https://reviews.llvm.org/D88976 Files:

[PATCH] D88754: [clang] Add a test for CGDebugInfo treatment of blocks

This revision was automatically updated to reflect the committed changes. Closed by commit rG40cef5a00eb8: [clang] Add a test for CGDebugInfo treatment of blocks (authored by scott.linder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88754/new/

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder added a comment. I need to add more tests, but I wanted to float the idea of the change and get feedback first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88978/new/ https://reviews.llvm.org/D88978

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

scott.linder created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. scott.linder requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. A dbg.declare for a local/parameter describes the hardware location

[PATCH] D88976: [clang] Use correct address space for global variable debug info

scott.linder added a comment. I'm not certain I fully understand NVPTX's relationship with its debugger, but from https://reviews.llvm.org/D57162 I gather the "default" address space in the debugger is global, and so the frontend omits it rather than explicitly mentioning it. I think it would

[PATCH] D88976: [clang] Use correct address space for global variable debug info

scott.linder created this revision. Herald added subscribers: cfe-commits, jholewinski. Herald added a project: clang. scott.linder requested review of this revision. The target needs to be queried here, but previously we seemed to only duplicate CUDA's (and so HIP's) behavior, and only

[PATCH] D88754: [clang] Add a test for CGDebugInfo treatment of blocks

scott.linder updated this revision to Diff 296558. scott.linder added a comment. Don't hardcode x86_64-specific offsets Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88754/new/ https://reviews.llvm.org/D88754 Files:

[PATCH] D88754: [clang] Add a test for CGDebugInfo treatment of blocks

scott.linder created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. scott.linder requested review of this revision. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D88754 Files: clang/test/CodeGen/debug-info-block-expr.c Index:

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

scott.linder accepted this revision. scott.linder added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: llvm/cmake/modules/AddLLVM.cmake:1916 endif() - if(EXISTS "${path}/.svn") -set(svn_files Nit: can this be in a

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

scott.linder added inline comments. Comment at: llvm/cmake/modules/AddLLVM.cmake:1945 + if (NOT touch_head_result EQUAL 0) +return() + endif() This seems to implement the behavior that when the log is not present and is not

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

scott.linder added a comment. I'd be interested in the answer concerning why we need to avoid `git rev-parse HEAD`; it seems like the cleanest solution is to just always check if `git rev-parse HEAD` changes to determine whether to regenerate the header. If that is not feasible for some

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

scott.linder added a comment. Thank you for the explanation; I'm also still lost as to why `.git/logs/HEAD` is referenced at all then. I would propose removing `find_first_existing_vc_file` entirely, as it seems to be completely redundant if there is another mechanism for ensuring the VCS

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

scott.linder added a comment. Can you provide more context in the diff? Looking at the current `master` locally it seems like this patch changes the behavior of the `llvm_vcsrevision_h` target when `logs/HEAD` isn't found, such that it only depends on the generation script. I.e. if you

[PATCH] D77923: OpenCL: Fix some missing predefined macros

scott.linder added a comment. In D77923#1978261 , @arsenm wrote: > In D77923#1978172 , @scott.linder > wrote: > > > https://www.khronos.org/registry/OpenCL/sdk/2.0/docs/man/xhtml/preprocessorDirectives.html > >

[PATCH] D77923: OpenCL: Fix some missing predefined macros

scott.linder added a comment. https://www.khronos.org/registry/OpenCL/sdk/2.0/docs/man/xhtml/preprocessorDirectives.html describes `__OPENCL_VERSION__` as "an integer reflecting the version number of the OpenCL supported by the OpenCL device." as contrasted with `__OPENCL_C_VERSION__` which is

  1   2   3   >