Opinions Wanted: New asm Comments

I have a patch I'd like to commit that adds commentary to asm files
about which TableGen pattern generated a particular instruction. The
output looks like this:

  cvtpd2ps %xmm0, %xmm0 # source.c:39
                                        # Src: (intrinsic_wo_chain:v4f32 927:iPTR, VR128:v2f64:$src)
                                        # Dst: (Int_CVTPD2PSrr:v4f32 VR128:v2f64:$src)

This is enormously helpful when trying to track down codegen bugs but
clutters the asm file pretty badly for "ordinary" users.

Right now I have this under control of a separate -asm-pattern flag.
All other asm commentary is controlled by -asm-verbose.

Is a new flag appropriate for this or should I just put it under
-asm-verbose with everything else? If we want a new flag, does anyone
have a spelling they'd recommend? I'm not particularly fond of
-asm-pattern but it was the best concise name my uncreative brain could
conjur up.

                               -Dave

I wouldn't mind this sort of thing, but yes a separate option is definitely needed :slight_smile:

-eric

How do you plan to implement this? This is a compiler-hacker-only feature, can it be protected fully in !NDEBUG code?

-Chris

Chris Lattner <clattner@apple.com> writes:

Is a new flag appropriate for this or should I just put it under
-asm-verbose with everything else? If we want a new flag, does anyone
have a spelling they'd recommend? I'm not particularly fond of
-asm-pattern but it was the best concise name my uncreative brain could
conjur up.

How do you plan to implement this? This is a compiler-hacker-only
feature, can it be protected fully in !NDEBUG code?

The codegen portion certainly can be protected with !NDEBUG.

The change has two parts: a TableGen enhancement to output a table of
pattern strings and some code to pair up MachineInstrs with the
appropriate index into the pattern string table and an AsmPrinter
portion to actually print the comments.

The latter can be easily controlled via NDEBUG. The former is more
difficult since at the time TableGen is run we don't know how the rest
of the compiler will be built.

How would you prefer this work? Even if the output were controlled by
NDEBUG, I feel the added pattern comments make the asm file too
cluttered even for day-to-day compiler developers. This is really a
feature to debug instruction selection problems. That's why I put it
under the control of a separate option.

                               -Dave

It's not really clear how this should work. I'm primarily concerned that it will cause substantiate table/code bloat that doesn't make sense for a shipping compiler. I agree that not all compiler hackers will want to see it, I'd suggest adding a cl::opt that is only even available when built with assertions on.

-Chris

Chris Lattner <clattner@apple.com> writes:

How would you prefer this work? Even if the output were controlled by
NDEBUG, I feel the added pattern comments make the asm file too
cluttered even for day-to-day compiler developers. This is really a
feature to debug instruction selection problems. That's why I put it
under the control of a separate option.

It's not really clear how this should work. I'm primarily concerned
that it will cause substantiate table/code bloat that doesn't make
sense for a shipping compiler.

Ok, in that case I think we'll need a TableGen flag to control whether
this information gets generated. A debug build would include it, a
release build would not.

I agree that not all compiler hackers will want to see it, I'd
suggest adding a cl::opt that is only even available when built with
assertions on.

Assertions or Debug? I would think we'd want it in the latter.

                             -Dave

Chris Lattner <clattner@apple.com> writes:

How would you prefer this work? Even if the output were controlled by
NDEBUG, I feel the added pattern comments make the asm file too
cluttered even for day-to-day compiler developers. This is really a
feature to debug instruction selection problems. That's why I put it
under the control of a separate option.

It's not really clear how this should work. I'm primarily concerned
that it will cause substantiate table/code bloat that doesn't make
sense for a shipping compiler.

Ok, in that case I think we'll need a TableGen flag to control whether
this information gets generated. A debug build would include it, a
release build would not.

tblgen could unconditionally generate code that is protected by NDEBUG.

I agree that not all compiler hackers will want to see it, I'd
suggest adding a cl::opt that is only even available when built with
assertions on.

Assertions or Debug? I would think we'd want it in the latter.

Assertions (which are on by default in a debug build) is the knob we have to play with.

-Chris

Chris Lattner <clattner@apple.com> writes:

Ok, in that case I think we'll need a TableGen flag to control whether
this information gets generated. A debug build would include it, a
release build would not.

tblgen could unconditionally generate code that is protected by NDEBUG.

All right, that's doable.

I agree that not all compiler hackers will want to see it, I'd
suggest adding a cl::opt that is only even available when built with
assertions on.

Assertions or Debug? I would think we'd want it in the latter.

Assertions (which are on by default in a debug build) is the knob we
have to play with.

Ah, because of NDEBUG. Ok, makes sense. Not much of a loss for
Debug-Assertions. I doubt many people debug with it.

                               -Dave