Modifying option for nvrtc #2926

jjsjann123 · 2024-09-10T14:06:12Z

Adding NVFUSER_ENABLE=kernel_debug to enable debug option -G in nvrtc;
Moving DebugDumpOption::DebugInfo to EnableOption::KernelLineInfo.

jacobhinkle · 2024-09-10T14:27:10Z

Option naming

We already have -lineinfo which can be enabled using NVFUSER_DUMP=debug_info. That is similar to -G which also disables optimizations and is called NVFUSER_ENABLE=jit_debug in this PR. I would suggest we try to disambiguate these two. I think debug_info is not clear, while neither is the cmdline option -lineinfo unfortunately.

NVFUSER_DUMP=debug_info is mostly useful for profiling so we can see code locations with ncu. However, to add to the confusion we also have NVFUSER_ENABLE=kernel_profile which enables intra-kernel profiling markers, and NVFUSER_PROF=... which does CUPTI profiling of kernels as well as host latency.

So I think we have four related utilities that use three different env vars. One of the utilities is clearly for functional debugging (this PR), while the others are mostly for profiling and perf debugging. I think we should rename the NVFUSER_DUMP=debug_info option as NVFUSER_ENABLE=kernel_lineinfo (it affects the generated kernel so I think it should be an EnableOption instead of a dump option) and as a separate issue we might want to make sure NVFUSER_ENABLE=kernel_profile is not too confusing: e.g. the NVFUSER_PROF profiler uses a structure called KernelProfile.

naoyam · 2024-09-10T14:45:12Z

Here's the original PR to introduce debug_info: csarofeen/pytorch#1855

naoyam · 2024-09-10T14:51:02Z

I think we should rename the NVFUSER_DUMP=debug_info option as NVFUSER_ENABLE=kernel_lineinfo (it affects the generated kernel so I think it should be an EnableOption instead of a dump option)

+1

jjsjann123 · 2024-09-10T14:53:28Z

funny enough, I was trying to use debug_info but saw it taken for the line info already....

Looks like you guys don't mind the renaming. I'll:

