Page MenuHomePhabricator

[MachineCopyPropagation] Fix differences in code gen when compiling with -g
ClosedPublic

Authored by predator5047 on Jun 16 2021, 8:59 AM.

Details

Summary

Fixes bugs 50580 and 49446

When compiling with -g "DBG_VALUE <reg>" instructions are added in the MIR, if such a instruction is inserted between instructions that use <reg> then MachineCopyPropagation invalidates <reg> , this causes some copies to not be propagated and causes differences in code generation (ex bugs 50580 and 49446 ). DBG_VALUE instructions should be ignored since they don't actually modify the register.

Diff Detail

Event Timeline

predator5047 created this revision.Jun 16 2021, 8:59 AM
predator5047 requested review of this revision.Jun 16 2021, 8:59 AM
lattner resigned from this revision.Jun 16 2021, 1:23 PM

I'm not a competent reviewer for this patch.

lattner removed a subscriber: lattner.
lkail edited reviewers, added: jmorse, condy, qcolombet, efriedma; removed: lattner.Jun 17 2021, 4:51 AM
lkail added a subscriber: lkail.Jun 17 2021, 4:54 AM

There is a patch https://reviews.llvm.org/D98401 doing the same. As discussed in D98401, the register in DBG_VALUE should be also updated. I think backward mcp is missing tracking usage when MO.isDebug() like forward mcp does.

predator5047 abandoned this revision.Jun 18 2021, 10:29 AM

@predator5047 This patch is worth pursuing -- D98401 involves codegen alterations and will necessarily take a long while to review; wheras this patch could land before llvm-13.

This patch is obviously effective at preventing codegen from being changed by debug-info; the downside is that the variable location is wrong afterwards (DBG_VALUE $rcx in the CHECK lines). I'm fairly confident we would prefer to have a false variable location over unstable code generation so it could land as-is; @aprantl @probinson @dblaikie would you agree?

To get the variable location right, you need to populate the CopyDbgUsers container, and re-write them when propagateDefs deletes the copy.

predator5047 reclaimed this revision.Jun 21 2021, 5:24 AM

@jmorse OK, I will reclaim this revision and make the changes you suggested.

lkail added a comment.Jun 28 2021, 2:38 AM

Reverse ping. Hi @predator5047 are you still working on this? One solution might be tracking debug instruction like what MachineCopyPropagation::ReadRegister does.

Update debug instructions if a register is changed because of backward propagation.

lkail added inline comments.Jun 28 2021, 4:17 AM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
829

This looks unnecessary, can use findCopyDefViaUnit to find out corresponding MachineInstr.

872–873

nit:

if (MO.readsReg()) {
  if (MO.isDebug()) {
    ...
  } else {
    ...
  }
}
894

nit: OldR -> Def, NewR -> Src for consistency.

predator5047 added inline comments.Jun 28 2021, 7:25 AM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
829

findCopyDefViaUnit or ReadRegister don't work for backwards propagation, because Tracker.trackCopy sets the CopyInfo::MI filed for the source register to null (line 167), so both would always return null.

lkail added inline comments.Jun 28 2021, 8:14 AM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
876
-      if (MO.readsReg())
-        Tracker.invalidateRegister(MO.getReg().asMCReg(), *TRI);
+      if (MO.readsReg()) {
+        if (MO.isDebug()) {
+          for (MCRegUnitIterator RUI(MO.getReg(), TRI); RUI.isValid(); ++RUI) {
+            if (MachineInstr *Copy = Tracker.findCopyDefViaUnit(*RUI, *TRI)) {
+              CopyDbgUsers[Copy].insert(MI);
+            }
+          }
+        } else {
+          Tracker.invalidateRegister(MO.getReg().asMCReg(), *TRI);
+        }
+      }

Doesn't it work?

Implemented lkail suggestions and addressed his comments.

predator5047 marked 4 inline comments as done.Jun 28 2021, 9:05 AM
lkail accepted this revision.Jun 28 2021, 9:10 AM

LGTM, thanks for your patience. May be @jmorse has further comments.

This revision is now accepted and ready to land.Jun 28 2021, 9:10 AM
lkail added a comment.Fri, Jul 2, 3:40 AM

Hi @predator5047 do you need help landing this patch?

Yes, can you commit on my behalf.
name: Butiu Alexandru Octavian
email: alexandru.octavian.butiu at gmail.com