[NVPTX] Disable DWARF .file directory for PTX

[asavonic]

Summary

Default behavior for .file directory was changed in D105856, but
ptxas (CUDA 11.5 release) refuses to parse it:

$ llc -march=nvptx64 llvm/test/DebugInfo/NVPTX/debug-file-loc.ll
$ ptxas debug-file-loc.s
ptxas debug-file-loc.s, line 42; fatal   : Parsing error near
'"foo.h"': syntax error

Added a new field to MCAsmInfo to control default value of UseDwarfDirectory.
This value is used if -dwarf-directory command line option is not specified.

[tra]

Can you please update the diff with more context as described in LLVM docs: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[MaskRay]

Can you list the clang and ptxas commands to be clearer? Also, should the change be in clang/lib/Driver? There are existing settings for CUDA.
It's also worth adding comments like "As of <date>/<version>, ptxas does not support ..." so that others can revisit the opt-out in the future.

[asavonic]

Re-upload the patch with -U99999

[tra]

In D121299#3370530, @MaskRay wrote:

Can you list the clang and ptxas commands to be clearer?

I do not think ptxas gets any special command line options. Debug info format affects the generated PTX outout only.

Also, should the change be in clang/lib/Driver? There are existing settings for CUDA.
It's also worth adding comments like "As of <date>/<version>, ptxas does not support ..." so that others can revisit the opt-out in the future.