change this flag to NVFUSER_ENABLE=kernel_debug`
As @jacobhinkle suggested, change NVFUSER_DUMP=debug_info option as NVFUSER_ENABLE=kernel_lineinfo

naoyam · 2024-09-10T15:00:17Z

Do we need -G rather than just --lineinfo? The former also disables optimization.

jjsjann123 · 2024-09-10T15:03:46Z

Do we need -G rather than just --lineinfo? The former also disables optimization.

Yes, those two are different.

--device-debug (-G)

Generate debug information. If '--dopt' is not specified, then turns off all optimizations.

--generate-line-info (-lineinfo)

Generate line-number information.

I verified with the generated PTX. We only see debug when -G is given to nvrtc.

.version 8.5
.target sm_80, debug

jjsjann123 · 2024-09-10T15:04:35Z

But realistically, since this is only for @xwang233 's compute sanitizer stuff, is -lineinfo enough for that, or do we need debug mode?

…on::KernelLineInfo

naoyam · 2024-09-10T15:07:57Z

Yes, that's what I'm asking about. For sanitizer, I think --lineinfo is sufficient. That said, -G may also be useful for actual debugging, so I'm fine to add that too.

jjsjann123 · 2024-09-10T15:08:39Z

csrc/options.cpp

@@ -162,6 +161,8 @@ std::unordered_map<EnableOption, std::vector<std::string>> Options<
      {"static_fusion_count", EnableOption::StaticFusionCount},
      {"warn_register_spill", EnableOption::WarnRegisterSpill},
      {"io_to_lower_precision", EnableOption::IoToLowerPrecision},
+      {"kernel_debug", EnableOption::KernelDebug},
+      {"kernel_lineinfo", EnableOption::KernelLineInfo},


tagging @zasdfgbnm , this is renamed from DebugDumpOption::DebugInfo

naoyam · 2024-09-10T15:15:47Z

csrc/fusion_executor/executor.cpp

@@ -179,7 +179,7 @@ std::string FusionExecutor::getStructuredCode(
            << code << "\n======================================\n\n";
  }
  if (isDebugDumpEnabled(DebugDumpOption::CudaToFile) ||
-      isDebugDumpEnabled(DebugDumpOption::DebugInfo)) {
+      isOptionEnabled(EnableOption::KernelLineInfo)) {


It seems to me a little unexpected that an enable option dumps a file. Do we need this?

I agree and I'm more than happy to remove this one. tagging @zasdfgbnm to see if there's a reason that we were dumping the cuda source in the first place.

This is not a strong request (so approved the PR already), but my preference is to remove this line. I feel that's clearer.

I agree and I'm more than happy to remove this one. tagging @zasdfgbnm to see if there's a reason that we were dumping the cuda source in the first place.

I think the original intention was indeed dumping some useful info for analyses using ncu. This can be done NVFUSER_ENABLE=kernel_lineinfo NVFUSER_DUMP=cuda_to_file. Now that we have (too) many options, I'd prefer each option as simple as possible.

It's probably there because the line info is pretty useless without the dumped kernel. If you only provide NVFUSER_DUMP=debug_info and not cuda_to_file, then ncu-ui will not show the source (it gives a file not found when you try to look at the source and asks where the file is).

It's probably there because the line info is pretty useless without the dumped kernel. If you only provide NVFUSER_DUMP=debug_info and not cuda_to_file, then ncu-ui will not show the source (it gives a file not found when you try to look at the source and asks where the file is).

🤯 That's a real surprise... I though I've been using the lineinfo on my local machine when I copy over just the profile file without the actual cuda source.

I must have got something wrong... Let me try playing with it a bit.

The discussion here covers all the information. I have nothing more to add, and I am OK with the change. Just a small request that we should document how to use these flags in our wiki page.

good point. I'll remember to do that before merging this one.

Updated in https://github.com/NVIDIA/Fuser/wiki/Developer-guide#profile-kernels

FYI, you don't have to manually copy the cuda source. you can opt in with --import-source 1 in ncu to import the source into the report, so you don't need to manually resolve that in ncu-ui.

But yeah, you still need to have the cuda source dumped out in the first place.

naoyam

Thanks for the cleanup!

adding debug option as enable options

4ff7feb

jjsjann123 requested review from naoyam and xwang233 September 10, 2024 14:06

address review comment, move DebugDumpOption::DebugInfo to EnableOpti…

b335006

…on::KernelLineInfo

jjsjann123 requested review from zasdfgbnm and jacobhinkle September 10, 2024 15:06

jjsjann123 changed the title ~~debug option for nvrtc~~ Modifying option for nvrtc Sep 10, 2024

oops

289f875

jjsjann123 commented Sep 10, 2024

View reviewed changes

naoyam reviewed Sep 10, 2024

View reviewed changes

naoyam approved these changes Sep 10, 2024

View reviewed changes

removing kernel dump per review request

04a8a3e

jjsjann123 merged commit 6d70792 into main Sep 10, 2024
5 checks passed

jjsjann123 deleted the jit_debug_option branch September 10, 2024 19:10

liqiangxl mentioned this pull request Oct 4, 2024

save cuda file when kernel line info is enabled #3113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifying option for nvrtc #2926

Modifying option for nvrtc #2926

jjsjann123 commented Sep 10, 2024 •

edited

Loading

jacobhinkle commented Sep 10, 2024

naoyam commented Sep 10, 2024

naoyam commented Sep 10, 2024

jjsjann123 commented Sep 10, 2024

naoyam commented Sep 10, 2024

jjsjann123 commented Sep 10, 2024

jjsjann123 commented Sep 10, 2024

naoyam commented Sep 10, 2024

jjsjann123 Sep 10, 2024

naoyam Sep 10, 2024

jjsjann123 Sep 10, 2024

naoyam Sep 10, 2024

naoyam Sep 10, 2024

jacobhinkle Sep 10, 2024

jjsjann123 Sep 10, 2024 •

edited

Loading

zasdfgbnm Sep 10, 2024

jjsjann123 Sep 10, 2024

jjsjann123 Sep 10, 2024

jjsjann123 Sep 10, 2024 •

edited

Loading

naoyam left a comment

Modifying option for nvrtc #2926

Modifying option for nvrtc #2926

Conversation

jjsjann123 commented Sep 10, 2024 • edited Loading

jacobhinkle commented Sep 10, 2024

Option naming

naoyam commented Sep 10, 2024

naoyam commented Sep 10, 2024

jjsjann123 commented Sep 10, 2024

naoyam commented Sep 10, 2024

jjsjann123 commented Sep 10, 2024

jjsjann123 commented Sep 10, 2024

naoyam commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsjann123 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsjann123 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

naoyam left a comment

Choose a reason for hiding this comment

jjsjann123 commented Sep 10, 2024 •

edited

Loading

jjsjann123 Sep 10, 2024 •

edited

Loading

jjsjann123 Sep 10, 2024 •

edited

Loading