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 methods for creating/checking for poison values. #431

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

tylanphear
Copy link
Contributor

@tylanphear tylanphear commented Aug 15, 2023

Description

This patch adds get_poison() methods to *Type, as well as is_poison() methods to *Value, implemented identically to the corresponding get_undef()/is_undef() methods. This is particularly useful for operations on vector types, as the use of poison for masks, shuffles, and inserts is idiomatic.

How This Has Been Tested

  • Passes cargo test --features llvm15-0 locally.
  • Two tests added for creating/checking poison values.

Checklist

This patch adds `get_poison()` methods to `*Type`, as well as
`is_poison()` methods to `*Value`, implemented almost identically to the
corresponding `get_undef()`/`is_undef()` methods. This is particularly
useful for operations on vector types, as the use of `poison` for
masks, shuffles, and inserts is idiomatic.
@TheDan64
Copy link
Owner

Is it valid and correct for any single type imaginable to be poisionable? If so maybe it makes more sense to add the method to the AnyType trait rather than on all types individually

@tylanphear
Copy link
Contributor Author

Is it valid and correct for any single type imaginable to be poisionable? If so maybe it makes more sense to add the method to the AnyType trait rather than on all types individually

Hmm, OK. I figured I would mirror what was done with undef but if you'd like me to make it a trait method, I can probably do that.

@tylanphear
Copy link
Contributor Author

Two immediate downsides of moving the method to the trait:

  1. The user has to use inkwell::types::AnyType to get the method in scope.
  2. The trait function (I think) has to return AnyValueEnum instead of a value of the actual type (i.g. IntValue<'ctx>)

@TheDan64
Copy link
Owner

I see #1 as an advantage since it reduces the api surface and makes the functionality generic.

As for 2, I'm not really sure what you mean. IntValue implements the any trait so it's able to use the method

@tylanphear
Copy link
Contributor Author

tylanphear commented Aug 19, 2023

I see #1 as an advantage since it reduces the api surface and makes the functionality generic.

As for 2, I'm not really sure what you mean. IntValue implements the any trait so it's able to use the method

Just to be clear, you mean move the get_poison() methods to AnyType and get rid of get_poison() on each type, right? If that's the case, how would I return IntValue when calling get_poison() on an instance of IntType?

@TheDan64
Copy link
Owner

TheDan64 commented Aug 22, 2023

I think I see what you're saying. It could be an associated type if we still want to go down that route. But we don't have to

@tylanphear
Copy link
Contributor Author

tylanphear commented Aug 22, 2023

I think I see what you're saying. It could be an associated type if we still want to go down that route. But we don't have to

Perhaps if you're inclined to take a stab, that could be worked. When I tried, that seemed to make the changes much broader in scope (had to specify the associated type everywhere AnyType is used) -- plus, even if we have some associated type, we can't necessarily construct it without another trait.

A more pressing issue (reason why the CI failed) seems to be that LLVMIsPoison and LLVMGetPoison weren't added to the C API until LLVM 12. I think that means the methods need to be gated on #[llvm_versions(12.0..=latest)].

@tylanphear
Copy link
Contributor Author

As a side note: It does looks like I can move is_poison() to the trait though, so that's nice.

@TheDan64
Copy link
Owner

Sounds good, let's not increase the scope then for now

@tylanphear
Copy link
Contributor Author

@TheDan64 I think I need your approval for another CI run, when you get the chance.

@tylanphear
Copy link
Contributor Author

Whoops, forgot to import AnyValue in the doc-tests. Should be fixed now.

@tylanphear
Copy link
Contributor Author

@TheDan64 CI appears to be passing, just needs a review.

Copy link
Owner

@TheDan64 TheDan64 left a comment

Choose a reason for hiding this comment

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

Thank you!

@TheDan64 TheDan64 merged commit 5378a77 into TheDan64:master Aug 25, 2023
14 checks passed
@tylanphear tylanphear deleted the add_poison branch August 25, 2023 01:22
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