[SVE] Add support for scalable data type strings by lhutton1 · Pull Request #16612 · apache/tvm

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 · 2024-02-20T10:28:45Z

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true).

Co-authored-by: Elen Kalda elen.kalda@arm.com
Co-authored-by: Neil Hickey neil.hickey@arm.com

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com> Change-Id: I883dbdf29f500cc6bf6f9dc3097fa1d5af12b020

Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

lhutton1 · 2024-02-21T11:03:22Z

@tvm-bot rerun

ekalda

Thanks @lhutton1 LGTM in a general, just one comment :)

ekalda · 2024-02-22T16:21:17Z

 }
 /*! \return whether type is a scalar type. */
- bool is_scalar() const { return lanes() == 1; }
+ bool is_scalar() const { return !is_scalable_vector() && lanes() == 1; }


I think it would make more sense to use !is_scalable_or_fixed_length_vector() here. The current version is going to throw an error if applied on a scalable vector.

Due to placing a call to is_scalable_vector() before lanes(), is_scalar() should work with scalable vectors in its current form, I'll add some tests for this. I don't think we can simply return !is_scalable_or_fixed_length_vector() as it wouldn't account for the case when lanes == 0.

Ok yes, thinking about it, this probably is the most solid way to do it, even though it reads a bit awkward 👍

Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda

LGTM!

ekalda · 2024-02-27T09:24:53Z

Thanks @lhutton1!

tqchen · 2024-02-27T13:06:43Z

there is a regression due to concurrent merge, cc @Lunderberg

lhutton1 · 2024-02-27T13:17:38Z

thanks for raising @tqchen, I'm taking a look now

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI. Change-Id: If1b06cb836d62e85620971fd1ad0498c9fdc66b9

lhutton1 · 2024-02-27T13:36:52Z

Apologies for missing this previously, I've uploaded #16649 which I'd expect to fix the failing test case

ekalda · 2024-02-27T13:46:40Z

Any idea why the CI didn't catch this?

lhutton1 · 2024-02-27T14:15:15Z

CI was last run on the patch yesterday (26th Feb) while the PR that caught the issue was merged a week ago #16563. I didn't rebase before pushing, but had expected CI to do this automatically.

I think it might have something to do with a missing call to merge_with_main()(link) in the unity_jenkinsfile.groovy file since the other jenkinsfiles used in CI make calls to this, which presumably updates the PR branch before CI is run

A default value for lanes was unintentionally removed in #16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

This commit adds support for representing scalable vectors using the string data type format. For example, "float32xvscalex4" may be used to represent the following scalable type: `DataType(kDLFloat, 32, /*lanes=*/4, /*is_scalable=*/true)`. --------- Co-authored-by: Elen Kalda <elen.kalda@arm.com> Co-authored-by: Neil Hickey <neil.hickey@arm.com>

A default value for lanes was unintentionally removed in apache#16612, this PR fixes this which in turn fixes the test failure seen in `test_tensor_dtype_lanes` in CI.

lhutton1 mentioned this pull request Feb 20, 2024
[Tracking Issue] Scalable Vector Extension (SVE) upstreaming #16455
Closed
12 tasks

fix i386
94d6389
Change-Id: Iea28d0c276eed75e9dc2d2f17ea153ad7100adad

ekalda requested changes Feb 22, 2024
View reviewed changes

add is_scalar tests
25db1ac
Change-Id: I3e6c56a0f1e171278c50fefe10bddd1feeca1b69

ekalda approved these changes Feb 26, 2024
View reviewed changes

ekalda merged commit 563ef95 into apache:mainFeb 27, 2024

lhutton1 mentioned this pull request Feb 27, 2024
[BugFix][FFI] Add a missing default for datatype lanes #16649
Merged

lhutton1 deleted the p3-scalable-dtype-strings branch February 28, 2024 13:40

lhutton1 mentioned this pull request Feb 29, 2024
[CI Problem] unity/pr-head does not automatically rebase against main before CI run #16657
Closed

ysh329 mentioned this pull request Apr 21, 2024
[Release] v0.16.0 Release Candidate Notes #16911
Closed