<div dir="ltr"><div dir="ltr">On Mon, Jan 27, 2020 at 4:31 PM Eli Friedman <<a href="mailto:efriedma@quicinc.com">efriedma@quicinc.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">





<div lang="EN-US">
<div class="gmail-m_-3639985746653032053WordSection1">
<p class="MsoNormal">I assume by “drop support”, you mean reject it in the bitcode reader/IR parser?  We can’t reasonably support a complex feature like inalloca if nobody is testing it. If we can’t reasonably upgrade it, and we don’t think there are any users
 other than clang targeting 32-bit Windows, probably dropping support is best.</p></div></div></blockquote><div><br></div><div>That's a good point. There are already enough lightly tested features in LLVM. There's no reason to leave another one lying around like a trap for the first unsuspecting user to try it.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_-3639985746653032053WordSection1">
<p class="MsoNormal">More details comments on the proposal:<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">“llvm.call.setup must have exactly one corresponding call site”: Normal IR rules would allow cloning the call site (in jump threading), or erasing the call site (if there’s a noreturn call in an argument).  What’s the benefit of enforcing
 this rule, as opposed to just saying all the call sites must have the same signature?</p></div></div></blockquote><div> </div><div>I think we could cope with unreachable code elimination deleting a paired call site (zero or one), but code duplication creating a second call site could be problematic. The call setup doesn't describe the prototype of the main call site, so if there were multiple call sites, the backend would have to pick one call site arbitrarily or compare the call sites when setting up the call. If there are zero call sites, the backend can create static allocas of the appropriate type to satisfy the allocations. Of course, an IR pass (instcombine?) should do this transform first if it sees it. Maybe we could have CGP take care of it, too.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_-3639985746653032053WordSection1"><p class="MsoNormal"><u></u></p>
<p class="MsoNormal">The proposal doesn’t address what happens if llvm.call.setup is called while there’s another llvm.call.setup still active.  Is it legal to call llvm.call.setup in a loop?  Or should nested llvm.call.setup calls have the parent callsetup
 token as an operand?</p></div></div></blockquote><div><br></div><div>Nested setup is OK, but the verifier rule that there must be a paired call site should make it impossible to do in a loop. I guess we should have some rule to reject the following:</div><div>%cs1 = llvm.call.setup()</div><div>%cs2 = llvm.call.setup()</div><div>call void @cs1() [ "callsetup"(token %cs1) ]</div><div>call void @cs2() [ "callsetup"(token %cs2) ]</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_-3639985746653032053WordSection1">
<p class="MsoNormal">Is there some way we can allow optimizations if we can’t modify the callee, but we can prove nothing captures the address of the preallocated region before the call?  I guess under the current proposal we could transform preallocated->byval,
 but that isn’t very exciting.</p></div></div></blockquote><div><br></div><div>I suppose we could say that the combo of byval+preallocated just means `byval`, and teach transforms that that's OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_-3639985746653032053WordSection1">
<p class="MsoNormal">How does this interact with other dynamic stack allocations?  Should we switch VLAs to use a similar mechanism?  (The problems with dynamic alloca in general aren’t as terrible, but it might still benefit: for example, it’s much easier
 to transform a dynamic allocation into a static allocation.)</p></div></div></blockquote><div><br></div><div>VLAs could use something like this, but they are generally of unknown size while call sites have a known fixed size. I think that makes them pretty different.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_-3639985746653032053WordSection1">
<p class="MsoNormal">“If an exception is thrown and caught within the call setup region, the newly established SP must be saved into the EH record when a call is setup.”  What makes this case special vs. what we currently implement?  Is this currently broken? 
 Or is it related to supporting frame pointer elimination?</p></div></div></blockquote><div><br></div><div>I think of it as a special case because you can't write this in standard C++. Today, I think we leak stack memory in this case. There's no correctness issue because we copy SP into its own virtual register at the point of the alloca, and arguments are addressed relative to the vreg. What I had in mind for the new system is that we make some kind of fixed stack object that uses pre-computed SP offsets, assuming there are no dynamic allocas in the function. This would be a problem for a program that does:</div><div><br></div><div>setup call 1</div><div>store call 1 arg 0</div><div>try { </div><div>  setup call 2 </div><div>  throw exception</div><div>  call 2</div><div>} catch (...) {}</div><div>; call 2's frame is still on the stack</div><div>store call 1 arg 1 ; SP offset would be incorrect<br></div><div>call 1</div></div></div>