[PATCH] D38616: [RegisterCoalescer] Don't set read-undef if there is a previous def

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks for the patch and the testcase, this looks good.

I slightly simplified the testcase to this while trying out the patch:

  # RUN: llc -mtriple=x86_64-- %s -o - -run-pass=simple-register-coalescing | FileCheck %s
  ---
  name: f
  body: |
    bb.0:
      JB_1 %bb.2, undef implicit killed %eflags
      JMP_1 %bb.1
  
    bb.1:
      %0 : gr64 = IMPLICIT_DEF
      NOOP implicit-def undef %1.sub_32bit : gr64
      NOOP implicit-def %1.sub_16bit : gr64
      JMP_1 %bb.3
  
    bb.2:
      NOOP implicit-def %0
      %1 = COPY %0
  
    bb.3:
      NOOP implicit killed %0
      NOOP implicit killed %1
  ...
  
  # We should have a setting of both sub_32bit and sub_16bit. The first one
  # should be undef and not dead, and the second should not be undef.
  
  # CHECK-NOT:  dead
  # CHECK:      NOOP implicit-def undef %1.sub_32bit
  # CHECK-NOT:  undef
  # CHECK-NEXT: NOOP implicit-def %1.sub_16bit



================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:2688-2692
               if (MO.getSubReg() != 0)
-                MO.setIsUndef(EraseImpDef);
+                // Only set undef on the MO if we can't find any reaching
+                // definition of the value. If it does exist and we set undef on
+                // the MO we will effectively kill the reaching definition.
+                MO.setIsUndef(EraseImpDef && !LR.getVNInfoBefore(Def));
----------------
I wonder if we shouldn't change it to this instead which:
```
              if (MO.getSubReg() != 0 && MO.isUndef() && !EraseImpDef)
                MO.setIsUndef(false);
```
it's simpler, matches the comment in front of the for and it seems to pass the x86 tests at least...


https://reviews.llvm.org/D38616