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

Add attributes for memory intrinsics #555

Merged

Conversation

tathanhdinh
Copy link
Contributor

@tathanhdinh tathanhdinh commented Oct 21, 2021

Hello all,

This is not a ready PR (does not compile yet with LLVM below 3.9, an only for a single 64-bit read intrinsic), just for discussion at this issue.

I still do not know the interference between readnone, set by FindPureIntrinsic for dead store elimination:

// We want memory intrinsics to be marked as not accessing memory so that
// they don't interfere with dead store elimination.
function->addFnAttr(llvm::Attribute::ReadNone);

and readonly and argmemonly.

Many thanks for any review.

@pgoodman
Copy link
Collaborator

@tathanhdinh we've stopped supporting llvm versions less than 12, and we plan to move to llvm 13 soon. Do you rely on older versions?

@pgoodman
Copy link
Collaborator

Also, for the read-modify-write intrinsics, e.g. cmpxchg, fetch_add, etc. perhaps a __restrict keyword could be used as well.

@tathanhdinh
Copy link
Contributor Author

@tathanhdinh we've stopped supporting llvm versions less than 12, and we plan to move to llvm 13 soon. Do you rely on older versions?

@pgoodman Ah, I don't know that. I did not follow the development of remill for so long (nearly 2 years!?). So it would compile just fine, I think. A minor remark, I still see:

#if LLVM_VERSION_NUMBER < LLVM_VERSION(3, 5)
# error "Minimum supported LLVM version is 3.5"
#endif

Also, for the read-modify-write intrinsics, e.g. cmpxchg, fetch_add, etc. perhaps a __restrict keyword could be used as well.

Sure, that's just because I still not sure what I do is correct or not (from your response in the issue).

Thank you for the feedback.

@pgoodman
Copy link
Collaborator

So right now we mark the intrinsics as readnone, i.e. they don't read or write to memory. So marking them as readonly is perhaps not desirable, but then marking their Memory * as readnone could be. For the read-modify-write intrinsics, we don't mark those as readnone, because they take in an argument by reference (in the code) and pointer (in bitcode). That argument is the value to store and update, and so that value gets read and written, but we could still mark the memory pointer argument as readnone.

@tathanhdinh tathanhdinh force-pushed the memory_intrinsics_attributes branch 2 times, most recently from 183ad39 to 777168f Compare October 23, 2021 16:00
@pgoodman pgoodman merged commit 5955659 into lifting-bits:master Oct 25, 2021
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