[PATCH] D103275: Update musttail verification to check all swiftasync->swiftasync tail calls.

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

In D103275#2787268 <https://reviews.llvm.org/D103275#2787268>, @aschwaighofer wrote:

> I guess that the 'in tail position' restriction cannot really be used by frontends because it has to predict that `otherStuff()` cannot be optimized away. So it would not buy us much other than a loss of clarity ...

Yeah, I think that's right. The check for `ReturnInst` seems quite fragile. Replacing it with a restriction that frontends need to explicitly say `notail` or `musttail` seems much easier to reason about.

To that end, a bunch of more specific feedback inline.



================
Comment at: llvm/docs/LangRef.rst:447-450
 "``swifttailcc``"
     This calling convention is like ``swiftcc`` in most respects, but also the
     callee pops the argument area of the stack so that mandatory tail calls are 
     possible as in ``tailcc``.
----------------
I think this is a good place to document that all calls to swifttailcc from swifttailcc must be marked with either `notail` or `musttail`.


================
Comment at: llvm/docs/LangRef.rst:11323-11329
+#. The optional ``tail`` and ``musttail`` markers (with one exception for
+   mandatory ``musttail`` when using ``swifttailcc``, see below) indicate that
+   the optimizers should perform tail call optimization. The ``tail`` marker is
+   a hint that `can be ignored <CodeGenerator.html#sibcallopt>`_. The
+   ``musttail`` marker means that the call must be tail call optimized in
+   order for the program to be correct. The ``musttail`` marker provides
+   these guarantees:
----------------
I'm not sure you need this change. The text is already clear that `musttail` means the tail call is mandatory for this particular call.


================
Comment at: llvm/docs/LangRef.rst:11360
+   - ``musttail`` is mandatory for tail calls where both the caller and
+     the callee use the ``swifttailcc`` convention.
 
----------------
aschwaighofer wrote:
> I think we should clarify that this only applies to calls immediately followed by return.
Given John's feedback about using `notail` and `musttail`, I think the return restriction should be dropped, and this content moved to the section on `swifttailcc`.


================
Comment at: llvm/lib/IR/Verifier.cpp:3424-3425
+// default.
+static cl::opt<bool> DisableSwiftTailCCMustTailCheck(
+    "disable-swifttailcc-musttail-check", cl::init(false),
+    cl::desc("Check that tail calls from swifttailcc functions to"
----------------
Maybe a better name would be `-disable-swifttailcc-check` or `-disable-verify-swifttailcc`? Since it's not just about `musttail` anymore.


================
Comment at: llvm/lib/IR/Verifier.cpp:3507
   visitCallBase(CI);
-
-  if (CI.isMustTailCall())
-    verifyMustTailCall(CI);
+  verifyMustTailCall(CI);
 }
----------------
I suggest reverting this change (and the one to `verifyMustTailCall()`), since the new invariant isn't just about `musttail`. Instead, you can just add the new check directly in `visitCallInst()`, or add/call a new function called something like `verifyCallingConv()`.

Incorporating John's feedback about using `notail` instead of `ret`, I think the check would be something like:
```
lang=c++
if (!Disable... &&
    CI.getCallingConv() == CallingConv::SwiftTail &&
    CI.getCaller()->getCallingConv() == CallingConv::SwiftTail)
  Assert(CI.isMustTailCall() || CI.isNoTailCall(), ...);
```



================
Comment at: llvm/test/Verifier/musttail-missing.ll:5-9
+define swifttailcc void @swift_caller() {
+; CHECK: tail call from swifttail->swifttail should be marked musttail
+  call swifttailcc void @swift_callee()
+  ret void
+}
----------------
This is a great start. It should also check that the verifier doesn't fire when the IR is valid.

Taking into account John's feedback, I think the matrix should be:
- `musttail` vs. `notail` vs. `tail` vs. none of the above (former two are valid, latter two are invalid)
- one of callee and caller is `swifttailcc`, and the other is not (one example for each direction where the IR would be invalid if both were `swiftailcc`)

Assuming we're dropping the rule about other instructions before `ret` we probably don't need that in the matrix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103275