Support vector types in math ops, fixes #45 #47

cpdt · 2018-06-22T10:04:51Z

This PR introduces the ability for math functions to take and return vector values in a type-safe way, as discussed in #45. Please note that I'm pretty new to Rust in general, so there are possibly some things here that can be done better :)

Description

The PR primarily introduces two new traits, FloatMathValue and IntMathValue, which are implemented by FloatValue/VectorValue and IntValue/VectorValue respectively. Applicable methods on the Builder have been adjusted to take and return a type that implements these traits - meaning, for example, if you call build_float_add with a vector as the LHS, you must pass a vector for the RHS and you will get a vector back. Since there's no sub-type support yet we can't ensure the passed vectors are actually composed of integers or floats, however I assume this will be a part of 0.2.0.

There are some more complicated statements that require both a value and a target type, such as build_float_to_signed_int (which takes a floating point and new integer type, and returns an integer). LLVM allows us to pass in a vector as the value here, but also requires in that case that we pass in a vector type - in order to support this, the PR adds FloatMathType and IntMathType traits and associated types to navigate between value -> type -> converted type -> value. This means we can ensure the target type is valid (i.e is a vector when the input value is a vector, or an int type otherwise in the build_float_to_signed_int case), at the cost of some long type paths in builder.rs.

Note: this PR doesn't implement vector support for functions like build_ptr_to_int, although LLVM does support vectors for these functions, since I didn't want to complicate the PR too much. I can add these in if it makes sense though.

Breaking Changes

As per one of the requested changes, all math functions now take ownership of their input types/values instead of taking references.

How This Has Been Tested

Due to some weird llvm-sys linking issues on my computer, I haven't actually been able to run the test suite (so we'll see if the CI passes :)). However, this PR is entirely type changes and shouldn't affect anything during runtime. Both the tests, the example, and my own personal project compile fine without any changes.

Checklist

I have read the Contributing Guide

…tions

…s necessary

TheDan64

These changes look great, thanks! I just had a couple comments on things that stuck out to me.

Also, build_ptr_to_int and the reverse seem to follow the same pattern we're implementing here? So, I think it's probably fine to add those changes as well

TheDan64 · 2018-06-23T02:01:17Z

src/builder.rs

