MIR YAML TracksRegLiveness default to true in yaml parser

[bogner]

Why? (As in, please update the commit message to explain why)

[plotfi]

In D47999#1132075, @bogner wrote:

Why? (As in, please update the commit message to explain why)

Ah sorry. I think I had the comment in my diff and forgot to add it to the summary box.

Updated.

[qcolombet]

Hi Puyan,

If that property is required for proper processing, why don't we print it in the mir file?

Adding an option to force that sounds wrong to me.

Cheers,
-Quentin

[plotfi]

Ah yeah, I like that suggestion. Do you think it would be better to add the property by default rather than have a flag?
So a lot of the mir tests don't have this line added but if we added it by default a simple cat foo.mir | llc -run-pass=none -x mir -o - could add the "tracksRegLiveness: true"
property that is needed to do the full pipeline. Or maybe not having "tracksRegLiveness:" at all should imply "tracksRegLiveness: true" ?

I think it would be good to discuss probably what the sane defaults should be here.

PL

In D47999#1133723, @qcolombet wrote:

Hi Puyan,

If that property is required for proper processing, why don't we print it in the mir file?

Adding an option to force that sounds wrong to me.

Cheers,
-Quentin

[qcolombet]

Either changing the default (trackLiveness == true by default) or printing it is fine.

The odd thing is I would have expected the right printing/default to happen. Do you know when this property is set during the regular pipeline? (i.e., why stop-after/before)

[plotfi]

In D47999#1135278, @qcolombet wrote:

Either changing the default (trackLiveness == true by default) or printing it is fine.

The odd thing is I would have expected the right printing/default to happen. Do you know when this property is set during the regular pipeline? (i.e., why stop-after/before)

I don't recall off the top of my head. Will investigate.

[MatzeB]

From what I remember our use of this flag is strange:

My understanding of the flag is that it indicates that the live-in lists are up-to-date. Which only has a meaning post regalloc. Yet we always set the flag at the beginning when a MachineFunction is created and just happen to clear it for the targets that don't track liveness past regalloc (which should be close to noone nowadays). The flag keeps being very confusing to people IMO as it doesn't really tell you about other liveness tracking mechanisms like LiveRegister or LiveIntervals.

Either way: This patch is a strange solution, I'd much rather see us do some cleanup in the way MI handles it. You could try keeping the flag disabled by default and only set if in the regalloc pass for the post-ra passes...

[plotfi]

In D47999#1135607, @MatzeB wrote:

From what I remember our use of this flag is strange:

My understanding of the flag is that it indicates that the live-in lists are up-to-date. Which only has a meaning post regalloc. Yet we always set the flag at the beginning when a MachineFunction is created and just happen to clear it for the targets that don't track liveness past regalloc (which should be close to noone nowadays). The flag keeps being very confusing to people IMO as it doesn't really tell you about other liveness tracking mechanisms like LiveRegister or LiveIntervals.

Either way: This patch is a strange solution, I'd much rather see us do some cleanup in the way MI handles it. You could try keeping the flag disabled by default and only set if in the regalloc pass for the post-ra passes...

So in the short term: leave the default for the flag in place, and have have the reg-alloc pass set it to true by default?

[MatzeB]

IMO MachineFunction::init() should not set TracksLiveness by default and instead VirtRegMap / FastRegAlloc should set it once it actually filled in the basic block live-in lists.

Could you check what happens when you do that? (I suspect you will be hitting an assert() somewhere or you wouldn't have bothered with this patch in the first place... Hopefully we can just fix that assert).

[MatzeB]

Looking around the code I would guess it just works. You should however also remove the && MRI.tracksLiveness() from MachineBasicBlock::print() if you change the default.

(This is because we do live-in lists for two purposes right now: Marking exception handling registers popping up on CFG paths and tracking complete liveness post regalloc. Because of the first use case we should always print the live-in lists I think).

[plotfi]

I've updated this to a much simpler diff. I think instead of changing when we set this stuff in the MachineIntrs before regalloc maybe the simplest thing to do is make it so that absence of "tracksRegLiveness" in the MIR Yaml section implies "tracksRegLiveness: true" @MatzeB What do you think?

[MatzeB]

I still think we should rather try to not set TracksRegLiveness early in the codegen pipeline instead. Let me try how well that works...

[MatzeB]

In D47999#1162130, @MatzeB wrote:

I still think we should rather try to not set TracksRegLiveness early in the codegen pipeline instead. Let me try how well that works...

After looking around the code some more, I realized again that we also use livein lists to annotate the result of exception handling live-in or (according to some comments at least) flag registers being live accross blocks. From that perspective it is fine to start out a MachineFunction as TracksRegLiveness==true and also make this the MIR default.

test/CodeGen/MIR/X86/force-liveness-track.mir

3–12

Just check explicitely for tracksRegLiveness: True in the output (unless -simplify-mir is used the printer will print everything even if it has default values, so the value should exist in the output).

include/llvm/CodeGen/MIRYamlMapping.h

     YamlIO.mapOptional("regBankSelected", MF.RegBankSelected, false);
     YamlIO.mapOptional("selected", MF.Selected, false);
     YamlIO.mapOptional("failedISel", MF.FailedISel, false);
-    YamlIO.mapOptional("tracksRegLiveness", MF.TracksRegLiveness, false);
+    YamlIO.mapOptional("tracksRegLiveness", MF.TracksRegLiveness, true);
     YamlIO.mapOptional("registers", MF.VirtualRegisters,
                        std::vector<VirtualRegisterDefinition>());
     YamlIO.mapOptional("liveins", MF.LiveIns,

test/CodeGen/MIR/X86/force-liveness-track.mir

+# RUN: llc -o - %s -mtriple=x86_64 | FileCheck %s
+---
+# CHECK: xorl  %eax, %eax
+# CHECK: movl  %eax, (%rcx)
+# CHECK: retq
+name: func0
+body: |
+  bb.0:
+    %0 : gr32 = MOV32r0 implicit-def $eflags
+    dead %1 : gr32 = COPY %0
+    MOV32mr undef $rcx, 1, _, 0, _, killed %0 :: (volatile store 4)
+    RETQ undef $eax
+...