cranelift: Use requested ISA Flags in run tests

[afonso360]

👋 Hey,

This PR Enables using the ISA flags that run tests request. We skip tests that request flags that the host arch does not support.

I did a quick pass on the backends to find ops that we have optimizations based on flags and enabled the tests for those.

Fixes #1891

[jameysharp]

Tell me if I've understood the implications of this change correctly:

It looks like the existing behavior is to select an ISA with all native-supported features enabled.

The new behavior is to select an ISA with all features disabled unless you specifically ask for them, and then skip the test if the features you asked for aren't available.

Is that right?

That seems like a good move to me: I'm not sure I like the idea that the same tests check different things depending on which host machine you run them on. Being explicit that some tests are useful to run both with and without particular hardware features seems great.

But I'd like to get @alexcrichton or somebody to double-check, or @cfallin when he's back next week.

[jameysharp]

Can we have a more helpful message in case somebody tries putting a non-boolean flag in a test? Something like:

Suggested change

	_ => unimplemented!(),
	k => unimplemented!("ISA flag {} of kind {:?}", value.name, k),

It's not really important if there's a similar check added to is_isa_compatible as that will get hit first, but I'd still do it both places just in case.

[afonso360]

I'm going to backport what I did in #4453 which adds a value_string function and makes this a whole lot cleaner, and supports all flag kinds.

[afonso360]

It looks like the existing behavior is to select an ISA with all native-supported features enabled.

The new behavior is to select an ISA with all features disabled unless you specifically ask for them, and then skip the test if the features you asked for aren't available.

Is that right?

Yes, except that we don't start with all features disabled, we start with the defaults enabled by cranelift. So on x86_64 for example we start with has_sse41 and a bunch of others. These can be explicitly disabled to test the lowering without those features.

Starting from an empty set of features would be valuable, I think we can try that. But I'd like to hear what people think is preferable.

Also CC @abrown and @bnjbvr from the original issue #1891

[alexcrichton]

Seems reasonable to me!

We technically already had a problem where if Cranelift was buggy missing a feature flag then if you ran the tests on that host without the feature flag tests would fail where tests run on a host with the feature flag would not fail. Overall this seems like a better state of things where tests are run independent of the host executing the test, and if a test fails then it should fail on any host which can execute the test.

[jameysharp]

Great, I'm going to merge this then. Separately we can experiment with going further by turning off all feature flags by default during these tests, but I'm guessing that will involve more noise, updating a bunch of tests to enable features that were default before.