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

[Varun Gandhi via Phabricator via llvm-commits]

varungandhi-apple added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:3429
+  if (!CI.isMustTailCall()) {
+#ifndef NDEBUG
+    if (EnableSwiftTailCCMustTailCheck &&
----------------
dexonsmith wrote:
> aschwaighofer wrote:
> > dexonsmith wrote:
> > > Hmm... I'm not sure it's right for the verifier to behave differently depending on `NDEBUG` -- note that these `Assert` calls are not the same as the `assert` macro from `<cassert>`. If this was intentional, can you explain why?
> > We want these errors in assert builds -- so we can fix instances where we are missing `musttail` instructions -- but we don't want them in release builds because they are not necessarily fatal.
> > 
> > An alternative, is to just enable `EnableSwiftTailCCMustTailCheck` (which is disabled per default) in the frontend if we compile with asserts.
> I think that alternative (skip the `NDEBUG`, only set `EnableSwiftTailCCMustTailCheck` when there's a desire to enable it) seems a bit cleaner to me -- fewer moving parts. Also, I just noticed there's no test for this verifier check; I think we usually use llvm/test/Verifier (and there won't be a need for `REQUIRES: asserts` if you don't use `NDEBUG`).
> 
> Is the goal to eventually have it on always in the fullness of time? I think we usually use the verifier for IR invariants, but it sounds like maybe that's not what this is.
>  Also, I just noticed there's no test for this verifier check; I think we usually use llvm/test/Verifier

I hadn't realized this; I'll add a test.

> Is the goal to eventually have it on always in the fullness of time?

Ideally yes. There shouldn't be a good reason for us to miss `musttail` on tail calls like this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103275