Vector: add constructor taking an element reference

[amyspark]

Hi again!

This MR fixes #256. Tested with the following example under MSVC 19.29.30038.1:

#include <Vc/Vc>
#include <iostream>

int main()
{
	Vc::float_v a;

	a[2] = 4.0f;

	Vc::float_v works = (float)a[2];
	Vc::float_v fails = a[2]; // error: cannot convert Vc::float_v::reference to float

	std::cout << works << fails << std::endl;

	return 0;
}

❯ .\build\Debug\main.exe
[4, 4, 4, 4][4, 4, 4, 4]

[bernhardmgruber]

LGTM

I am still not sure however, if there could be any implications. Having this ctor seems so straight forward to me, that there might have been a reason to not include it. I would like to have a second opinion before merging this.

[agheata]

Shouldn't this come with a unit test before merging?

[bernhardmgruber]

Shouldn't this come with a unit test before merging?

Absolutely right! @amyspark could you please add a small unit test as well? Something similar to the snippet you present here. Thank you!

[amyspark]

Shouldn't this come with a unit test before merging?

Absolutely right! @amyspark could you please add a small unit test as well? Something similar to the snippet you present here. Thank you!

Hey! I'm trying to find which unit test location fits this case, but I've not been able to yet.

[amadio]

I agree with Bernhard that there may have been a reason not to include the constructor, e.g. to avoid enabling automatic conversions that are detrimental to performance and/or ambiguities about which constructor to use. However, if it is added as explicit, it is probably ok.

[amyspark]

I see only passes at http://cdash.cern.ch/index.php?project=Vc, anyone knows what's going on with CI?

[amadio]

We are looking into why the CI is showing the builds as failed. We should get this sorted out next week.

[amyspark]

@amadio any news as to CDash?

[amadio]

We think it's an issue with GitHub Actions itself. Nothing has changed on the CI configuration on our side. If GitHub Actions shows failures but we know from CDash that the builds actually passed, we can go ahead and merge the changes. I restarted the jobs to see if it will make any difference.

[amyspark]

It seems it's a known issue with CTest: https://gitlab.kitware.com/cmake/cmake/-/issues/20975

[amyspark]

I've pushed a commit that should resolve those two warnings that appear on CDash.

[bernhardmgruber]

I think I have sorted out the CI issues: #297. You can rebase once the fix is merged.

[bernhardmgruber]

@amyspark please rebase on 1.4 again.

[bernhardmgruber]

@amyspark I think we might also need to add the new overload to SimdArray. Or at least add a unit test, that it also works with SimdArray please!

[amadio]

Can we go with this for all compilers and remove the ifdefs? I think it should be optimal in all cases, no?

[bernhardmgruber]

Depends. I read somewhere that sometimes generating the constant is faster than reading a value from static memory. So we would need to check the generated code whether ~0 is stored in the static data or computed. But then, you are the expert on such matter, so I take your recommendation ;)

[amadio]

It's computed for the other compilers, and I think that's actually ok, because at most that takes a few cycles, so it's like loading from L1. Loading from memory can be fast, or not, depending on where you're loading from.

[bernhardmgruber]

Let's use this implementation on all compilers now, but in a separate PR: #299.