This is an archive of the discontinued LLVM Phabricator instance.

debug output incorrect ["UNKNOWN"] when a MachineInstr is generated during the MachineCombiner pass and the relevant debug output is enabled
ClosedPublic

Authored by sebpop on Dec 9 2016, 5:05 PM.

Details

Summary

Some of the LLVM debug output is incorrect ["UNKNOWN"] when a MachineInstr is generated during the MachineCombiner pass and the relevant debug output is enabled.

The fix is generic, but the test case [based on machine-reduced Eigen3 C++ code] is AArch64-specific.

Debug output before the fix:

NEW INSTR %vreg15<def> = UNKNOWN %vreg1, %vreg13<kill>, %vreg11<kill>

Debug output after the fix:

NEW INSTR %vreg15<def> = MADDXrrr %vreg1, %vreg13<kill>, %vreg11<kill>

I [Abe] searched hard for a less-kludgy way to accomplish this, but did not find one.

Diff Detail

Event Timeline

Abe updated this revision to Diff 80982.Dec 9 2016, 5:05 PM
Abe retitled this revision from to debug output incorrect ["UNKNOWN"] when a MachineInstr is generated during the MachineCombiner pass and the relevant debug output is enabled.
Abe updated this object.
Abe updated this object.Dec 9 2016, 5:05 PM
Abe updated this object.Dec 9 2016, 5:09 PM
Gerolf added inline comments.Dec 9 2016, 5:28 PM
llvm/include/llvm/CodeGen/MachineInstr.h
1157

How about "Dumper that prevents ..."

1159

Why not overlay dump(...)? At least use naming convention eg. dumpWithGiven...

llvm/lib/CodeGen/MachineInstr.cpp
1705

This can be simpler: use default initializer third parameter TTI = nullptr

1718

if (!TII) TII = ...

1746

The entire statement can be removed

1999

use TTI rather then TTI_in

Abe updated this revision to Diff 80989.Dec 9 2016, 6:29 PM

Integrated some good suggestions from Gerolf, thereby making the fix less kludgy.

echristo edited edge metadata.Dec 9 2016, 6:33 PM

I'd suggest formatting with clang-format as well.

sebpop added a subscriber: sebpop.Dec 12 2016, 7:57 AM
sebpop added inline comments.
llvm/test/CodeGen/AArch64/correct_debug_output_when_a_new_MachineInstr_is_generated_in_the_machine_combiner.ll
1

Please replace this testcase with the one that I reduced for the other instr-combiner patch:
llvm/test/CodeGen/AArch64/machine-combiner-madd.ll

Also, please use reasonably shorter file names.

5

Restrict the flags of the run command to a minimum. Please see the RUN command in the above mentioned ll file.

Abe updated this revision to Diff 81105.Dec 12 2016, 10:23 AM
Abe edited edge metadata.
Abe marked 2 inline comments as done.

Implemented some recommendations/suggestions from Sebastian.

Abe updated this revision to Diff 81108.Dec 12 2016, 10:47 AM

Incorporated Sebastian`s further reduction of the new test case.

Abe updated this revision to Diff 81111.Dec 12 2016, 11:12 AM

Now using "= nullptr" instead of "= 0" in one spot.

Gerolf added inline comments.Dec 12 2016, 11:40 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1160

Target... *const vs * const?

llvm/lib/CodeGen/MachineInstr.cpp
1705

Shouldn't it be ...Target... *const vs * const?

When TTI_in is not nullptr *and* TII actually does get defined in the function, how would TTI_in be different from TII?

Also TTI_in is declared differently from TII? Why?

Abe updated this revision to Diff 81126.Dec 12 2016, 12:35 PM

Minor changes in source code formatting, editing according to "clang-format" output [ran with "-style=LLVM"].

Abe updated this revision to Diff 81266.EditedDec 13 2016, 11:33 AM
Abe marked 2 inline comments as done.

Changed the interface for the new "MachineInstr::print(...)" overload as Gerolf requested.

Abe updated this revision to Diff 81268.Dec 13 2016, 11:40 AM

Trivial edit; NFC WRT prev. diff in same patch.

Gerolf added inline comments.Dec 13 2016, 1:02 PM
llvm/lib/CodeGen/MachineInstr.cpp
1724

I think this confirms that you want to initialize the TII with the nullptr.
Then remove const .. *TII in line 1710.
Finally you can remove if (TTI_).

Abe updated this revision to Diff 81292.Dec 13 2016, 1:17 PM

fixed a bug Gerolf caught

sebpop commandeered this revision.Dec 20 2016, 9:56 AM
sebpop added a reviewer: Abe.

Taking over this patch: I will update the patch addressing all the comments from Gerolf.

sebpop updated this revision to Diff 82118.Dec 20 2016, 9:59 AM
sebpop marked an inline comment as done.

Fixed the problems that Gerolf has asked to be addressed, and reduced the number of changes.

Gerolf edited edge metadata.Dec 20 2016, 11:05 AM

Thanks! LGTM.

This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Jan 20 2017, 4:47 PM

With this change in place the a typical lldb operation fails now:

lldb > p I->dump()
error: too few arguments to function call, expected 1, have 0

It looks like lldb is not able to put in the default value. However as this can be really annoying for developers and seems unnecessary, can we revert the changes to the dump() function and only keep the extra parameter on the print() function? (MachineCombiner.cpp really should be chaned to the simpler dbgs() << *InstrPtr instead of using the debug helper dump())

sebpop added inline comments.Jan 20 2017, 5:51 PM
llvm/trunk/include/llvm/CodeGen/MachineInstr.h
1156 ↗(On Diff #82186)

If you don't pass TII to dump() it should just get nullptr as a default value.

With this change in place the a typical lldb operation fails now:

lldb > p I->dump()
error: too few arguments to function call, expected 1, have 0

I suspect this is an error in lldb. Please try with gdb to confirm.

With this change in place the a typical lldb operation fails now:

lldb > p I->dump()
error: too few arguments to function call, expected 1, have 0

I suspect this is an error in lldb. Please try with gdb to confirm.

Getting a working gdb version on macOS is hard (last time I tried, I could not get code signing to work in a way that allowed gdb to actually debug processes). So I cannot really test.
Even though this is clearly a deficiency in lldb, using MI.dump() in a debugger is such a common operation, that I'd rather have the dump() function changed.