-
Notifications
You must be signed in to change notification settings - Fork 80
Add type checking to {bitcast,ptrtoint,inttoptr} #69
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
Conversation
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.
Thanks for working on this Peter!
I think before we can merge this, we must gain a better understanding of what types are supported for each instruction. Is there any official documentation for this?
Robin
@@ -536,6 +553,12 @@ type InstBitCast struct { | |||
// NewBitCast returns a new bitcast instruction based on the given source value | |||
// and target type. | |||
func NewBitCast(from value.Value, to types.Type) *InstBitCast { | |||
if _, isFromPtr := from.Type().(*types.PointerType); !isFromPtr { |
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.
I'm fairly sure you can bitcase, e.g. a 64-bit integer into a 64-bit floating point value.
llvm/test/CodeGen/Hexagon/float-bitcast.ll:
%v0 = bitcast double %a0 to i64
4698703
to
4e8d2c0
Compare
Hmm, this is harder than I thought to get exhaustively correct. The documentation would be: http://llvm.org/docs/LangRef.html I've fixed the error message consistency issue, but the getting the rest right will take a bit more effort. I would be happy to abandon those changes we are uncertain about for now, since I want to work on other things at the moment. |
Sure, sounds good to me. We can revisit later when the need arise. Edit: I'll look more carefully at the PR in the coming days. Just started Uni in Korea, so have the first week just to get settled into study life again :) |
* Support vector and int typed arguments. * Add a table driven test for the different cases. I've tried my best to make the error messages informative and consistent with other error messages, but further improvements are welcomed - please also feel free to take ownership of the branch and tweak the messages if appropriate. Updates #65. Subsumes part of PR #69.
Since this turned out to be complicated I've broken out:
I'll continue to pare this PR down until there is nothing left. |
* Support vector and int typed arguments. * Add a table driven test for the different cases. I've tried my best to make the error messages informative and consistent with other error messages, but further improvements are welcomed - please also feel free to take ownership of the branch and tweak the messages if appropriate. Updates #65. Subsumes part of PR #69.
I'm going to discard this PR. I've fleshed out the original issue (#65) to track progress. |
As discussed in #65, having this sort of check can be useful for pinpointing bugs.
There may be use cases for using incomplete types and filling them in later. Those may be fulfilled by manually constructing values of the types.
There is precedent in the code for this kind of type checking.
This PR adds type checking to:
I also made aggregateElemType support pointer types, and decided to include the fix to #67 in this PR, since it was trivial.
Closes #65.
Closes #67. (I have done
grep -A1 'Compute type'
and checked that all similar comments have an appropriate call - it looks like this was the only one).