<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 Oct 27, 2016, at 5:30 PM, Matthias Braun <<a href="mailto:mbraun@apple.com" class="">mbraun@apple.com</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 Oct 27, 2016, at 5:05 PM, Andrew Trick <<a href="mailto:atrick@apple.com" class="">atrick@apple.com</a>> wrote:</div></blockquote><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">The system works because the default basic block iterator moves from bundle to<br class="">bundle skipping the instructions inside the bundle. Iterating over the operands<br class="">will only give us the operands of the BUNDLE instruction but that is fine,<br class="">because it basically has a copy of everything inside the bundle.<br class=""></blockquote><br class="">The BUNDLE instruction simply isn’t necessary to do anything you just described.<br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">That may all be true. However I'd like to point out that this is the status quo! finalizeBundle() will give you the BUNDLE instruction in the header and it is used by everyone using bundles: ARM, and AMDGPU target and the DFAPacketizer (which is used by Hexagon).</div><div class=""><br class=""></div><div class="">Not using BUNDLE and correctly using MIBundleOperands at the right places in the register allocator is not where the code is today! I believe that we are far enough away from it that we should rather fix the status quo first to avoid all the confusion and then move forward to the header-less scheme in a targetted change. That is why I added the last paragraph in my mail.</div></div></div></blockquote><div><br class=""></div>I don’t understand. BUNDLE was never meant for vreg operands. Are you saying that regalloc breaks if you form preRA bundles (without “finalizing” them)?</div><div><br class=""></div><div>Incidentally, it’s fine to temporarily bundle, e.g. during scheduling, erase bundles afterward, and potentially rebundle again later. That works great if you don’t have to keep materializing BUNDLE instructions.</div><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=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class="">== Too many different iterators ==<br class=""><br class="">Another source of confusion even for experience register allocation developers<br class="">is that we have 3 kinds of iterators for MachineOperands:<br class=""><br class="">- There is MachineInstr::iterator which is used by the majority of passes and<br class=""> gives you the operands of a single instruction.<br class="">- There is (Const)MIOperands which appears to be equivalent to<br class=""> MachineInstr::iterator. I think we do not need a 2nd iterator and should get<br class=""> rid of this one (the only real reason to use it today is<br class=""> analyze{Virt|Phys}Reg() but that can be changed).<br class="">- There is (Const)MIBundleOperands which iterates all machine operands of all<br class=""> instructions inside a bundle.<br class=""></blockquote><br class="">A pass needs to know whether it’s cares about bundles or instructions.<br class="">I don’t understand how adding an extra BUNDLE instruction does anything to solve this problem or make the MIR more robust.<span class="Apple-converted-space"> </span><br class=""><br class="">A pass that cares about liveness, dependencies, instruction insertion or reordering needs to work on bundles.<br class="">Machine-independent passes should probably work on bundles.<br class=""><br class="">By default, passes now use the bundle iterator for instructions and non-bundle iterator for operands. That allows passes to limp along in the presence of bundles without actually handling the bundles. I think the bundles will just silently defeat optimizations. It’s not safe, but it’s not too badly broken either.<br class=""><br class="">The MIBundleOperands iterator simply makes more sense to me than the BUNDLE instruction. It seems straightforward to migrate passes to the new iterator, but it’s a lot of places that need updating.<br class=""></div></div></blockquote><div class="">I am convinced that as soon as we decide for a scheme with or without BUNDLE instruction we should remove all but one iterator (or at least write a long comment on the other iterator why you should not use it in most situation). Whatever the result it should use a C++ style iterator so at the very least MIBundleOperators needs to be rewritten for that.</div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">The last one appears to be necessary in a world without the initial BUNDLE<br class="">instruction repeating all the operands inside the bundle. In a setting where<br class="">finalization happens as a separate pass at the end of register allocation this<br class="">would be necessary for earlier register allocation passes.<br class=""><br class="">However given that delaying finalization to a pass appears broken/unused it<br class="">seems we could just as well use MachineInstr::iterator instead and remove<br class="">MIBundleOperands. Any objections?<br class=""></blockquote><br class="">IIUC, live intervals, the register allocator, and the scheduler already handle bundles.<br class=""><br class="">I’m fairly sure that adding new vreg uses is not what we want to do.<br class=""></div></div></blockquote><div class="">The code looks like it can handle it, but as I said above it is not exercised by any of the existing targets and I can show you some places where uses of MachineInstr::iterator sneaked in even in the regalloc passes which would be invalid in the BUNDLE-header-less scheme.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">== Moving to a scheme without repeating the operands in the bundle header ==<br class=""><br class="">I've heard some comments that the long term plan was to move to a scheme where<br class="">the operands inside the bundle are not repeated in a bundle header and instead<br class="">everyone uses an iterator like MIBundleOperands. I could not find any mails<br class="">documenting this, so it would be nice if some people could chime in here if<br class="">that was indeed the plan.<br class=""><br class="">Even with this long term plan in mind I would suggest to remove<br class="">MIBundleOperands. If we implement this plan we should rather change<br class="">MachineInstr::iterator later instead of being in the confusin in-between state<br class="">that we have today.<br class=""><br class="">- Matthias<br class=""></blockquote><br class="">I’m not sure what you mean by changing MachineInstr::iterator. You mean mop_iterator?<br class=""></div></div></blockquote><div class="">Oh sorry, I was talking about mop_iterator indeed. The thing you get when you use</div><div class=""> `for (MachineOperand &MO : someinstruction.operands()) { ... }` which is the standard for the majority of codegen passes today.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">You can’t replace an instr iterator with a bundle iterator without breaking some basic invariants:<br class="">MI == MI->operands_begin()->getParent()<br class=""><br class="">That’s why passes should explicitly ask for the bundle operands.<br class=""></div></div></blockquote>If we move to a BUNDLE-less world then the majority of passes will need something like MIBundleOperands, in that case we really should replace MachineInstr::iterator anyway, make the typical use the most convenient one and adapt the passes to not expect those invariants. But again I consider this a change of the status quo, not something we already do or just need to fix in a handful of places.</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=""><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><br class=""></div><div>It seems to me that machine-specific passes still want to iterate over per-instruction operands in a lot of cases.</div><div><br class=""></div><div>Of course, I haven’t looked at any of this code in a very long time. I can only explain the intent and you’ll have to use your judgement about migrating things.</div><div><br class=""></div><div>-Andy</div><br class=""></body></html>