<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 9, 2016, at 11:11 AM, Matthias Braun <<a href="mailto:matze@braunis.de" class="">matze@braunis.de</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Aug 9, 2016, at 10:03 AM, Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Aug 8, 2016, at 6:06 PM, Matthias Braun <<a href="mailto:matze@braunis.de" class="">matze@braunis.de</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="Apple-interchange-newline">On Aug 8, 2016, at 5:53 PM, Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class=""><br class="">Hi Matthias,<br class=""><br class="">I haven’t read all the discussion but I think I read enough to chime in.<br class="">I would like to keep the AllVRegsAllocated property.<br class=""><br class="">I’m fine that the parser recomputes it based on the number of virtual registers, but I do not want it to be tight to that information dynamically. I believe part of the problem is in the naming of the property and the semantic attached to it.<br class="">This property means that:<br class="">1. No virtual registers are present (the dynamic check capture that)<br class="">2. Passes cannot introduce new VRegs (the name does not convey that, but the dynamic check would lose that information)<br class=""><br class="">Regarding your example:<br class="">file1.mir:<br class="">...<br class="">name: foo<br class="">AllVRegsAllocated: true<br class="">body: |<br class=""> bb.0:<br class=""> RETQ<br class="">---<br class=""><br class=""><br class=""><br class="">file2.mir<br class="">...<br class="">name: foo<br class="">AllVRegsAllocated: false<br class="">body: |<br class=""> bb.0:<br class=""> RETQ<br class="">—<br class=""><br class="">This are semantically different in the sense that a pass working on that input could performance differently based on the value of the property. E.g., with AllVRegsAllocated == false, it could introduce vregs, whereas with AllVRegsAllocated == true, it would run the scavenger before exiting. If we make the property dynamic, the pass won’t see it has to scavenge the vregs it introduced.<br class=""></blockquote><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">But if we imagine such a situation (today we don't AFAIK) with pass "FooBar", then wouldn't it be simpler to have "FooBarBeforeRegAlloc" and "FooBarAfterRegAlloc" and push that decision on the code setting up the pass pipeline instead of the "FooBar" pass switching modes based on a property in the MI?</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"></div></blockquote><div class=""><br class=""></div><div class="">That already happens with passes like MachineLICM that performs differently before or after regalloc. Sure we could have a different mode for the pass that we set in the pipeline, though I like the property in particular for the MachineVerifier that gets added automatically. We may change how it is added so that it knows where it is in the pipeline, but as we add more properties it does not scale.</div></div></div></blockquote><div class="">I don't see why this wouldn't scale. You can always add more parameters to the pass constructor if you want to avoid creating complete new subclasses for different situations.</div></div></div></blockquote><div><br class=""></div>That’s the problem, you then get more and more parameters for different properties. Like isAfterLegalization, isAfterRegBankSelect, isAfterISel.</div><div>The properties abstract us away and the passes do what they want.</div><div>I kind of like the idea of having that being parameters but where it does not scale is for the MachineVerifier. Again that thing is added automatically and feeding the proper parameters to the pass seems complicated in the current scheme.<br class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class=""><br class=""></div><div class="">In fact I think we should change MachineLICM to not rely on the isSSA property to detect whether it runs before or after regalloc. We should rather have a switch in the constructor that configures that.</div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""><blockquote type="cite" class=""><div class=""><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">The later style can already be seen in the MachineScheduler/PostMachineScheduler which both use the same base class but are specialized into two independent passes.</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="">Again, I believe this is a naming problem, but I don’t have a great naming, CanUseVRegs maybe?<br class=""></blockquote><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">It's not even "CanUseVRegs" because we actually can and do use: Most frame lowering code creates temporary vregs (that are replaced by physregs in PrologueEpilogueInserter) so all this means is "NoVRegs between passes" (and for that see above for splitting into different passes).</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"></div></blockquote><div class=""><br class=""></div><div class="">That’s what I meant. I am fine with such name.</div><br class=""><blockquote type="cite" class=""><div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="">Also, staying on that example, what would be the default value in that case?<br class=""></blockquote><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Again I believe this to be a property of scheduling passes correctly not a property of the MachineIR. If there is no semantic difference in the two functions above then it would be best if they are the same.</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">If anything we would need this to be a property of the passes and have some pass verification layer on top that checks the correct order of the passes. However I personally find the assert()s strong enough to catch typical errors here so we do not need extra machinery…</span></div></blockquote><div class=""><br class=""></div><div class="">We already have this “extra” machinery… I don’t see what you do not like with that.</div><div class="">Also if you go in that direction, then isSSA shouldn’t be a property as well we can recompute it as well, but again I believe those are two different goal and I do like the property machinery.</div></div></div></blockquote><div class=""><br class=""></div></div><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">This is exactly why I have another patch out there that does not print or parse isSSA in .mir files but computes it instead.</span><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">I think of isSSA as a cached analysis result. I can always recompute the information but checking the bool field and keeping it up to date is faster than checking all vregs for the SSA property in multiple time throughout codegen.</div></div></blockquote><div><br class=""></div><div>Same here, even if you get isSSA == yes, that does not mean you want to preserve that. E.g., if the function is just a BB and it happens to be in SSA.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""></div><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">I can live with the machinery. What I find annoying/wrong with CanUseVRegs is that we have a property in the MI that has no semantic meaning at all, it seems to be purely a configuration setting for the passes we run on a given MI function.</div></div></div></blockquote><div><br class=""></div><div>At least, it has a semantic for me :).</div><br class=""><blockquote type="cite" class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">- Matthias</div></blockquote></div><br class=""></body></html>