iree_get_platform return just ${IREE_ARCH}-${CMAKE_SYSTEM_NAME}

[bjacob]

Fixes #12692

[GMNGeoffrey]

I'd like @pzread to weigh in to make sure we're not missing a place this is used. Maybe run the benchmarks on this PR to test also?

[GMNGeoffrey]

Probably makes sense to change the name of this enum

[GMNGeoffrey]

Maybe rename these to match?

Suggested change

	ANDROID_ARMV8_A = "arm_64-Android"
	LINUX_RISCV32 = "riscv_32-Linux"
	LINUX_RISCV64 = "riscv_64-Linux"
	LINUX_X86_64 = "x86_64-Linux"
	ARM_64_ANDROID = "arm_64-Android"
	RISCV_32_LINUX = "riscv_32-Linux"
	RISCV_64_LINUX = "riscv_64-Linux"
	X86_64_LINUX = "x86_64-Linux"

[bjacob]

Sorry I missed your comments here.

[pzread]

Not totally familiar with Android ABI/ARM arch. But will this be a problem if we have Android phones running with newer ARM_64 architecture with other SIMD extensions? Like ARMv9.1-A seems to have new GEMM instructions

Right now we compile these modules with ARMv8-A compile config https://github.com/openxla/iree/blob/123b7e65de1aababae43d8b8364f00e48ac6a714/build_tools/python/e2e_model_tests/test_definitions.py#L30

It's currently just translated into --iree-llvmcpu-target-triple=aarch64-none-android29. But this is something I always want to fix. I think we should be more specific about which SIMD extensions are enabled when compiling these modules for benchmark?

[bjacob]

Sorry I forgot to reply earlier.

It is true that each ISA, in particular arm64, has many optional features, such as SIMD extensions, that may or may not be supported on each CPU, and we may have test variants for each of these, and we may at some point need to implementa the ability to add this level of detail to things like UNSUPPORTED_PLATFORMS or XFAIL_PLATFORMS.

But ARM architecture versions, such as "ARMv8.2-A" or "ARMv9.1", were never a useful proxy for these features, essentially because they are too coarse-grained. Each new ARM architecture version:

• Introduces new set of optional features, which implementations may support.
• Promotes some existing features, that were optional in earlier versions, to mandatory status, so implementations of this new ARM architecture version must support them.

Either way, it's very coarse-grained. There is no way around tracking individual ISA features. And then, the ARM version number doesn't matter to us at all anymore, it's redundant with the tracking of individual features. That's why I think of it as "arm64+dotprod" rather than "ARMv8.2+dotprod". It doesn't matter to us that ARMv8.2 is the version where dotprod started to exist as an optional feature.

[GMNGeoffrey]

But wouldn't we then need to specify every ISA feature that is mandatory in ARMv8.2 in addition to +dotprod? Whereas ARMv8.2 would be a shorthand for "all the features mandatory in this version". Unless we don't expect that we will frequently need to care about specific ISA features and so generally "arm64" implies almost all the features we care about on its own (back to the first arm64 version, I guess). Maybe in the specific case of dotprod, the fact that it's +dotprod already implies ARMv8.2 because that's the only version where its optional (and it must imply at least v8.2), but then that means if we drop the "+dotprod" we no longer have an indication of ARM version. So I think it's useful to have arm_64-Android as a thing, but we probably also want a way to say "plus all the ISA features required in v8.2). It's also a question of how LLVM wants it specified for codegen. If it wants us to say ARMv8.2 somewhere then it would probably be nicer to have that rather than reverse-engineering the version from the fact that it has optional dotprod

[bjacob]

Unless we don't expect that we will frequently need to care about specific ISA features and so generally "arm64" implies almost all the features we care about on its own (back to the first arm64 version, I guess)

Yes, that's exactly the situation with arm64. arm64 is a fairly recent architecture and "going all the way back to the baseline" isn't going that far back. "dotprod" was the first non-baseline feature that we cared about. It wasn't quite the first interesting non-baseline feature --- ARMv8.1 introduced interesting atomic instructions and some interesting fp16 arithmetic --- but it was the first were the benefit was so large (4x speedup) that we decided to go through all the hassle of dealing with non-baseline features. And to date, the only 2 non-baseline arm64 features that we care about are "dotprod" and "i8mm".

So when we have been passing the compiler flag -march=armv8.2-a+dotprod, we never cared about the armv8.2 part per se -- we just had to pass it for +dotprod to be accepted as an optional feature to enabled. So it is not a problem for us that "we no longer have an indication of ARM version".

it would probably be nicer to have that rather than reverse-engineering the version from the fact that it has optional dotprod

