-
-
Notifications
You must be signed in to change notification settings - Fork 265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pragma(musttail) #4366
base: master
Are you sure you want to change the base?
Add pragma(musttail) #4366
Conversation
I like that better - I assume GDC could implement it too. |
f5ba4a4
to
4da4c6e
Compare
else if (ps.ident == Id.musttail) | ||
{ | ||
pragmaMustTailSemantic(ps); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a frontend file too (everything in dmd/
), so IN_LLVM
again. Note that there's a IN_LLVM
global/enum somewhere too, so that it's easy to use in if
expressions.
And version (IN_LLVM)
for pragmaMustTailSemantic()
below too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should non-ldc compilers do for this pragma? Will this file be syncronized with dmd at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstreaming our frontend diff is probably not realistic; some of it are hacks, and some others really totally LDC-specific and would just clutter the DMD frontend IMO. If another compiler supported it, we'd upstream a proper semantic for the new pragma though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clutter is meaningless versus the asymptotic cost of complexity - it should be upstreamed. LDC is the real D compiler, the codebase should be honest about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly less abstract: we should get a way upstream for there to no more IN_LLVM and so on other than registering a behaviour (and not "just override a weak symbol")
gen/funcgenstate.cpp
Outdated
llvm::FunctionType *calleeType, | ||
llvm::ArrayRef<llvm::Value *> args, | ||
const char *name, bool isNothrow) { | ||
const char *name, bool isNothrow, bool isMustTail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to avoid changing the signature of this function.
I guess you could use the DValue
returned by DtoCallFunction()
like this:
DValue *bla = DtoCallFunction(…);
if (ce->isMustTail) { // ce is the CallExp*
if (auto ci = llvm::dyn_cast<llvm::CallInst>(DtoRVal(bla))) {
ci->setTailCallKind(llvm::CallInst::TCK_MustTail);
} else {
// report error
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that probably only works for trivial return value types. Another option: giving DtoCallFunction()
an optional std::function
param, which gets the LLCallBasePtr call
as argument and gets a chance to manipulate it.
Wrt. a testcase, see e.g. https://github.com/ldc-developers/ldc/blob/master/tests/codegen/array_alloc_gh3041.d for a simple example checking the generated IR. The stuff in |
@ibuclaw Can GDC implement this too (or does it already have it?) ? |
628cb3b
to
218d800
Compare
ci->setTailCallKind(llvm::CallInst::TCK_MustTail); | ||
} else { | ||
if (!tf->isnothrow()) { | ||
error(loc, "cannot perform tail-call - callee must be nothrow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this error not be caught during semantic analysis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where in semantic can I check this? I'm not really familiar with the code.
if (!tf->isnothrow()) { | ||
error(loc, "cannot perform tail-call - callee must be nothrow"); | ||
} else { | ||
error(loc, "cannot perform tail-call - no code like destructors or scope(exit) should run after the call"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, can this error not be caught during semantic analysis?
cc @ibuclaw |
Discussed in #4355
Open questions:
LDC_musttail
what we want? Or we should usemusttail
?