[PATCH] D105807: [X86] pr51000 in-register struct return tailcalling

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

There's a long discussion involving musttail above that I'm going to ignore, since it doesn't seem to have anything to do with PR51000.

In D105807#2952398 <https://reviews.llvm.org/D105807#2952398>, @urnathan wrote:

> Ok, having had a nice long break here's an updated patch.  It addresses the case of tail calling a function that returns a struct in registers.  These are just like tail calling functions that return scalars.

I agree, I think the code change is functionally correct: we want to power-down TCO whenever the calling function has to return something in RAX. For now, you're conservatively assuming the callee won't do that, and that's fine.

> Is it acceptable to have the FIXME comment pointing at pr51000 about the more complicated case when return is via an sret pointer parameter?

I can't say these are normative conventions, but my experience is that LLVM generally avoids referencing bug numbers from comments. There are plenty of standard references and links to things like docs and object file standards, but otherwise, the idea is that the code comments should have everything you need to reason about the behavior, and the reader shouldn't have to refer to a bug to understand why the code is a certain way.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4853
+    // sret. Condition #b is not easy to determine at this point.
+    // FIXME: See pr51000 for more information.
     return false;
----------------
My sense is that the link isn't actually that helpful. I think the comments you wrote are reasonable straightforward. At least, I got it, but maybe I'm special. :)


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4679
+  // is incompatible.
+  if (callIsStructReturn(Outs, Subtarget.isTargetMCU()) == StackStructReturn) {
+    // For a compatible tail call the callee must return our sret pointer. So it
----------------
urnathan wrote:
> efriedma wrote:
> > I feel like I must be blind here, but this is what we used to call isCalleeStructRet, right? 
> > I thought we wanted to check isCallerStructRet?
> Not quite.  callIsStructReturn's result is tri-valued.  Previously we were checking != NotStructReturn.  It is sufficient to check == StackStructReturn, as RegStructReturn is tailcallable.  I moved the call to callIsStructReturn here, from the caller, because its the only use of that value.
I think the new way of writing this is clearer: the constraint we're really looking for is, do we need to return the sret parameter at the end of the function? If yes, block TCO. If not, do it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105807/new/

https://reviews.llvm.org/D105807