[CodeGen] Fix for PR39094.

[HsiangKai]

Summary

When using MachineInstr to get SlotIndex, the MI could not be a debug
instruction. mi2iMap does not contain debug instructions in it.

After enabling DBG_LABEL in the generated code, the first instruction in the bundle may be a debug instruction. In this patch, I use the first non-debug instruction in the bundle to query SlotIndex in mi2iMap.

Bugzilla report: https://bugs.llvm.org/show_bug.cgi?id=39094

[aprantl]

a testcase would be nice, but generally this looks fine.

include/llvm/CodeGen/SlotIndexes.h

416–421

please clang-format your patch.

Does auto work here? It might make the code easier to read.

[HsiangKai]

In D52927#1256594, @aprantl wrote:

a testcase would be nice, but generally this looks fine.

From the bug report, the bug occurred in Greedy Register Allocation. The attachment in the bug report is a complex C file. I have no idea how to reduce the test case to reproduce the bug.

The bug also occurred in Chromium build. However, it is also a complex C file to trigger the bug.

Do you have any suggestions?

[aprantl]

The tools we have available for reducing large inputs are (multi-)delta (http://delta.tigris.org) and creduce (https://embed.cs.utah.edu/creduce/). Delta is a "dumb" implementation of the delta debugging algorithm (same one that is implemented in llvm-bugpoint) that is in my personal experience faster than creduce. I highly recommend spending a day to learn how to use delta; you will find yourself using it a lot :-)

[HsiangKai]

In D52927#1257868, @aprantl wrote:

The tools we have available for reducing large inputs are (multi-)delta (http://delta.tigris.org) and creduce (https://embed.cs.utah.edu/creduce/). Delta is a "dumb" implementation of the delta debugging algorithm (same one that is implemented in llvm-bugpoint) that is in my personal experience faster than creduce. I highly recommend spending a day to learn how to use delta; you will find yourself using it a lot :-)

I will try to use these tools. Thanks a lot :-)

[bjope]

Still no test case :-(

I think that without a test case it is hard to tell where the actual fault is here. Nor if this fix is the correct one.

Isn't the problem that someone puts the dbg.label inside a bundle? Should that be allowed?
Afaik lots of late passes see bundles as instructions (only iterating using bundle iterators and not looking inside bundles). So putting meta/debug-instructions inside bundles will just hide that instructions.

What would it mean to have the dbg.label between two instructions inside a bundle? Is it semantically equivalent to having the dbg.label before the bundle (or after)?

[HsiangKai]

I am sorry that I misunderstood the meaning of the status. I thought that if the status is ‘ACCEPTED’, I could upstream the commit.

I have verified that the commit could fix PR39094. It also passed LLVM and Clang regression tests. I assumed the patch will not broken LLVM. So, I upstreamed the patch.

According to the test case provided in PR39094, dbg.label is put as the first instruction in some bundles. So, it triggered the assertion failed when users use getBundleStart() to get SlotIndex for the queried MI. I am not familiar about instruction bundling. I do not know under what conditions instructions will be bundled together. So, I could not answer your questions clearly.

I understood that it is better to have a test case for the patch. I will send another patch to provide the test case if I could find one afterward. Thanks for your kindly reminding.

[bjope]

In D52927#1268933, @HsiangKai wrote:

I am sorry that I misunderstood the meaning of the status. I thought that if the status is ‘ACCEPTED’, I could upstream the commit.

That is absolutely normal process, so I don't think you did anything wrong really.
I'm, just sorry that I did not see this earlier. I happened to see the review now when it was committed, so I thought I could give some post-commit comments.
Maybe nobody has thought about what it means to have a dbg.label inside a bundle.

include/llvm/CodeGen/SlotIndexes.h

public:
     /// otherwise returns false.
     bool hasIndex(const MachineInstr &instr) const {
       return mi2iMap.count(&instr);
     }
 
     /// Returns the base index for the given instruction.
     SlotIndex getInstructionIndex(const MachineInstr &MI) const {
       // Instructions inside a bundle have the same number as the bundle itself.
-      const MachineInstr &BundleStart = *getBundleStart(MI.getIterator());
-      assert(!BundleStart.isDebugInstr() &&
+      auto BundleStart = getBundleStart(MI.getIterator());
+      auto BundleEnd = getBundleEnd(MI.getIterator());
+      // Use the first non-debug instruction in the bundle to get SlotIndex.
+      const MachineInstr &BundleNonDebug =
+          *skipDebugInstructionsForward(BundleStart, BundleEnd);
+      assert(!BundleNonDebug.isDebugInstr() &&
              "Could not use a debug instruction to query mi2iMap.");
-      Mi2IndexMap::const_iterator itr = mi2iMap.find(&BundleStart);
+      Mi2IndexMap::const_iterator itr = mi2iMap.find(&BundleNonDebug);
       assert(itr != mi2iMap.end() && "Instruction not found in maps.");
       return itr->second;
     }
 
     /// Returns the instruction for the given index, or null if the given
     /// index has no instruction associated with it.
     MachineInstr* getInstructionFromIndex(SlotIndex index) const {
       return index.isValid() ? index.listEntry()->getInstr() : nullptr;