[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. I think I misunderstood @yamt's comment in https://reviews.llvm.org/D147714#4446780. I take back what I wrote above. I agree that `[[clang::nonportable_musttail]]` is a nicer semantic, and the restrictions around the existing `[[clang:musttail]]` don't seem to buy us

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. >> If all you want is (1), that doesn't require any attribute (the optimizer >> will do it automatically). Not entirely true, as compiler can do anything. In some cases you really need *musttail* (in llvm IR) to get or preserve tail call. CHANGES SINCE LAST ACTION

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. In D147714#4446809 , @haberman wrote: >> Such a definition doesn't make much sense because for some targets tail call >> optimization isn't available at all. >> Eg. Wasm w/o tail call proposal, xtensa windowed abi. > > That is

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. > Such a definition doesn't make much sense because for some targets tail call > optimization isn't available at all. > Eg. Wasm w/o tail call proposal, xtensa windowed abi. That is true. But what is the correct behavior if you try to use `[[clang::musttail]]` on such

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. In D147714#4446780 , @yamt wrote: >> musttail provide assurances that the tail call can be optimized on all >> targets. > > Such a definition doesn't make much sense because for some targets tail call > optimization isn't

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

yamt added a comment. > musttail provide assurances that the tail call can be optimized on all > targets. Such a definition doesn't make much sense because for some targets tail call optimization isn't available at all. Eg. Wasm w/o tail call proposal, xtensa windowed abi. So I'm not sure

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

nickdesaulniers added a comment. In D147714#4253247 , @xbolva00 wrote: >>> I'm curious if folks have pursued @efriedma 's suggestion #2 from >>> https://github.com/llvm/llvm-project/issues/54964#issuecomment-1101612886? > > This is something we

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. >> I'm curious if folks have pursued @efriedma 's suggestion #2 from >> https://github.com/llvm/llvm-project/issues/54964#issuecomment-1101612886? This is something we mentioned here, perform target specific checks in Sema (but still it is not possible to catch

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

nickdesaulniers requested changes to this revision. nickdesaulniers added a comment. This revision now requires changes to proceed. In D147714#4249274 , @efriedma wrote: > Any thoughts on diagnostics here? If I'm not mistaken, with this patch, if > you

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. `[[nonportable_musttail]]` makes sense to me as a semantic. It indicates that the algorithm requires tail calls, but the author is willing to accept that the algorithm may be non-portable. "Non-portable" here can mean architecture-specific, but it can also mean

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

erichkeane added a comment. In D147714#4251752 , @xbolva00 wrote: > So to make some conclusion - there is a need for less strict version of > musttail in LLVM, right? Ideally platform specific, like x86_musttail? I'm not sure what the rules for the

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. So to make some conclusion - there is a need for less strict version of musttail in LLVM, right? Ideally platform specific, like x86_musttail? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147714/new/ https://reviews.llvm.org/D147714

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

erichkeane added a comment. In D147714#4251690 , @haberman wrote: >> is a [[should_tail]] attribute sort of thing: a tail-hint where we do 'best >> effort with no promises', and make no guarantees that we're going to tail it. > > I'm not sure I see the

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. > is a [[should_tail]] attribute sort of thing: a tail-hint where we do 'best > effort with no promises', and make no guarantees that we're going to tail it. I'm not sure I see the value in that. The compiler already optimizes tail calls when it can in a best-effort

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. >> Will the diagnostic be emitted to the correct location if there's no debug >> info? I did some experiments and with no debug info, I got atleast the location of the caller. If we consider than currently there is no location, anything is improvement and this is

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

erichkeane added a comment. In D147714#4251208 , @xbolva00 wrote: >>> Could we instead encode the platform specific tail-call rules in Sema? We >>> definitely have attributes that differ in behavior between platforms, and >>> this seems like a perfect

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a subscriber: jacobsa. xbolva00 added a comment. >> Could we instead encode the platform specific tail-call rules in Sema? We >> definitely have attributes that differ in behavior between platforms, and >> this seems like a perfect candidate. But then you break promise of

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

erichkeane added a comment. Yeah, I think the diagnostics result of this attribute are unacceptable. I understand the goal here, but I suspect the more ergonomic (albeit, perhaps surprising) is a `[[should_tail]]` attribute sort of thing: a tail-hint where we do 'best effort with no

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

aaron.ballman added a reviewer: erichkeane. aaron.ballman added a comment. In D147714#4249531 , @xbolva00 wrote: > Added location when emiting diagnostic about failed TCO in the backend. Will the diagnostic behavior change based on optimization level?

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. Possibly verifier ignores it? Because I dont see any verifier error compiling this testcase: void NoParams(); void TestParamArityMismatchNonPortable(int x) { [[clang::nonportable_musttail]] return NoParams(); } But yeah, this is something we need to

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. Correct me if I am wrong, but won't this produce LLVM IR that fails to validate? The checks that exist today for `[[clang::musttail]]` are necessary to follow LLVM's rules about where `musttail` may appear: https://llvm.org/docs/LangRef.html#id328 > Calls marked

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. In D147714#4249357 , @efriedma wrote: > An error message pointing at the call in the source code would be good > enough, I think. Adding a "reason" might be nice, but not strictly necessary. Good idea. So now for: short

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 updated this revision to Diff 511498. xbolva00 added a comment. Herald added subscribers: llvm-commits, pengfei, hiraditya. Herald added a project: LLVM. Added location when emiting diagnostic about failed TCO in the backend. CHANGES SINCE LAST ACTION

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

efriedma added a comment. An error message pointing at the call in the source code would be good enough, I think. Adding a "reason" might be nice, but not strictly necessary. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147714/new/ https://reviews.llvm.org/D147714

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 added a comment. In D147714#4249274 , @efriedma wrote: > Any thoughts on diagnostics here? If I'm not mistaken, with this patch, if > you request an impossible tail call, you get a crash with very little useful > information. (Although, see

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

efriedma added a subscriber: nickdesaulniers. efriedma added a comment. Any thoughts on diagnostics here? If I'm not mistaken, with this patch, if you request an impossible tail call, you get a crash with very little useful information. (Although, see

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 updated this revision to Diff 511427. xbolva00 added a comment. Mention it in attribute documentation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147714/new/ https://reviews.llvm.org/D147714 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/AttrDocs.td

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

xbolva00 created this revision. xbolva00 added reviewers: aaron.ballman, efriedma, haberman. Herald added a project: All. xbolva00 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. As noted in