This is an archive of the discontinued LLVM Phabricator instance.

[mlir][test][Integration] Refactor Arm emulator configuration
ClosedPublic

Authored by c-rhodes on Apr 21 2023, 8:33 AM.

Details

Summary

The logic enabling the Arm SVE (and now SME) integration tests for
various dialects, that may run under emulation, is now duplicated in
several places.

This patch moves the configuration to the top-level MLIR integration
tests Lit config and renames the '%lli' substitution in contexts where
it will run exclusively (ArmSVE, ArmSME) on AArch64 (and possibly under
emulation) to '%lli_aarch64_cmd', and '%lli_host_or_aarch64_cmd' for
contexts where it will run on the host OR AArch64 (also possibly under
emulation). The latter is for integration tests that have
target-specific and target-agnostic codepaths such as SparseTensor,
which supports scalable vectors.

The two substitutions have the same effect but the names are different to
convey this information. The '%lli_aarch64_cmd' substitution could be
used in the SparseTensor tests but that would be a misnomer if the host
were x86 and the MLIR_RUN_SVE_TESTS=OFF.

The reason for renaming the '%lli' substitution is to not prevent running other
target-specific integration tests at the same time, since the same substitution
'%lli' is used for lli in other integration tests:

  • mlir/test/Integration/Dialect/Vector/CPU/X86Vector - (AVX emulation via Intel SDE)
  • mlir/test/Integration/Dialect/Vector/CPU/AMX - (AMX emulation via Intel SDE)
  • mlir/test/Integration/Dialect/LLVMIR/CPU/test-vp-intrinsic.mlir - (RISCV emulation via QEMU if supported, native otherwise)

and substituting '%lli' at the top-level with Arm specific logic would override
this.

Diff Detail

Event Timeline

c-rhodes created this revision.Apr 21 2023, 8:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 8:33 AM
c-rhodes requested review of this revision.Apr 21 2023, 8:33 AM

Many thanks for preparing this, @c-rhodes ! I am very much in favor of replacing generic LIT variables like %lli (that change meaning depending on context) with some more explicit variables. Also, it's always nice to see code being removed :)

These changes make a lot of sense to, though it seems there's room for a couple more improvements. Could you take a look? Ta!

mlir/test/Integration/Dialect/SparseTensor/CPU/lit.local.cfg
14–15

It feels that these two could be moved to "mlir/test/Integration/lit.local.cfg" too? This way, this config would only contain things "specific" to SparseTensor tests (these two LIT variables are more generic than that).

Otherwise, it would be good to clarify the relationship between this and what's in "mlir/test/Integration/lit.local.cfg" .

mlir/test/Integration/lit.local.cfg
14–15

[nit] Move "unconditional" code to the top of this block (because this applies to all configs, irrespective of the value of e.g. arm_emulator_lli_executable)

17

This would make a bit more sense to me:

if not config.arm_emulator_executable:
    config.substitutions.append(('%lli_aarch64_cmd', lli_cmd))
    config.substitutions.append(('%lli_host_or_aarch64_cmd', lli_cmd))
else
  (...)

This way, it's immediately obvious that the purpose of these two blocks is to set-up substitutions for '%lli_aarch64_cmd and %lli_host_or_aarch64_cmd. Otherwise that's obfuscated a bit.

31
  1. These two substitutions are equivalent

I think that this applies to the whole block, because you basically have:

if config.mlir_run_arm_sve_tests or config.mlir_run_arm_sme_tests:
        config.substitutions.append(('%lli_aarch64_cmd', emulation_cmd))
        config.substitutions.append(('%lli_host_or_aarch64_cmd', emulation_cmd))
    else:
        config.substitutions.append(('%lli_aarch64_cmd', lli_cmd))
        config.substitutions.append(('%lli_host_or_aarch64_cmd', lli_cmd))

So I would move this particular comment and also clarify, e.g.:

This block configures substitutions for:
*  `%lli_aarch64_cmd`, and 
* `%lli_host_or_aarch64_cmd'`. 

The former is only relevant/required when SVE integrations tests are enabled. The latter changes meaning depending on whether those tests are enabled. When the SVE tests _are_ enabled then both are equivalent.

This is absolutely LGTM for the sparse part, thanks for following up
(I leave the arms specific review to the experts, but the sparse tests look very clean now)

Matt added a subscriber: Matt.Apr 24 2023, 2:04 PM
c-rhodes updated this revision to Diff 516775.Apr 25 2023, 6:20 AM

Move '%lli_host_or_aarch64_cmd' -> 'lli' substitution from SparseTensor Lit config to top-level and move logic to function.

c-rhodes marked an inline comment as done.Apr 25 2023, 6:24 AM

Many thanks for preparing this, @c-rhodes ! I am very much in favor of replacing generic LIT variables like %lli (that change meaning depending on context) with some more explicit variables. Also, it's always nice to see code being removed :)

These changes make a lot of sense to, though it seems there's room for a couple more improvements. Could you take a look? Ta!

Cheers Andrzej, updated.

mlir/test/Integration/Dialect/SparseTensor/CPU/lit.local.cfg
14–15

It feels that these two could be moved to "mlir/test/Integration/lit.local.cfg" too? This way, this config would only contain things "specific" to SparseTensor tests (these two LIT variables are more generic than that).

Otherwise, it would be good to clarify the relationship between this and what's in "mlir/test/Integration/lit.local.cfg" .

I've moved '%lli_host_or_aarch64_cmd' -> 'lli' to top-level. %mlir_native_utils_lib_dir is also used by RISCV in mlir/test/Integration/Dialect/LLVMIR/CPU/test-vp-intrinsic.mlir so moving this substitution to top-level and predicating on !config.mlir_run_arm_sve_tests would break that test unless additional logic was added to handle that, so I think it's simpler to keep it here.

mlir/test/Integration/lit.local.cfg
17

This would make a bit more sense to me:

if not config.arm_emulator_executable:
    config.substitutions.append(('%lli_aarch64_cmd', lli_cmd))
    config.substitutions.append(('%lli_host_or_aarch64_cmd', lli_cmd))
else
  (...)

This way, it's immediately obvious that the purpose of these two blocks is to set-up substitutions for '%lli_aarch64_cmd and %lli_host_or_aarch64_cmd. Otherwise that's obfuscated a bit.

I've refactored this into a function, I think it's cleaner now and addresses your comments, let me know if not.

awarzynski accepted this revision.Apr 26 2023, 12:27 AM

LGTM, this is a massive improvement, thanks!

mlir/test/Integration/lit.local.cfg
38–42

[nit] I think that splitting this into bullet points will improve readability (that's the only change that I am suggesting)

This revision is now accepted and ready to land.Apr 26 2023, 12:27 AM

LGTM, this is a massive improvement, thanks!

Cheers Andrzej. I'll address your nit before committing.

This revision was landed with ongoing or failed builds.Apr 26 2023, 2:58 AM
This revision was automatically updated to reflect the committed changes.
c-rhodes marked an inline comment as done.Apr 26 2023, 3:08 AM