[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

arsenm added a comment. Needs test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78495/new/ https://reviews.llvm.org/D78495 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

arsenm added inline comments. Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5 + +void test_amdgcn_fence_failure() { + Does this really depend on C++? Can it use OpenCL like the other builtin tests?This also belongs in a Sema* test directory

[PATCH] D78190: Add Bfloat IR type

arsenm added a comment. What's the plan for handling this in LLT where we don't have specific types with the same size? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78190/new/ https://reviews.llvm.org/D78190 ___

[PATCH] D78817: clang: Allow backend unsupported warnings

arsenm created this revision. arsenm added reviewers: yaxunl, Anastasia, olista01, ostannard, xur. Herald added subscribers: tpr, wdng. Currently this asserts on anything other than errors. In one workaround scenario, AMDGPU emits DiagnosticInfoUnsupported as a warning for functions that can't be

[PATCH] D78862: [IR] Convert null-pointer-is-valid into an enum attribute

arsenm added a comment. FWIW I think this attribute should be replaced with a data layout property, so this would eventually be removed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78862/new/ https://reviews.llvm.org/D78862 ___

[PATCH] D78862: [IR] Convert null-pointer-is-valid into an enum attribute

arsenm added a comment. In D78862#2003684 , @arsenm wrote: > FWIW I think this attribute should be replaced with a data layout property, > so this would eventually be removed Also would be address space specific Repository: rG LLVM Github Monorepo

[PATCH] D78817: clang: Allow backend unsupported warnings

arsenm closed this revision. arsenm added a comment. 5c03beefa720bddb3e3f53c595a76bce7ad50f37 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78817/new/ https://reviews.llvm.org/D78817 __

[PATCH] D78900: [HIP][AMDGPU] Enable structurizer workarounds

arsenm added inline comments. Comment at: clang/lib/Driver/ToolChains/HIP.cpp:158 Args.MakeArgString(Twine("-filetype=") + (OutputIsAsm ? "asm" : "obj"))); + LlcArgs.push_back("--amdgpu-enable-structurizer-workarounds"); We should just flip this in the

[PATCH] D78979: OpenCL: Include builtin header by default

arsenm created this revision. arsenm added reviewers: Anastasia, yaxunl. Herald added subscribers: kerbowa, nhaehnle, wdng, jvesely. arsenm added a comment. I'm also wondering if using -nogpulib for the corresponding linker purpose was correct, since in the OpenCL case it's not really an offload

[PATCH] D78979: OpenCL: Include builtin header by default

arsenm added a comment. I'm also wondering if using -nogpulib for the corresponding linker purpose was correct, since in the OpenCL case it's not really an offload target. Maybe this should switch to -nostdlib? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78979/new/ https://reviews.l

[PATCH] D78980: Elaborate more on --rocm-path flag.

arsenm created this revision. arsenm added reviewers: yaxunl, t-tye. Herald added a subscriber: wdng. I'm not sure what the conventions are for this documentation. The format seems limiting. I don't see how to refer to other flags, or mark flags as deprecated. The rst I believe these generate seem

[PATCH] D78979: OpenCL: Include builtin header by default

arsenm added a comment. In D78979#2007356 , @Anastasia wrote: > > I'm somewhat confused by the way this is supposed to work, or why a > > separate option exists for OpenCL. The other language flags seem to be > > implemented in the driver, not cc1 like O

[PATCH] D78980: Elaborate more on --rocm-path flag.

arsenm marked an inline comment as done. arsenm added inline comments. Comment at: clang/include/clang/Driver/Options.td:612 def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group, - HelpText<"ROCm installation path">; + HelpText<"ROCm installation path, used for finding and a

[PATCH] D78979: OpenCL: Include builtin header by default

arsenm added a comment. In D78979#2011582 , @jvesely wrote: > In D78979#2006901 , @yaxunl wrote: > > > In D78979#2006847 , @arsenm wrote: > > > > > I'm also wondering if usin

[PATCH] D78979: OpenCL: Include builtin header by default

arsenm added a comment. In D78979#2010739 , @Anastasia wrote: > In D78979#2010527 , @svenvh wrote: > > > In D78979#2007643 , @Anastasia > > wrote: > > > > > > Would it not b

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

arsenm added a comment. In D77910#1989163 , @b-sumner wrote: > In D77910#1988807 , @arsenm wrote: > > > In D77910#1981828 , @b-sumner > > wrote: > > > > > In D77910#1981429

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997 case ABIArgInfo::Ignore: + case ABIArgInfo::IndirectAliased: return false; rjmccall wrote: > arsenm wrote: > > rjmccall wrote: > > > In principle, this can be `inreg` just as

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997 case ABIArgInfo::Ignore: + case ABIArgInfo::IndirectAliased: return false; arsenm wrote: > rjmccall wrote: > > arsenm wrote: > > > rjmccall wrote: > > > > In principle, this

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:8816 + // FIXME: Should use byref when promoting pointers in structs, but this + // requires adding implementing the coercion. + if (!getContext().getLangOpts().OpenCL && LTy == OrigLTy && -

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

arsenm updated this revision to Diff 283345. arsenm added a comment. Reword comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79744/new/ https://reviews.llvm.org/D79744 Files: clang/include/clang/CodeGen/CGFunctionInfo.h clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/TargetIn

[PATCH] D80416: [RFC][OpenCL] Set fp contract flag on -cl-mad-enable

arsenm added a comment. Is this still necessary? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80416/new/ https://reviews.llvm.org/D80416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

arsenm closed this revision. arsenm added a comment. 30eeb742f1d11d7a7036e3b8a3bffc1dfd252082 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79744/new/ https://reviews.llvm.org/D79744

[PATCH] D84068: AMDGPU/clang: Search resource directory for device libraries

arsenm added a comment. In D84068#2159391 , @tra wrote: > Could you walk me through how you see this working in practice? > > IIUIC, the idea is to have bitcode files located somewhere within clang > installation. Yes > If that's the case, will we ship

[PATCH] D84068: AMDGPU/clang: Search resource directory for device libraries

arsenm added a comment. In D84068#2208132 , @tra wrote: > In D84068#2204713 , @arsenm wrote: > >>> If we ship them with clang, who/where/how builds them? >>> If they come from ROCm packages, how would those packages

[PATCH] D85607: CfgTraits: add CfgInstructionRef

arsenm added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:100 +// provide one at all. We don't want to lay a subtle performance trap here. +abort(); + } llvm_unreachable? Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D84068: AMDGPU/clang: Search resource directory for device libraries

arsenm added a comment. In D84068#2210872 , @echristo wrote: > Hi All, > > I'd really like to avoid depending on any program other than clang installing > something into clang's working directory. I think this needs to be written > from the perspective o

[PATCH] D86020: [MemCpyOptimizer] Optimize passing byref function arguments down the stack

arsenm added inline comments. Comment at: llvm/test/Transforms/MemCpyOpt/byref-memcpy.ll:44 +} + +declare void @leaf(%struct.S* byref(%struct.S) align 4) I think this is missing an alignment check. Can you add a test where the callee requires a higher parameter

