<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Anton,<div><br class="webkit-block-placeholder"></div><div>Going back to the list now that it's working. :)</div><div><br><div><div>On 2008-01-04, at 08:22, Anton Korobeynikov wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hello, Gordon.<br><br>Here goes quick review.<br><br><br><blockquote type="cite">+// Determines whether a CALL node uses struct return semantics.<br></blockquote><blockquote type="cite">+static bool CallIsStructReturn(SDOperand Op)<br></blockquote>I like these predicates, because later we can move them to the<br>autogenerated code.<br></blockquote><div><br class="webkit-block-placeholder"></div><div>Yes!</div><br><blockquote type="cite"><blockquote type="cite">+  // Decoarate the function name.<br></blockquote>Typo<br></blockquote><div><br class="webkit-block-placeholder"></div>Okay.</div><div><br><blockquote type="cite"><blockquote type="cite">+  if (Is64Bit && CC == CallingConv::X86_FastCall &&<br></blockquote><blockquote type="cite">+      !Subtarget->isTargetCygMing() && !Subtarget->isTargetWindows() &&<br></blockquote><blockquote type="cite">+      (StackSize & 7) == 0)<br></blockquote></blockquote>and</div><div><blockquote type="cite"><blockquote type="cite">+  if (!Is64Bit && CC == CallingConv::X86_FastCall &&<br></blockquote><blockquote type="cite">+      !Subtarget->isTargetCygMing() && !Subtarget->isTargetWindows() &&<br></blockquote><blockquote type="cite">+      (NumBytes & 7) == 0)<br></blockquote><blockquote type="cite">+    NumBytes += 4;</blockquote></blockquote><div><br class="webkit-block-placeholder"></div>The former is also a typo. It should definitely be <font class="Apple-style-span" style=""><font class="Apple-style-span" color="#000000" face="Courier"><span class="Apple-style-span" style="background-color: transparent;">!</span></font></font><font class="Apple-style-span" color="#000000" face="Courier"><span class="Apple-style-span" style="background-color: transparent;">Is64Bit</span></font>.</div><div><br></div><div><blockquote type="cite">That's pretty interesting.<br></blockquote><div><br class="webkit-block-placeholder"></div><div>Indeed. I made some mechanical transformations where the rationale or invariants were unclear.</div><div><br class="webkit-block-placeholder"></div><div><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">The source code was:</span></blockquote></div><blockquote type="cite">-  if (!Subtarget->isTargetCygMing() && !Subtarget->isTargetWindows()) {<br>-    // Make sure the instruction takes 8n+4 bytes to make sure the<br>start of the<br>-    // arguments and the arguments after the retaddr has been pushed are<br>-    // aligned.<br>-    if ((NumBytes & 7) == 0)<br>-      NumBytes += 4;<br>-  }<br>-<br>Actually this is a hunk from the times, when fastcall and fastcc shared the same LowerFORMAL_ARGUMENTS call.</blockquote><div>History tends to repeat itself. :)</div><blockquote type="cite">The "funny" thing is that fastcall can be seen only on windows (32 bit!) targets, so the condition is twice bogus. Most probably the whole hunk should be just eliminated.</blockquote><div><br class="webkit-block-placeholder"></div><div>If you're sure, I'll happily delete the lot. But it doesn't look necessarily inert to me once the typo is fixed.</div><br><div>I think this should be sunken into GetAlignedArgumentStackSize if it remains, since it's always called something like this:</div><div><br class="webkit-block-placeholder"></div><div><div><font class="Apple-style-span" face="Courier">   unsigned NumBytes = CCInfo.getNextStackOffset();</font></div><div><font class="Apple-style-span" face="Courier">   if (CC == CallingConv::Fast)</font></div><div><font class="Apple-style-span" face="Courier">     NumBytes = GetAlignedArgumentStackSize(NumBytes, DAG);</font></div><div><font class="Apple-style-span" face="Courier"> </font></div><div><font class="Apple-style-span" face="Courier">+  // Make sure the instruction takes 8n+4 bytes to make sure the start of the</font></div><div><font class="Apple-style-span" face="Courier">+  // arguments and the arguments after the retaddr has been pushed are aligned.</font></div><div><font class="Apple-style-span" face="Courier">+  if (!Is64Bit && CC == CallingConv::X86_FastCall &&</font></div><div><font class="Apple-style-span" face="Courier">+      !Subtarget->isTargetCygMing() && !Subtarget->isTargetWindows() &&</font></div><div><font class="Apple-style-span" face="Courier">+      (NumBytes & 7) == 0)</font></div><div><font class="Apple-style-span" face="Courier">+    NumBytes += 4;</font></div><div><br class="webkit-block-placeholder"></div><div>Where<font class="Apple-style-span" color="#000000"><span class="Apple-style-span" style="background-color: transparent;"> GetAlignedArgumentStackSize is documented to…</span></font></div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><font class="Apple-style-span" face="Courier"><font class="Apple-style-span" size="3"><span class="Apple-style-span" style="font-size: 12px;">/// GetAlignedArgumentStackSize - Make the stack size align e.g 16n + 12 aligned</span></font></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><font class="Apple-style-span" face="Courier"><font class="Apple-style-span" size="3"><span class="Apple-style-span" style="font-size: 12px;">/// for a 16 byte align requirement.</span></font></font></div><div><br></div><div>And is coded fairly generically, although I didn't dig deep enough to figure out whether GetAlignedArgumentStackSize would DTRT in this particular case. (getNextStackOffset won't; it's just an accumulator.) Ultimately, I think GetAlignedArgumentStackSize should do this bit's job, and the lot should be sunk into getNextStackOffset—but I was trying to be safe and incremental.</div><div><br class="webkit-block-placeholder"></div><div><font class="Apple-style-span" color="#236E25" face="'Andale Mono'" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div></div></div><blockquote type="cite"><blockquote type="cite">+  if (IsCalleePop(Op)) {<br></blockquote><blockquote type="cite">+    BytesToPopOnReturn  = StackSize; // Callee pops everything.<br></blockquote>Maybe it will be better to make function return amount of bytes to pop and fold ArgsAreStructReturn() call there?<br></blockquote><div><br class="webkit-block-placeholder"></div><div>Maybe something like that. This logic is essentially duplicated in LowerCALL and LowerFORMAL_ARGUMENTS, except for the SDNode passed to IsStructReturn is of differing type. (IsCalleePop just happens to be looking at bits of FORMAL_ARGUMENTS or CALL nodes which are structurally identical; CallIsStructReturn and ArgsAreStructReturn don't have that luxury, though they could consult the SDNode's type.) The relevant passages:</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><span style="color: #000000">  </span>// Some CCs need callee pop.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">  <span style="color: #760f50">if</span> (IsCalleePop(Op)) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    BytesToPopOnReturn  = StackSize; <span style="color: #236e25">// Callee pops everything.</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    BytesCallerReserves = <span style="color: #0000ff">0</span>;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">  } <span style="color: #760f50">else</span> {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    BytesToPopOnReturn  = <span style="color: #0000ff">0</span>; <span style="color: #236e25">// Callee pops nothing.</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><span style="color: #000000">    </span>// If this is an sret function, the return should pop the hidden pointer.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    <span style="color: #760f50">if</span> (!Is64Bit && ArgsAreStructReturn(Op))</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">      BytesToPopOnReturn = <span style="color: #0000ff">4</span>;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    BytesCallerReserves = StackSize;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">  }</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">-----------------------------------------------------------------------------</div><div><span class="Apple-style-span" style="color: rgb(35, 110, 37); font-family: 'Andale Mono'; font-size: 10px; "><span style="color: #000000">  </span>// Create the CALLSEQ_END node.</span></div><div><font class="Apple-style-span" face="'Andale Mono'" size="2"><span class="Apple-style-span" style="font-size: 10px;"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">  <span style="color: #760f50">unsigned</span> NumBytesForCalleeToPush;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">  <span style="color: #760f50">if</span> (IsCalleePop(Op))</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    NumBytesForCalleeToPush = NumBytes;    <span style="color: #236e25">// Callee pops everything</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">  <span style="color: #760f50">else</span> <span style="color: #760f50">if</span> (!Is64Bit && CallIsStructReturn(Op))</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><span style="color: #000000">    </span>// If this is is a call to a struct-return function, the callee</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><span style="color: #000000">    </span>// pops the hidden struct pointer, so we have to push it back.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(35, 110, 37); "><span style="color: #000000">    </span>// This is common for Darwin/X86, Linux & Mingw32 targets.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    NumBytesForCalleeToPush = <span style="color: #0000ff">4</span>;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; color: rgb(118, 15, 80); "><span style="color: #000000">  </span>else</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal 'Andale Mono'; ">    NumBytesForCalleeToPush = <span style="color: #0000ff">0</span>;  <span style="color: #236e25">// Callee pops nothing.</span></div></span></font></div></div><br><blockquote type="cite"><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">+    if (IsTailCall || !Is64Bit ||</span></blockquote><blockquote type="cite">+        getTargetMachine().getCodeModel() != CodeModel::Large)<br></blockquote><blockquote type="cite">+      Callee = DAG.getTargetExternalSymbol(S->getSymbol(),<br></blockquote><blockquote type="cite">getPointerTy());<br></blockquote>I myself feel somehow inconvenient to have such condition here (mixing predicates of different sorts).</blockquote><div><br class="webkit-block-placeholder"></div><div>I don't like repeatedly consulting either<span class="Apple-style-span" style="font-family: 'Andale Mono'; font-size: 10px; "><span class="Apple-style-span" style="font-size: 12px; font-family: 'Trebuchet MS'; "> </span>PerformTailCallOpt<span class="Apple-style-span" style="font-family: 'Trebuchet MS'; font-size: 12px; ">/</span>IsTailCall</span> or <span class="Apple-style-span" style="font-family: 'Andale Mono'; font-size: 10px; ">!Subtarget->is64Bit()<span class="Apple-style-span" style="font-family: 'Trebuchet MS'; font-size: 12px; ">. Rather, I'd prefer to have a table-driven system. I think that's a followup patch, though; this one is already too large.</span></span></div><div><br></div><blockquote type="cite">Maybe it will be worth to keep 64 bit variant separate. I don't know. Evan?<br></blockquote></div><div apple-content-edited="true"><span class="Apple-style-span" style="border-collapse: separate; border-spacing: 0px 0px; color: rgb(0, 0, 0); font-family: Trebuchet MS; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: auto; -khtml-text-decorations-in-effect: none; text-indent: 0px; -apple-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><br class="webkit-block-placeholder"></div><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; ">The 64-bit code diverged only in a very few details.</div><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; border-spacing: 0px 0px; color: rgb(0, 0, 0); font-family: Trebuchet MS; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: auto; -khtml-text-decorations-in-effect: none; text-indent: 0px; -apple-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><span class="Apple-style-span" style="border-collapse: separate; border-spacing: 0px 0px; color: rgb(0, 0, 0); font-family: Trebuchet MS; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: auto; -khtml-text-decorations-in-effect: none; text-indent: 0px; -apple-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><span class="Apple-style-span" style="border-collapse: separate; border-spacing: 0px 0px; color: rgb(0, 0, 0); font-family: Trebuchet MS; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: auto; -khtml-text-decorations-in-effect: none; text-indent: 0px; -apple-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><span class="Apple-style-span" style="border-collapse: separate; border-spacing: 0px 0px; color: rgb(0, 0, 0); font-family: Trebuchet MS; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: auto; -khtml-text-decorations-in-effect: none; text-indent: 0px; -apple-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><span class="Apple-style-span" style="border-collapse: separate; border-spacing: 0px 0px; color: rgb(0, 0, 0); font-family: Trebuchet MS; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: auto; -khtml-text-decorations-in-effect: none; text-indent: 0px; -apple-text-size-adjust: auto; text-transform: none; orphans: 2; white-space: normal; widows: 2; word-spacing: 0px; "><div><br class="khtml-block-placeholder"></div>— Gordon<br class="Apple-interchange-newline"></span></span></span></span></span></div></span> </div></div><div><br></div></body></html>