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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG67357739c6d3: [AMDGPU] Add remarks to output some resource usage (authored by vangthao). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

vangthao updated this revision to Diff 444844. vangthao added a comment. Change `auto &` to `auto Argument`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123878/new/ https://reviews.llvm.org/D123878 Files:

[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

arsenm 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 line

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

vangthao updated this revision to Diff 444759. vangthao added a comment. Change "Kernel Name" to "Function Name" and rebased patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123878/new/ https://reviews.llvm.org/D123878 Files:

[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());

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

vangthao 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());

[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());

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

vangthao marked 5 inline comments as done. vangthao added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123878/new/ https://reviews.llvm.org/D123878 ___ cfe-commits mailing list

[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] D123878: [AMDGPU] Add remarks to output some resource usage

vangthao updated this revision to Diff 440447. vangthao added a comment. Remove "" delimiter. Change ScratchSize [bytes/thread] to ScratchSize [bytes/lane]. Use lambda expression to emit remarks. Do not output yaml if specific remark is not enabled. Add indentation to make it easier to

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

arsenm 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 print

[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

vangthao added a comment. 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 remark, warning, etc. but allowing for a diagnostic to output their own newline makes it

[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

afanfa added a comment. Herald added subscribers: kosarev, jsilvanus. 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 print new lines. Repository: rG LLVM

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

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

[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

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

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

vangthao 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) +return;

[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 , const SIProgramInfo , +bool isModuleEntryFunction, bool hasMAIInsts) { + if (!ORE) +return; +

[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] D123878: [AMDGPU] Add remarks to output some resource usage

vangthao added inline comments. Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:596 + MF.getFunction().getSubprogram(), ()) + << "--"; + }); scott.linder wrote: > This seems like the wrong place to

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

vangthao updated this revision to Diff 424344. vangthao added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Move remarks into its own function. Skip if !ORE. Add clang frontend test. Remove LDSSpillSize. Repository: rG LLVM Github Monorepo CHANGES SINCE