[PATCH] D86154: AMDGPU: Add llvm.amdgcn.{read,readfirst,write}lane2 intrinsics with type overloads

arsenm added a comment. Do we really have to use worse names here? Keeping the name works even if suboptimal for the attributes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86154/new/ https://reviews.llvm.org/D86154 _

[PATCH] D86154: AMDGPU: Add llvm.amdgcn.{read,readfirst,write}lane2 intrinsics with type overloads

arsenm added a comment. In D86154#2224270 , @nhaehnle wrote: > Note that part of my motivation here over D84639 > is to support more general types on the > lane intrinsics, since they also express some semantic content w

[PATCH] D78980: Elaborate more on --rocm-path flag.

arsenm closed this revision. arsenm added a comment. 684dc1bebe5cb70cfd27923940f9f8cba4f13195 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78980/new/ https://reviews.llvm.org/D78980 __

[PATCH] D78862: [IR] Convert null-pointer-is-valid into an enum attribute

arsenm added a comment. In D78862#2012560 , @jdoerfert wrote: > I think this is an improvement over the status quo and it looks fine to me. > > @arsenm I agree that we should tie this to the data layout (or at least > should try) but I guess there are ope

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

arsenm added inline comments. Comment at: llvm/docs/LangRef.rst:1052-1053 +memory of the callee. That means, a callee can write a ``byval`` parameter +and still be ``readonly`` or ``readnone`` because the write is, similar to +a write to an ``alloca``, not visible fro

[PATCH] D79732: AMDGPU/HIP: Don't replace pointer types in kernel argument structs