@@ -254,305 +254,305 @@ impl Builder {
    // TODO: Possibly make this generic over sign via struct metadata or subtypes
    // SubType: <I: IntSubType>(&self, lhs: &IntValue<I>, rhs: &IntValue<I>, name: &str) -> IntValue<I> {
    //     if I::sign() == Unsigned { LLVMBuildUDiv() } else { LLVMBuildSDiv() }
-    pub fn build_int_unsigned_div(&self, lhs: &IntValue, rhs: &IntValue, name: &str) -> IntValue {
+    pub fn build_int_unsigned_div<T: IntMathValue>(&self, lhs: &T, rhs: &T, name: &str) -> T {


The references here are artifacts of a time when *Values and *Types weren't Copy. Since you're touching this code, I would greatly appreciate if you could change this and the other methods you modified to take ownership of lhs/rhs trait types.

IE, the signature should be ..., lhs: T, rhs: T, ...

Thanks!

Is it worth going through and changing all methods to take ownership, or would you rather I just changed the ones I touched?

Just the ones you have touched - I have to look at the remainder on a case by case basis.

TheDan64 · 2018-06-23T02:04:04Z

src/builder.rs

    }

    // SubType: <F>(&self, op, lhs: &FloatValue<F>, rhs: &FloatValue<F>, name) -> IntValue<bool> { ?
-    pub fn build_float_compare(&self, op: FloatPredicate, lhs: &FloatValue, rhs: &FloatValue, name: &str) -> IntValue {
+    pub fn build_float_compare<T: FloatMathValue>(&self, op: FloatPredicate, lhs: &T, rhs: &T, name: &str) -> <<T::BaseType as FloatMathType>::ConvType as IntMathType>::ValueType {


I think the issue with build_int_compare and build_float_compare returning T and <<T::BaseType as FloatMathType>::ConvType as IntMathType>::ValueType (man is that ugly) respectively is that the return type is actually IntValue<bool> or VecValue<IntValue<bool>>.

This will be rendered incorrect once subtypes are implemented. IE input of IntValue<32bit> will return IntValue<32bit> when it should be IntValue<bool>

I think the way we might need to work around this with subtypes is to add more associated types to go from IntValue<X> -> IntValue<bool> & VecValue<IntValue<X>> -> Vec<IntValue<bool>> (and likewise for floats) somehow

...so I think this is fine as is, unless you can think of a good way to do that here. I would greatly appreciate adding another comment under the subtype comment either describing the above or pasting the URL to this comment

Yeah, there's possibly a similar problem with functions like build_int_cast, if the Type types are also going to have subtypes.

There's currently an RFC being worked on for generic associated types, which I think would allow us to do something like <<T::BaseType as FloatMathType>::ConvType<bool> as IntMathType>::ValueType (notice how ConvType has a generic parameter with the type of integer to 'return') - a similar thing would also work for BaseType for the cast functions. But yeah, it's pretty ugly.

TheDan64

These changes look good to me as is.

That said, would you be willing to write up some tests for the vector math operations? No need for ExecutionEngine type stuff, but it'd be great to have confirmation the API is accepting Vector types as expected (since current tests just use int/floats) and would greatly help with the future subtype refactoring that we planned.

cpdt · 2018-06-24T04:44:09Z

@TheDan64 Yeah, for sure. I was hoping to include tests with the original PR but wasn't actually able to get LLVM to link (Windows ugh). I've managed to get that working in the meantime (since I needed it for the project I've started working on anyway) so I should be able to get those tests in soon :)

cpdt · 2018-06-25T05:39:34Z

@TheDan64 Tests done :)

TheDan64

Looks great, thanks for all the help! Will merge this to master now and get the version branches rebased later today

cpdt added 3 commits June 22, 2018 14:39

Added IntMathValue/FloatMathValue traits to support vector math opera…

25706e9

…tions

Added MathValue constraints to Builder, and type information to trait…

10d1e43

…s necessary

Removed some unnecessary imports

61cd473

TheDan64 self-requested a review June 22, 2018 15:29

TheDan64 assigned cpdt Jun 22, 2018

TheDan64 requested changes Jun 23, 2018

View reviewed changes

cpdt added 4 commits June 23, 2018 14:15

Added vector support for pointer functions

e7b31b8

Allowed vectors to be passed to build_pointer_cast

95db40f

Take ownership of value and type parameters in math functions

4f264fd

Fixed tests and examples from ownership change

9fd5cd1

cpdt force-pushed the feature/45-vector-math-ops branch from c4a65ae to 9fd5cd1 Compare June 23, 2018 05:05

TheDan64 approved these changes Jun 23, 2018

View reviewed changes

Added tests for vector functions on Builder

8cbc981

TheDan64 approved these changes Jun 25, 2018

View reviewed changes

TheDan64 merged commit 50e94ee into TheDan64:master Jun 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support vector types in math ops, fixes #45 #47

Support vector types in math ops, fixes #45 #47

cpdt commented Jun 22, 2018 •

edited

Loading

TheDan64 left a comment

TheDan64 Jun 23, 2018

cpdt Jun 23, 2018

TheDan64 Jun 23, 2018

TheDan64 Jun 23, 2018

cpdt Jun 23, 2018

TheDan64 left a comment •

edited

Loading

cpdt commented Jun 24, 2018

cpdt commented Jun 25, 2018

TheDan64 left a comment

Support vector types in math ops, fixes #45 #47

Support vector types in math ops, fixes #45 #47

Conversation

cpdt commented Jun 22, 2018 • edited Loading

Description

Breaking Changes

How This Has Been Tested

Checklist

TheDan64 left a comment

Choose a reason for hiding this comment

TheDan64 Jun 23, 2018

Choose a reason for hiding this comment

cpdt Jun 23, 2018

Choose a reason for hiding this comment

TheDan64 Jun 23, 2018

Choose a reason for hiding this comment

TheDan64 Jun 23, 2018

Choose a reason for hiding this comment

cpdt Jun 23, 2018

Choose a reason for hiding this comment

TheDan64 left a comment • edited Loading

Choose a reason for hiding this comment

cpdt commented Jun 24, 2018

cpdt commented Jun 25, 2018

TheDan64 left a comment

Choose a reason for hiding this comment

cpdt commented Jun 22, 2018 •

edited

Loading

TheDan64 left a comment •

edited

Loading