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

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added a comment.

In https://reviews.llvm.org/D38616#893636, @MatzeB wrote:

> 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
>


Nice, I like that!

Thanks for the comments, I'll update the patch.



================
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));
----------------
MatzeB wrote:
> 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...
But this means we will never set isUndef, we will only clear it? So the stuff I talk about in the commit message that we sometimes need to introduce undef, if we've removed an IMPLICIT_DEF, isn't really true? In those cases maybe there would already be undef on the MO?

I suppose the simplified code works if
 (MO.getSubReg() != 0) && !MO.isUndef() && EraseImpDef && !LR.getVNInfoBefore(Def)
can never happen?

I ran a bunch of tests with 

              assert(!((MO.getSubReg() != 0) && !MO.isUndef() && EraseImpDef && !LR.getVNInfoBefore(Def)) &&
                     "Unexpected need of read-undef on subreg write!");
              if (MO.getSubReg() != 0 && MO.isUndef() && !EraseImpDef)
                MO.setIsUndef(false);

and I didn't see the assert blowing and everything went well so maybe it's indeed what we want to do.

I'll update the patch to your suggestion!


https://reviews.llvm.org/D38616