<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Jul 8, 2013, at 11:49 AM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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><br class="Apple-interchange-newline">On Jul 8, 2013, at 11:02 AM, Jakob Stoklund Olesen <<a href="mailto:jolesen@apple.com">jolesen@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="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>On Jul 2, 2013, at 5:19 PM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:<br><br><blockquote type="cite">For a call site with struct byval, we will generate a sequence of instructions: STACKDOWN, STRUCT_BYVAL and STACKUP.<br>STRUCT_BYVAL later on is expanded to a for loop. This caused issue in PEI when trying to eliminate frame indices.<br><br>BB1:<br>STACKDOWN<br>BB2:<br>memcpy<br>BB3:<br>STACKUP<br><br>PEI currently assumes SPAdj is 0 at the beginning of a basic block, but this is not true for BB2 or BB3. The proposed patch tries<br>to fix the problem by inserting SET_SP_ADJ in BB2 and BB3:<br>BB1:<br>STACKDOWN<br>BB2:<br>SET_SP_ADJ<br>memcpy<br>BB3:<br>SET_SP_ADJ<br>STACKUP<br><br>A target-independent opcode SET_SP_ADJ is introduced, and pseudo instruction STRUCT_BYVAL is modified to take one extra<br>argument (the amount of SP Adjustment). When we are expanding STRUCT_BYVAL to a for loop, we add SET_SP_ADJ to BB2 and BB3.<br><br>PEI is also modified to handle SET_SP_ADJ. To make sure SET_SP_ADJ is always before any spill code that references frame indices,<br>SET_SP_ADJ is treated as a label.<br><br>SET_SP_ADJ can be useful for other targets when STACKDOWN and STACKUP are not paired up in the same basic block.<br><br>Comments and other suggestions are welcome.<br></blockquote><br>Hi Manman,<br><br>I am a bit worried that these solutions are too fragile. Every pass that changes the machine CFG needs to know how to preserve the SET_SP_ADJ instructions or MBB::StackAdjustment property. This is likely to break because SET_SP_ADJ is used so rarely.<br></div></blockquote>Yes it is a bit fragile.</div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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><blockquote type="cite"><div style="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>How about teaching PEI that STACKDOWN/STACKUP pairs don’t have to be in the same basic block? PEI could verify that:<br><br>- On every path through the CFG, a STACKDOWN <n> is always followed by a STACKUP <n> instruction.<br>- STACKDOWN/STACKUP pairs are never nested.<br>- Stack adjustments are identical on all CFG edges to a merge point.<br><br>This could be implemented by having PEI traverse the CFG in a DFS order, keeping track of the stack state at the end of every basic block on the DFS stack.<br></div></blockquote><div><br></div>Theoretically, this should work. But there are some corner cases in PEI which may break this.</div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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;">Jim, do you have any opinion?</div></blockquote><div dir="auto"><br></div><div dir="auto">I haven’t followed closely enough to know all the recent details, but I don’t like the idea of having a property on MachineBasicBlock. I much prefer the pseudo-instruction to that.</div><div dir="auto"><br></div><div dir="auto">My only hesitation about teaching PEI to do a CFG walk is that it’s adding a non-trivial bit of complexity, especially in the presence of crazy CFGs with computed-gotos and such in them. In theory, those shouldn’t interact with stack adjustments, but in practice?</div><div dir="auto"><br></div><div dir="auto">The only bit I feel strongly about is that I agree we should have something that tries to verify those three invariants.</div><div dir="auto"><br></div><div dir="auto">-Jim</div></div></body></html>