arsenm created this revision. arsenm added reviewers: hliao, yaxunl. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, nhaehnle, wdng, jvesely, kzhuravl. Currently this is counterproductive and doesn't have the desired effect. The way the promotion is handled is by reinterpreting the poin

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm created this revision. arsenm added reviewers: yaxunl, hliao, jdoerfert, rjmccall, Anastasia, rampitec. Herald added subscribers: kerbowa, nhaehnle, wdng, jvesely. arsenm added parent revisions: D79732: AMDGPU/HIP: Don't replace pointer types in kernel argument structs, D79630: AMDGPU: Star

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm updated this revision to Diff 263274. arsenm added a comment. Forgot to commit a new test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79744/new/ https://reviews.llvm.org/D79744 Files: clang/include/clang/CodeGen/CGFunctionInfo.h clang/lib/CodeGen/CGCall.cpp clang/lib/Cod

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm added a comment. In D79744#2030294 , @rjmccall wrote: > The parameter variable is formally considered to be in a particular address > space, and taking the address of it yields a pointer in that address space. > That can only work for an indirect

[PATCH] D79754: [OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 1

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

[PATCH] D79754: [OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 1

arsenm added inline comments. Comment at: clang/lib/AST/Decl.cpp:3224 + if (Context.getTargetInfo().getTriple().isAMDGCN() && + Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID) && This is also identical to the cuda handling above, can you merge them

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm added a comment. In D79744#2030535 , @rjmccall wrote: > In D79744#2030481 , @arsenm wrote: > > > In D79744#2030294 , @rjmccall > > wrote: > > > > > The parameter vari

[PATCH] D77885: AMDGPU: Search for new ROCm bitcode library structure

arsenm marked an inline comment as done. arsenm added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:75 +// The desired structure is (${ROCM_ROOT} or +// ${OPENCL_ROOT})/amdgcn/bitcode/*, so try to detect this layout. yaxunl wrote:

[PATCH] D76957: HIP: Merge builtin library handling

arsenm closed this revision. arsenm added a comment. 14e184571139ba4c7347ea547074c6d9ec9c7b14 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76957/new/ https://reviews.llvm.org/D76957 __

[PATCH] D77885: AMDGPU: Search for new ROCm bitcode library structure

arsenm accepted this revision. arsenm added a comment. This revision is now accepted and ready to land. Device library change submitted. 123bee602a260150ff55c74287f583a67ee78f36 CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D79788: AMDGPU/OpenCL: Accept -nostdlib in place of -nogpulib

arsenm created this revision. arsenm added reviewers: yaxunl, Anastasia, svenvh. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, nhaehnle, wdng, jvesely, kzhuravl. -nogpulib makes sense when there is a host (where -nostdlib would apply) and offload target. Accept nostdlib when there is

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm added a comment. In D79744#2030710 , @rjmccall wrote: > Okay. So the only real ABI here is the layout of the memory that the > arguments are actually written into? And that memory needs to be treated as > constant? Yes, the actual kernel ABI i

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm added a comment. In D79744#2035109 , @arsenm wrote: > >> Unfortunately, I think `byval` just doesn't match what you want because of >> the mutability — the frontend *has* to have a copy into a local to get IR >> with correct semantics, because

[PATCH] D79754: [OpenMP][AMDGCN] Support OpenMP offloading for AMDGCN architecture - Part 1

arsenm added inline comments. Comment at: llvm/include/llvm/ADT/Triple.h:699 + /// Tests whether the target is a GPU i.e. NVPTX or AMDGCN + bool isGPU() const { return isNVPTX() || isAMDGCN(); } + This is skipping out r600, and probably should leave this in cla

[PATCH] D79788: AMDGPU/OpenCL: Accept -nostdlib in place of -nogpulib

arsenm closed this revision. arsenm added a comment. 235fb7dc24b1cf7034dfc76bb853ffb4ac5dec5d CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79788/new/ https://reviews.llvm.org/D79788 __

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

arsenm added a comment. In D71726#1801346 , @__simt__ wrote: > In D71726#1792852 , @yaxunl wrote: > > > In D71726#1791904 , @jfb wrote: > > > > > This generally seems fine. D

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

arsenm added a comment. In D79744#2035283 , @rjmccall wrote: > > A completely different approach: OpenMP has to solve some very similar > problems and just lowers them completely in the frontend; have you considered > just doing that? Kernels need a

[PATCH] D40806: CodeGen: Fix invalid bitcasts for memcpy

arsenm accepted this revision. arsenm added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D40806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D40956: [AMDGPU] Switch to the new addr space mapping by default for clang

arsenm accepted this revision. arsenm added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D40956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D133726: [OpenMP][AMDGPU] Link bitcode ROCm device libraries per-TU

arsenm added a comment. Does this fix the weird behavior where you needed to use -lm to use anything in the device libraries? I don't see that being removed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133726/new/ https://reviews.llvm.org/D133726

[PATCH] D130096: [Clang][AMDGPU] Emit AMDGPU library control constants in clang

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9449-9450 + !(Features & llvm::AMDGPU::FEATURE_WAVE32) || + llvm::is_contained(CGM.getTarget().getTargetOpts().FeaturesAsWritten, + "+wavefrontsize64"); +

[PATCH] D141700: AMDGPU: Move enqueued block handling into clang

arsenm added inline comments. Comment at: llvm/lib/IR/CMakeLists.txt:84 Demangle + TransformUtils + This introduces a circular dependency between LLVMCore and TransformUtils. Options are: 1. Move appendToUsed into Module 2. Don't bother with bitcode compati

[PATCH] D141700: AMDGPU: Move enqueued block handling into clang

arsenm updated this revision to Diff 558095. arsenm added a comment. Drop bitcode auto upgrade handling CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141700/new/ https://reviews.llvm.org/D141700 Files: clang/lib/CodeGen/Targets/AMDGPU.cpp clang/test/CodeGenOpenCL/amdgpu-enqueue-ker

[PATCH] D128700: [AMDGPU][Clang] Skip adding noundef attribute to AMDGPU HIP device functions

arsenm added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2309 + bool EnableNoundefAttrs = CodeGenOpts.EnableNoundefAttrs && +!(getLangOpts().HIP && getLangOpts().CUDAIsDevice); + Shouldn't be limited to HIP. All language

[PATCH] D128907: [Clang] Disable noundef attribute for languages which allow uninitialized function arguments

arsenm added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:534 + bool allowUninitializedFunctionsArgs() const { +return ConvergentFunctions; + } Should add a comment explaining it here Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D128158: [AMDGPU] Add amdgcn_sched_group_barrier builtin

arsenm added inline comments. Comment at: clang/test/SemaOpenCL/builtins-amdgcn-error.cl:70 +{ + __builtin_amdgcn_sched_group_barrier(x, 0, 1); // expected-error {{argument to '__builtin_amdgcn_sched_group_barrier' must be a constant integer}} +} Test error for

[PATCH] D129298: Add denormal-fp-math attribute for f16

arsenm added a comment. Missing ARM changes that demonstrate the use of the control? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129298/new/ https://reviews.llvm.org/D129298 ___ cfe-commits mailing lis

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

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

[PATCH] D122589: Additionally set f32 mode with denormal-fp-math

arsenm added a comment. Testcase? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122589/new/ https://reviews.llvm.org/D122589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D122589: Additionally set f32 mode with denormal-fp-math

arsenm added a comment. I think my intent here was to not emit denormal-fp-math-f32 at all if it wasn't specified (i.e. denormal-fp-math-f32 is a special override only really used for AMDGPU) The interaction between the two and the default is supposed to be handled by the backend interpretatio

[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

arsenm added a comment. I can't see a scenario where we would need this with a variadic call. Ultimately these wrap specific physical instructions, not some kind of arbitrary call CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130224/new/ https://reviews.llvm.org/D130224 _

[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

arsenm added a comment. In D130224#3686756 , @nlopes wrote: > Would it be possible to implement this by dropping the `noundef` attribute > rather than emitting a freeze? Seems like a much better option to me. See @nhaehnle's post here

[PATCH] D131183: AMDGPU/clang: Remove dead code

arsenm created this revision. arsenm added reviewers: yaxunl, sameerds, saiislam, JonChesterfield, RKSimon. Herald added subscribers: kosarev, t-tye, tpr, dstuttard, kzhuravl. Herald added a project: All. arsenm requested review of this revision. Herald added a subscriber: wdng. The order has to b

[PATCH] D131183: AMDGPU/clang: Remove dead code

arsenm closed this revision. arsenm added a comment. c5b36ab1d6a667554bf369c34e51d02add039d16 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131183/new/ https://reviews.llvm.org/D131183 __

[PATCH] D106030: [Clang] add support for error+warning fn attrs

arsenm added inline comments. Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:208 } +// TODO: is this the best pass for this? +for (BasicBlock &BB : F) { Why not just do in in codegen in IRTranslator/SelectionDAGBuilder?

[PATCH] D106030: [Clang] add support for error+warning fn attrs

arsenm added a comment. Adding something to the IR for the sole purpose of producing a diagnostic feels really weird. I'm not sure I see why the frontend can't see this attribute and directly warn Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1060

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

arsenm accepted this revision. arsenm added a comment. This revision is now accepted and ready to land. A spurious warning still isn't ideal but this is an improvement Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121951/new/ https://reviews.llvm.o

[PATCH] D123825: clang/AMDGPU: Define macro for -munsafe-fp-atomics

arsenm created this revision. arsenm added reviewers: yaxunl, rampitec. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, jvesely, kzhuravl. Herald added a project: All. arsenm requested review of this revision. Herald added a subscriber: wdng. The HIP headers want to use this to swap the

[PATCH] D123825: clang/AMDGPU: Define macro for -munsafe-fp-atomics

arsenm closed this revision. arsenm added a comment. a1303b23c9de6ef6d667aa923ec266ca4a0334e7 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123825/new/ https://reviews.llvm.org/D123825 __

[PATCH] D122589: Additionally set f32 mode with denormal-fp-math

arsenm accepted this revision. arsenm added a comment. This revision is now accepted and ready to land. LGTM. I played around with this and it seems to have the overriding behavior I would expect Comment at: clang/test/CodeGen/denormalfpmode-f32.c:21 + +// CHECK-LABEL: main +

[PATCH] D86351: WIP: llvm-buildozer

arsenm added inline comments. Comment at: llvm/lib/Analysis/MemorySSA.cpp:89 #ifdef EXPENSIVE_CHECKS -bool llvm::VerifyMemorySSA = true; +LLVM_THREAD_LOCAL bool llvm::VerifyMemorySSA = true; #else Do command line flags like this really need to be thread local?

[PATCH] D119216: [AMDGPU] replace hostcall module flag with function attribute

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:198 +const auto *STI = TM.getMCSubtargetInfo(); +return llvm::AMDGPU::getHostcallImplicitArgPosition(STI); + } The ABI should not be a property of the subtarget, and th

[PATCH] D119216: [AMDGPU] replace hostcall module flag with function attribute

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:521 + + while (!WorkList.empty()) { +auto UseInfo = WorkList.back(); Can you use checkForAllUses instead of creating your own worklist? Repository: rG LLVM Gith

[PATCH] D119216: [AMDGPU] replace hostcall module flag with function attribute

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:198 +const auto *STI = TM.getMCSubtargetInfo(); +return llvm::AMDGPU::getHostcallImplicitArgPosition(STI); + } arsenm wrote: > The ABI should not be a property of the s

[PATCH] D119216: [AMDGPU] replace hostcall module flag with function attribute

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:198 +const auto *STI = TM.getMCSubtargetInfo(); +return llvm::AMDGPU::getHostcallImplicitArgPosition(STI); + } arsenm wrote: > sameerds wrote: > > arsenm wrote: > > > T

[PATCH] D119216: [AMDGPU] replace hostcall module flag with function attribute

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:566 + return false; +}; + sameerds wrote: > sameerds wrote: > > jdoerfert wrote: > > > sameerds wrote: > > > > jdoerfert wrote: > > > > > sameerds wrote: > > > > > > jd

[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper

arsenm added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8195 + // Get the AMDGPU math libraries. + // FIXME: This method is bad, remove once AMDGPU has a proper math library. + for (auto &I : llvm::make_range(OpenMPTCRange.first, OpenMPTCRange.second))

[PATCH] D124158: [Clang][Attr] Skip adding noundef attribute to arguments when function has convergent attribute

arsenm added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2435 DetermineNoUndef(ParamType, getTypes(), DL, AI)) { - Attrs.addAttribute(llvm::Attribute::NoUndef); + if(!FuncAttrs.contains(llvm::Attribute::Convergent)) +Attrs.addAttribute(llv

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

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1178-1266 + +void AMDGPUAsmPrinter::emitResourceUsageRemarks( +const MachineFunction &MF, const SIProgramInfo &CurrentProgramInfo, +bool isModuleEntryFunction, bool hasMAIInsts) { + if

[PATCH] D95063: AMDGPU: Use optimization remarks for register usage

arsenm abandoned this revision. arsenm added a comment. Herald added subscribers: hsmhsm, foad. Herald added a project: All. Obsoleted by D123878 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95063/new/ https://reviews.llvm.org/D95063

[PATCH] D124158: [Clang][Attr] Skip adding noundef attribute to arguments when function has convergent attribute

arsenm added a comment. As far as I know this is supposed to be a broadcast from lane 0 to every lane, so not sure why the control flow really matters Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124158/new/ https://reviews.llvm.org/D124158

[PATCH] D124158: [Clang][Attr] Skip adding noundef attribute to arguments when function has convergent attribute

arsenm added a comment. In D124158#3478319 , @jdoerfert wrote: >> The issue you're describing sounds like it's specific to @__shfl_sync. In >> general, in C++, you aren't allowed to read from an uninitialized variable; >> see [basic.indet] in the stand

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

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1222 + &MF.front()) + << "ScratchSize [bytes/thread]: " + << ore::NV("ScratchSize", CurrentProgramInfo.ScratchSize); -

[PATCH] D124700: [AMDGPU] Add llvm.amdgcn.sched.barrier intrinsic

arsenm added a comment. Can you add a test to make sure the hazard recognizer and code size estimate don’t think this is a real instruction Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124700/new/ https://reviews.llvm.org/D124700 ___

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

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1264 + &MF.front()) + << "--"; + }); Get rid of the ——. It’s not a remark and only kind of mak

