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 type checking to {bitcast,ptrtoint,inttoptr} #69

Closed
wants to merge 5 commits into from

Conversation

pwaller
Copy link
Member

@pwaller pwaller commented Feb 23, 2019

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:

  • insertvalue
  • trunc
  • bitcast
  • ptrtoint
  • inttoptr

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).

@coveralls
Copy link

coveralls commented Feb 23, 2019

Coverage Status

Coverage decreased (-0.09%) to 68.184% when pulling 4e8d2c0 on type-checking into 10a64da on master.

Copy link
Member

@mewmew mewmew left a 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

ir/inst_aggregate.go Outdated Show resolved Hide resolved
ir/inst_conversion.go Show resolved Hide resolved
@@ -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 {
Copy link
Member

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

@pwaller
Copy link
Member Author

pwaller commented Feb 24, 2019

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.

@mewmew
Copy link
Member

mewmew commented Feb 24, 2019

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 :)

pwaller added a commit that referenced this pull request Feb 24, 2019
* 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.
@pwaller
Copy link
Member Author

pwaller commented Feb 24, 2019

Since this turned out to be complicated I've broken out:

I'll continue to pare this PR down until there is nothing left.

@pwaller pwaller changed the title Add type checking to {insertvalue,trunc,bitcast,ptrtoint,inttoptr} Add type checking to {bitcast,ptrtoint,inttoptr} Feb 24, 2019
pwaller added a commit that referenced this pull request Feb 24, 2019
* 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.
@pwaller
Copy link
Member Author

pwaller commented Feb 25, 2019

I'm going to discard this PR. I've fleshed out the original issue (#65) to track progress.

@pwaller pwaller closed this Feb 25, 2019
@pwaller pwaller deleted the type-checking branch February 25, 2019 12:18
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.

ir: proposal: typecheck more things Missing Type call?
3 participants