I don't know about that! To me, ARM version numbers are meant for, say, hardware makers licensing the architecture, implementing it in their hardware, and certifying it as implementing that ARM version. As a software author I just don't feel concerned at all with ARM versions. When the compiler requires me to specify one as in -march=armv8.2-a+dotprod I regard that as a local annoyance that I would rather contain to the part of my build system that deals with that, rather than propagate that.

[pzread]

I see, I'm fine to not use "armv8.2-a", if "arm64+dotprod" is a better way to describe the ISA we want to enable. The build system can generate the proper flags from the name later.

I think another main point is that "${IREE_ARCH}-${CMAKE_SYSTEM_NAME}" might not be sufficient here? IMO IREE_ARCH is for the place we don't care about the ISA extensions, while the e2e benchmark tests here care about the ISA extensions.

Not very sure about the solution. One way is asking users to specify an extra CMake variable IREE_ARCH_FEATURES. So ${IREE_ARCH}-${IREE_ARCH_FEATURES}-${CMAKE_SYSTEM_NAME} gives us enough details about the host environment. If IREE_ARCH_FEATURES is not set, no e2e benchmark tests will be run.

Feel like this is a similar situation as iree_generated_trace_runner_test. Those tests requiring special ISA have labels to indicate the requirements. And in the build_tools/cmake/ctest_all.sh, by default we disable those tests that require special ISA, unless the feature variable is set.

[bjacob]

Good point that some tests, like iree_generated_trace_runner_test, care about ISA extensions.

In this PR, for now, I was only looking at the current users of iree_get_platform, which do not currently care about ISA extensions.

Note that the difference between ISA extensions and the other variables here (arch, system name) goes deeper than that. In a given CMake build directory, we are consistently targeting 1 specific arch and 1 specific system name. so ${IREE_ARCH}-${CMAKE_SYSTEM_NAME} is a single constant value across the entire build directory. If one wants to test multiple values of this, one makes multiple separate build directories.

By contrast, ISA extensions are per-target; they vary within one build dir. This is so for a few reasons:

• There are potentially many values, and it would not be convenient to ask users to specialize build dirs for each.
• One test device typically supports many values, and one would like to get all the test coverage possible on this test device at once. Example: a Moto Edge X30 supports baseline arm64, arm64+dotprod, arm64+i8mm. It's useful to be able to test all 3 at once. Otherwise, when I make changes to arm64 codegen, I would need to go test in 3 separate build dirs.
• The exact set of features supported by the test device may not be convenient to hardcode in the build dir. This means that not only we avoid specializing the build dir to that, but the ISA-extension-specific targets need to be able to do a runtime query and automatically skip testing unsupported features. This is arguably less than ideal but the alternatives tend to be worse.

Because of how different ISA extensions are compared to arch and system name, I was assuming that we would want to focus only on the latter here, treating ISA extensions as out of scope here.

[bjacob]

It would still be possible, and a good idea, to make things like iree_generated_trace_runner_test able to support UNSUPPORTED_PLATFORMS and XFAIL_PLATFORMS, making them start to care about iree_get_platform. In my opinion that's fine, and orthogonal to these tests dealing with ISA extensions:

• Filtering by arch and system name can happen in CMake.
• Filtering ISA extensions has to happen within the C code anyway and be evaluated at runtime anyway based on the actual test device. The aspects of ISA extensions that need to be reflected in CMake are about enumerating the ISA variants to build and the CFLAGS that they imply, but that's orthogonal to actually running or skipping them at runtime.

[pzread]

Eventually we should care about the ISA extension here, because these tests test against compiled benchmark modules, which are ISA-extension-specific targets. Therefore right now these tests are set up incorrectly. I'm fine to keep the current state for now, as long as we have a way to make them ISA-extension-specific.

For the perspective of CI, I'm a little bit worried about ISA-extension-specific targets need to be able to do a runtime query and automatically skip testing unsupported features. As we might skip some tests in CI without knowing (wrong device setup) and it's not easy to know what is tested on CI (we don't know what will be run before we run it). But I agree it's really important for local development.

I think in CI we should have a way to specify the set of ISA-extension-specific targets to run and fail if things go wrong, in addition to auto-detection

[bjacob]

I agree that ideally in CI we would know exactly what features are supported and fail if they aren't rather than silently continue. This requires having dedicated CI scripts for each class of CI test host, like we do with the benchmark runners, https://github.com/openxla/iree/blob/31bbc9597713acd856a806fae56545e3e148f8cc/build_tools/buildkite/cmake/android/arm64-v8a/pipeline.yml#L21-L66
Wherever we have that prerequisite, sure let's fail hard. Retaining the option of skipping may remain useful to keep it easy to generalize to new test targets where it may not be easy to know each target machine's capabilities. E.g. imagine that we port to a new architecture where we can use a pool of CI devices outside of our direct control (maybe provided by a 3rd party) with uneven capabilities and no way to tell what we got to run on prior to actually starting running on it.