Skip to content
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

Refine tail call conditions for tail and musttail markers #1094

Merged

Conversation

antoniofrighetto
Copy link
Contributor

musttail has stronger requirements than tail, so ensure this is taken into account.

@antoniofrighetto antoniofrighetto force-pushed the feature/refine-tail-calls branch 2 times, most recently from fead176 to 05d48bc Compare October 5, 2024 07:56
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why delete this test? it's correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it to musttail-memcpy.srctgt.ll and replace tail w/ musttail (since with tail only marker it shouldn't be UB, we're verifying only isMustTailCall now).

%call2 = alloca i64
%call3 = alloca i64
store i64 %arg, ptr %call2, align 8
musttail call tailcc void @llvm.memcpy.p0.p0.i64(ptr byval(ptr) %call3, ptr byval(ptr) %call2, i64 8, i1 false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this not a good test because it's mixing several conditions at once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separated it in multiple functions.

@antoniofrighetto
Copy link
Contributor Author

I think we miss checking the following:

In addition, if the calling convention is not swifttailcc or tailcc:
- All ABI-impacting function attributes, such as sret, byval, inreg,
returned, and inalloca, must match.
- The caller and callee prototypes must match. Pointer types of parameters
or return types may differ in pointee type, but not in address space.

(Not completely sure if this refers to musttail too though)

@nunoplopes
Copy link
Member

From a quick read, this doesn't seem to be what we want at all. We need both tail & musttail, as both have constraints.
I suggest create a new thing in ir/attr.h (tail call type or smth like that). It can then encode the conditions for each case.

@antoniofrighetto antoniofrighetto force-pushed the feature/refine-tail-calls branch 2 times, most recently from 78c3527 to 69bb131 Compare October 8, 2024 10:48
@antoniofrighetto
Copy link
Contributor Author

Added a new type TailCallInfo to encode both tail call markers. Updated the preconditions such that they are verified for both markers: tail position only for musttail, whereas callee not accessing allocas, va_args, or byval arguments from the caller for both. Restored tailcall-memcpy.srctgt.ll test since it violates this last constraint.

@antoniofrighetto antoniofrighetto changed the title Refine tail call conditions for musttail Refine tail call conditions for tail and musttail markers Oct 8, 2024
@@ -0,0 +1,11 @@
define tailcc i64 @src(i64 noundef %arg) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test doesn't seem very useful; doesn't check for anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped it.

store i64 %arg, ptr %call2, align 8
tail call void @llvm.memcpy.p0.p0.i64(ptr %call3, ptr %call2, i64 8, i1 false)
ret i64 poison
ret i64 undef
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use poison instead pls. we want to get rid of undef

ir/instr.cpp Outdated Show resolved Hide resolved
@@ -521,4 +525,67 @@ llvm::Function *findFunction(llvm::Module &M, const string &FName) {
return F && !F->isDeclaration() ? F : nullptr;
}

TailCallInfo parse_tail_call_info(const llvm::CallInst &i) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't encode semantics like this. The semantics must be in the ir folder.
We support other IRs -> Alive2 IR, hence we do as little as possible in the translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left parse_tail_call_info in utils.cpp so that it may be used from known_functions too (originally was in llvm2alive). I think we still need it there as we're parsing data from LLVM (like parse_fn_attrs). Do you mean that the semantics in are_tailcall_preconditions_met is to be moved in ir inside check_tailcall?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also made it more coherent (parse_tail_call_info -> parse_fn_tailcall).

llvm_util/known_fns.h Outdated Show resolved Hide resolved
switch (libfn) {
case llvm::LibFunc_memset: // void* memset(void *ptr, int val, size_t bytes)
BB.addInstr(make_unique<Memset>(*args[0], *args[1], *args[2], 1, is_tailcall));
BB.addInstr(make_unique<Memset>(*args[0], *args[1], *args[2], 1,
std::move(tci)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move is a bit overkill

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be changed after the change above.

ir/instr.cpp Show resolved Hide resolved
@antoniofrighetto
Copy link
Contributor Author

Updated, moved semantics in checkTailCall.

@nunoplopes nunoplopes merged commit 40d1b5b into AliveToolkit:master Oct 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants