[PATCH] D17356: Fix PR26655: Bail out if all regs of an inst BUNDLE have the correct kill flag

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a reviewer: MatzeB.
MatzeB added a comment.
This revision is now accepted and ready to land.

In http://reviews.llvm.org/D17356#405435, @mgrang wrote:

>




> So I ran the entire set of correctness tests in our validation framework without the check for isInternalRead() and I did not see any extra regressions. Also there are no extra unit test failures without this check. So I guess the check for isInternalRead() is really not   needed and can be safely removed.


You have to be carefull here, I think there are just no in-tree targets at the moment that use bundles before register allocation. Having said that I looked at the code and I believe it was checking the first instruction of a bundle only, the first instruction is the BUNDLE summary instruction and should not have any internal reads anyway so with this reasoning it should be fine to ignore it.

I believe the check for DebugValue nodes can be done outside (see below). With the issues below addressed this looks good to me.


================
Comment at: lib/CodeGen/MachineInstr.cpp:1944-1950
@@ -1943,2 +1943,9 @@
       continue;
+
+    // DEBUG_VALUE nodes do not contribute to code generation and should
+    // always be ignored. Failure to do so may result in trying to modify
+    // KILL flags on DEBUG_VALUE nodes.
+    if (MO.isDebug())
+      continue;
+
     unsigned Reg = MO.getReg();
----------------
Why not check `MI.isDebugValue()` at the caller?

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:1200
@@ -1200,1 +1199,3 @@
+                                 bool NewKillState,
+                                 const TargetRegisterInfo *TRI) {
   if (MI->getOpcode() != TargetOpcode::BUNDLE)
----------------
Use a reference to pass `TRI` as it must not be nullptr.

================
Comment at: test/CodeGen/Thumb2/thumb2-cpsr-liveness.ll:1
@@ +1,2 @@
+; RUN: llc < %s -mtriple=thumbv7-linux-gnueabi -misched-postra=true
+
----------------
It would be nice to add a comment on what the test is testing.


Repository:
  rL LLVM

http://reviews.llvm.org/D17356