We already clamp DWARF version for targets that don't support newer ones.
If split directory/file arguments for .file directive are properties of DWARF5 (AFAICT, they are: https://sourceware.org/binutils/docs/as/File.html) , then not generating this format for older versions looks sensible to me. It does not have anything to do with CUDA itself.
The question is whether this is the right place to do it, or would it better to do it wherever UseDwarfDirectory is set.

llvm/lib/MC/MCAsmStreamer.cpp

1557–1559

Nit. It may be more readable to rephrase it as:

bool DwarfDirIsSupported = getContext().getDwarfVersion() >= 5 || MAI->useIntegratedAssembler();

printDwarfFileDirective(FileNo, Directory, Filename, Checksum, Source,
                          UseDwarfDirectory && DwarfDirIsSupported, OS1);

[MaskRay]

This comment was removed by MaskRay.

[MaskRay]

I am a bit unsure whether we should make such logic spread in two places (clang Driver and MCAsmStreamer).
It's nice that llc without other options (-dwarf-directory=0) produced assembly can be passed to the assembler for the target but that's not a requirement.

[asavonic]

The question is whether this is the right place to do it, or would it better to do it wherever UseDwarfDirectory is set.

This option is first set in llc.cpp and then propagated to MCAsmStreamer. I think this is the right place for to do it, but we can move this code to the class constructor instead.

In D121299#3376523, @MaskRay wrote:

I am a bit unsure whether we should make such logic spread in two places (clang Driver and MCAsmStreamer).
It's nice that llc without other options (-dwarf-directory=0) produced assembly can be passed to the assembler for the target but that's not a requirement.

I think it is important for llc to produce valid assembly output by default, especially considering that the error message from ptxas is not obvious.
Although the way we should change this default depends on use cases and expectations for -dwarf-directory. I traced it back to 2011 (commit 40f8f2ff "Add support for a new extension to the .file directive") , where it it was added as an extension and (I assume) later included into DWARFv5. Do we still want to support it as an extension for DWARF < 5, or it is only needed for integrated assembler?

[probinson]

I traced it back to 2011 (commit 40f8f2ff "Add support for a new extension to the .file directive") , where it it was added as an extension and (I assume) later included into DWARFv5. Do we still want to support it as an extension for DWARF < 5, or it is only needed for integrated assembler?

The DWARF line table has had separate directory and file lists going back to DWARF v2; this .file directive syntax is unrelated to DWARF version.
If some external assemblers don't support the split .file syntax, the flags/conditions should reflect that based on target or whatever. Certainly not on DWARF version.

[asavonic]

In D121299#3397875, @probinson wrote:

I traced it back to 2011 (commit 40f8f2ff "Add support for a new extension to the .file directive") , where it it was added as an extension and (I assume) later included into DWARFv5. Do we still want to support it as an extension for DWARF < 5, or it is only needed for integrated assembler?

The DWARF line table has had separate directory and file lists going back to DWARF v2; this .file directive syntax is unrelated to DWARF version.
If some external assemblers don't support the split .file syntax, the flags/conditions should reflect that based on target or whatever. Certainly not on DWARF version.

Thanks a lot! So .file fileno directory filename syntax *can* be lowered to DWARF < 5, but some assemblers decide not to support it: GNU as only suppots it for DWARFv5 [1], ptxas does not support it at all. Therefore it is important to keep -dwarf-directory option as a way to force this feature, but we also need a way to override the default value - setting it to true does not make sense for NVPTX.

[1]: https://sourceware.org/binutils/docs/as/File.html

[asavonic]

• Added a new field to MCAsmInfo to control default value of UseDwarfDirectory. This value is used if -dwarf-directory command line option is not specified.

[MaskRay]

In D121299#3376523, @MaskRay wrote:

I am a bit unsure whether we should make such logic spread in two places (clang Driver and MCAsmStreamer).
It's nice that llc without other options (-dwarf-directory=0) produced assembly can be passed to the assembler for the target but that's not a requirement.

I think it is important for llc to produce valid assembly output by default, especially considering that the error message from ptxas is not obvious.

I just worry whether this patch may be busywork. It seems like a "UI" thing of llc anyway, not really affecting Clang.
Have you tried contacting CUDA folks about supporting the syntax in ptxas?
If ptxas will soon get the support, in the future someone may propose to revert this patch (the enum DwarfDirectory mechanism won't be needed).

Although the way we should change this default depends on use cases and expectations for -dwarf-directory. I traced it back to 2011 (commit 40f8f2ff "Add support for a new extension to the .file directive") , where it it was added as an extension and (I assume) later included into DWARFv5. Do we still want to support it as an extension for DWARF < 5, or it is only needed for integrated assembler?

[This has been answered by Paul]

[MaskRay]

llvm/include/llvm/MC/MCTargetOptions.h

61 ↗

(On Diff #417931)

I am not sure the presumably temporary ptxas issue justifies the complexity here and in MCAsmInfo.h. If you really think so strongly that llc should use non-directory .file for NVPTX, you can temporarily only change llc instead.
But as my comment suggests, it will be unneeded.

[tra]

In D121299#3405993, @MaskRay wrote:

I just worry whether this patch may be busywork. It seems like a "UI" thing of llc anyway, not really affecting Clang.
Have you tried contacting CUDA folks about supporting the syntax in ptxas?
If ptxas will soon get the support, in the future someone may propose to revert this patch (the enum DwarfDirectory mechanism won't be needed).

I can ask NVIDIA, but it's not going to help us much.
Even if we can convince NVIDIA to support this syntax, it will only apply to ptxas in some future CUDA version.
We'll still need to deal with ptxas that has been shipped with all existing CUDA versions we support.

[MaskRay]

In D121299#3406012, @tra wrote:

In D121299#3405993, @MaskRay wrote:

I just worry whether this patch may be busywork. It seems like a "UI" thing of llc anyway, not really affecting Clang.
Have you tried contacting CUDA folks about supporting the syntax in ptxas?
If ptxas will soon get the support, in the future someone may propose to revert this patch (the enum DwarfDirectory mechanism won't be needed).

I can ask NVIDIA, but it's not going to help us much.

Thanks

Even if we can convince NVIDIA to support this syntax, it will only apply to ptxas in some future CUDA version.
We'll still need to deal with ptxas that has been shipped with all existing CUDA versions we support.

AIUI the complexity is to make llc -mtriple=nvptx64-nvidia-cuda -dwarf-directory=0 a.ll users convenient, but they can use clang --target=nvptx64-nvidia-cuda -S a.ll instead, to avoid setting -dwarf-directory.
What we need to do is make tradeoff between complexity and convenience. Omitting -dwarf-directory=0 considering that (a) clang doesn't need the manual setting (b) the mechanism will need to be removed in the future
makes me unsure we need to introduce the seemingly heavy infrastructure as implemented in this patch.

If the patch is reworked to avoid EnableDwarfFileDirectoryDefault, I may concede.

[asavonic]

In D121299#3405993, @MaskRay wrote:

I just worry whether this patch may be busywork. It seems like a "UI" thing of llc anyway, not really affecting Clang.

Yes, this is only about llc command line interface for PTX.

AIUI the complexity is to make llc -mtriple=nvptx64-nvidia-cuda -dwarf-directory=0 a.ll users convenient, but they can use clang --target=nvptx64-nvidia-cuda -S a.ll instead, to avoid setting -dwarf-directory.

Well, my motivation is to avoid adding this option to every llc LIT test that requires it (as part of D121727), and there is no way to use clang for them.
Also, I believe is that "by default" llc should output valid assembly for the target. If it doesn't, then it is inconvenient to use at best, or a source of errors for no good reason.

What we need to do is make tradeoff between complexity and convenience. Omitting -dwarf-directory=0 considering that (a) clang doesn't need the manual setting (b) the mechanism will need to be removed in the future makes me unsure we need to introduce the seemingly heavy infrastructure as implemented in this patch.

Clang driver is a separate subject. Many tests do not use clang driver, developers may not use it, and there are other frontends that all need to pass this option.

If the patch is reworked to avoid EnableDwarfFileDirectoryDefault, I may concede.

I see a couple of options:

• revert this default back to false.
• add a check for Triple = NVPTX directly to llc.

[tra]

In D121299#3406044, @MaskRay wrote:

AIUI the complexity is to make llc -mtriple=nvptx64-nvidia-cuda -dwarf-directory=0 a.ll users convenient, but they can use clang --target=nvptx64-nvidia-cuda -S a.ll instead, to avoid setting -dwarf-directory.

What we need to do is make tradeoff between complexity and convenience. Omitting -dwarf-directory=0 considering that (a) clang doesn't need the manual setting (b) the mechanism will need to be removed in the future
makes me unsure we need to introduce the seemingly heavy infrastructure as implemented in this patch.

To me it looks like yet another knob to accommodate a target-specific quirk. We already have to deal with various oddities of PTX, including limits on supported DWARF version and various oddities of PTX syntax. This particular knob appears relatively noninvasive to me.

I would argue that it's worth making things 'just work'. We can not expect all users to remember that if they want to compile for target X with debug info, then they need to specify additional options.

[asavonic]

• Fixed Clang build.

[asavonic]

@MaskRay, should we merge this patch or rework it?

[asavonic]

ping

[asavonic]

I'm going to merge the patch in a couple of days, unless anyone has a strong opinion against it.
Alternative options:

• revert this default back to false (not great for other targets)
• add a check for Triple == NVPTX directly to llc (tools other than llc will have to replicate the same logic)

[probinson]

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."

llvm/lib/MC/MCAsmStreamer.cpp

 
   SmallString<128> Str;
   raw_svector_ostream OS1(Str);
+  bool UseDwarfDir = UseDwarfDirectory;
+  if (getContext().getDwarfVersion() < 5 && !MAI->useIntegratedAssembler())
+    UseDwarfDir = false;
+
   printDwarfFileDirective(FileNo, Directory, Filename, Checksum, Source,
-                          UseDwarfDirectory, OS1);
+                          UseDwarfDir, OS1);
 
   if (MCTargetStreamer *TS = getTargetStreamer())
     TS->emitDwarfFileDirective(OS1.str());

llvm/test/DebugInfo/NVPTX/dbg-declare-alloca.ll

-; RUN: llc < %s -mtriple=nvptx64-nvidia-cuda -dwarf-directory=0 | FileCheck %s
+; RUN: llc < %s -mtriple=nvptx64-nvidia-cuda | FileCheck %s
 
 ; CHECK: .target sm_20, debug
 

llvm/test/DebugInfo/NVPTX/debug-file-loc-only.ll

   ret void, !dbg !11
 }
 
-; CHECK-DAG: .file [[FOO]] "{{.*}}foo.h"
-; CHECK-DAG: .file [[BAR]] "{{.*}}bar.cu"
+; CHECK-DAG: .file [[FOO]] "/source/dir/foo.h"
+; CHECK-DAG: .file [[BAR]] "/source/dir/bar.cu"
 
 ; CHECK-NOT: .section .debug{{.*}}
 

llvm/test/DebugInfo/NVPTX/debug-file-loc.ll

   ret void, !dbg !11
 }
 
-; CHECK-DAG: .file [[FOO]] "{{.*}}foo.h"
-; CHECK-DAG: .file [[BAR]] "{{.*}}bar.cu"
+; CHECK-DAG: .file [[FOO]] "/source/dir/foo.h"
+; CHECK-DAG: .file [[BAR]] "/source/dir/bar.cu"
 ; CHECK: .section .debug_abbrev
 ; CHECK-NEXT: {
 ; CHECK-NEXT: .b8 1                                // Abbreviation Code

llvm/test/DebugInfo/NVPTX/debug-loc-offset.ll

-; RUN: llc -mtriple=nvptx64-nvidia-cuda -dwarf-directory=0 < %s | FileCheck %s
+; RUN: llc -mtriple=nvptx64-nvidia-cuda < %s | FileCheck %s
 
 ; CHECK: .target sm_{{[0-9]+}}, debug