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

Support boolean literals for VectorRepeatValue #4997

Closed
onelson opened this issue Jul 14, 2022 · 2 comments · Fixed by #4998
Closed

Support boolean literals for VectorRepeatValue #4997

onelson opened this issue Jul 14, 2022 · 2 comments · Fixed by #4998
Assignees

Comments

@onelson
Copy link
Contributor

onelson commented Jul 14, 2022

The initial work towards wrapping constants/literals for vectorization only covered a subset of literals (int, float, string time). boolean was intended to be included but I (@onelson) was unable to figure out how to get them working on the rust side.

Rust complained about "undefined identifier" for true and false in tests and the bool literal match arm in vectorize() never seemed to match.

Possibly all that's missing is some configuration of the Analyzer but I wasn't sure how to make it work.

DOD:

  • boolean literals can "vectorize" by being wrapped with ~~vecRepeat~~() calls
  • boolean literals produce VectorRepeatValue instances on the Go side
  • tests for both Rust and Go areas of the code
onelson pushed a commit that referenced this issue Jul 14, 2022
During #4622 lots of attention was given to Vector usage in binary ops
since the literals that could be vectorized tended to show up there.

Since boolean literals were not on the list, I somehow neglected to look
at the logical operators (which depend on booleans to be used).

This diff adds the necessary branching to cover the vec repeat case,
looking forward to the time where boolean literals _can be vectorized_.

It also adds skipped/commented out tests with reference to #4997 which
should aim to enable these code paths by finally allowing bool literals
to become vectorized.
@onelson onelson changed the title Extend supported literals for VectorRepeatValue Support boolean literals for VectorRepeatValue Aug 1, 2022
onelson pushed a commit that referenced this issue Aug 11, 2022
Bool literals were not included in the vec repeat test since they are
still not eligible for vectorization. Ref: #4997
onelson pushed a commit that referenced this issue Aug 12, 2022
Bool literals were not included in the vec repeat test since they are
still not eligible for vectorization. Ref: #4997
onelson pushed a commit that referenced this issue Aug 12, 2022
Bool literals were not included in the vec repeat test since they are
still not eligible for vectorization. Ref: #4997
@onelson
Copy link
Contributor Author

onelson commented Aug 15, 2022

For the "undefined identifier" problem, it seems like the fix is to define the builtin local to the test, as was done here for float:

#5048 (comment)

For booleans, this will mean adding the following to your test source:

builtin true : bool
builtin false : bool

// ... the function to vectorize follows
(r) => ({ r with ... })

@Marwes
Copy link
Contributor

Marwes commented Aug 16, 2022

There is also imp

imp: map![
"path/to/foo" => package![
"f" => " (x: A) => A where A: Addable + Divisible",
],
and env
env: map![
"findColumn" => "() => [A] where A: Record",
"yield" => "(<-tables: stream[A]) => stream[A] where A: Record",
],
to allow for things to be defind before the script.

onelson pushed a commit that referenced this issue Aug 16, 2022
Bool literals were not included in the vec repeat test since they are
still not eligible for vectorization. Ref: #4997
onelson pushed a commit that referenced this issue Aug 16, 2022
Bool literals were not included in the vec repeat test since they are
still not eligible for vectorization. Ref: #4997
onelson pushed a commit that referenced this issue Aug 16, 2022
* feat: rewrite calls to float as _vectorizedFloat

For functions matching our point of entry pattern, `(r) => A`
rewrite calls to `float()`, even those in the stdlib.

This rewrite is naive in that it just plops a new identifier in place of
the old. Unsure if there's a way to do a proper lookup for the real
Symbol or not.

* test: confirm calls to float can vectorize from Go

* test: acceptance test for _vectorizedFloat

Bool literals were not included in the vec repeat test since they are
still not eligible for vectorization. Ref: #4997

* feat: add feature flag for `vectorizedFloat`

* chore: remove fixed fixme

* chore: define builtin float in test instead of loading stdlib

* chore: move float builtin def to analyzer Environment

Feels more official this way.

* chore: make generate
onelson pushed a commit that referenced this issue Aug 17, 2022
During #4622 lots of attention was given to Vector usage in binary ops
since the literals that could be vectorized tended to show up there.

Since boolean literals were not on the list, I somehow neglected to look
at the logical operators (which depend on booleans to be used).

This diff adds the necessary branching to cover the vec repeat case,
looking forward to the time where boolean literals _can be vectorized_.

It also adds skipped/commented out tests with reference to #4997 which
should aim to enable these code paths by finally allowing bool literals
to become vectorized.
onelson pushed a commit that referenced this issue Aug 18, 2022
During #4622 lots of attention was given to Vector usage in binary ops
since the literals that could be vectorized tended to show up there.

Since boolean literals were not on the list, I somehow neglected to look
at the logical operators (which depend on booleans to be used).

This diff adds the necessary branching to cover the vec repeat case,
looking forward to the time where boolean literals _can be vectorized_.

It also adds skipped/commented out tests with reference to #4997 which
should aim to enable these code paths by finally allowing bool literals
to become vectorized.
onelson pushed a commit that referenced this issue Aug 19, 2022
* refactor: add vec repeat support for logical ops

During #4622 lots of attention was given to Vector usage in binary ops
since the literals that could be vectorized tended to show up there.

Since boolean literals were not on the list, I somehow neglected to look
at the logical operators (which depend on booleans to be used).

This diff adds the necessary branching to cover the vec repeat case,
looking forward to the time where boolean literals _can be vectorized_.

It also adds skipped/commented out tests with reference to #4997 which
should aim to enable these code paths by finally allowing bool literals
to become vectorized.

* feat: support vectorized bool literals

* test: verify bool literals can be vectorized in Go/Rust

* fix: add early return for const folding branches in vectorAnd/vectorOr

* test: flux acceptance tests for bool literals/vec repeat

* fix: only rewrite `boolean.true` and `boolean.false` identifiers

Also updates supporting test code to emulate package membership and
prelude.

* fix: only rewrite calls to `universe.float`

Previously we rewrote any call to a function named `float`. This could
cause issues if the regular float had been shadowed.

* chore: combine tests

* chore: make generate
@onelson onelson self-assigned this Aug 19, 2022
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 a pull request may close this issue.

2 participants