[PATCH] D124915: Check for resource exhaustion when recursively parsing declarators

arsenm added a comment. I think I've seen this one before Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124915/new/ https://reviews.llvm.org/D124915 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D129298: Add denormal-fp-math attribute for f16

arsenm added a comment. In D129298#3638759 , @dcandler wrote: > There are currently no Arm specific changes, this is just being able to more > accurately describe the floating point environment via attributes in the case > where singles and doubles shou

[PATCH] D128907: [Clang] Disable noundef attribute for languages which allow uninitialized function arguments

arsenm added a comment. In D128907#3650750 , @jdoerfert wrote: > That said, I doubt this is even what we want. Throwing away the benefits of > the noundef for one special case. IIRC, I mentioned alternatives in the other > discussion already,... not tha

[PATCH] D128907: [Clang] Disable noundef attribute for languages which allow uninitialized function arguments

arsenm added a comment. In D128907#3652140 , @jdoerfert wrote: > In D128907#3652077 , @arsenm wrote: > >> In D128907#3650750 , @jdoerfert >> wrote: >> >>> That said, I do

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

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1222 + // printing multiple diagnostic location and diag opts. + EmitResourceUsageRemark("KernelName", "Kernel Name", + MF.getFunction().getName()); W

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

arsenm added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1222 + // printing multiple diagnostic location and diag opts. + EmitResourceUsageRemark("KernelName", "Kernel Name", + MF.getFunction().getName()); v

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

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

[PATCH] D128907: [Clang] Disable noundef attribute for languages which allow uninitialized function arguments

arsenm added a comment. The consensus in the meeting was that we should try introducing a new argument attribute for values that are allowed to use undef values. It would need to be applied to the builtins and any wrapper functions for them. This would leave the normal language undefined behavi

[PATCH] D130096: [Clang][AMDGPU] Emit AMDGPU library control constants in clang

arsenm added a comment. I've thought that directly emitting these constants would be better. This will also make it so you can't try to continue using llvm-link for these libraries, which is a plus since it doesn't have the same necessary attribute propagation clang does when linking these Re

[PATCH] D130096: [Clang][AMDGPU] Emit AMDGPU library control constants in clang

arsenm added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9456 +llvm::ConstantInt::get(Type, Value), Name, nullptr, +llvm::GlobalValue::ThreadLocalMode::NotThreadLocal, 4); +GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Local); --

[PATCH] D130096: [Clang][AMDGPU] Emit AMDGPU library control constants in clang

arsenm added a comment. In D130096#3663398 , @jhuber6 wrote: > In D130096#3663295 , @yaxunl wrote: > >> There is no constant propagation for globals with weak linage, right? >> Otherwise, it won't work. My concer

[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

arsenm added a comment. In D130224#3668225 , @aaron.ballman wrote: > It sounds like there's an extremely specific use case in mind for this > attribute; is there a reason why this needs to be the user's problem instead > of something the compiler can i

[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

arsenm added a comment. In D130224#3668243 , @aaron.ballman wrote: > In D130224#3668240 , @arsenm wrote: > >> In D130224#3668225 , >> @aaron.ballman wrote: >> >>> It sou

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