[PATCH] D148929: [mlir][test][Integration] Refactor Arm emulator configuration

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

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!



================
Comment at: mlir/test/Integration/Dialect/SparseTensor/CPU/lit.local.cfg:15-16
     config.substitutions.append(('%VLA_ARCH_ATTR_OPTIONS', ''))
-    config.substitutions.append(('%lli', 'lli'))
+    config.substitutions.append(('%lli_host_or_aarch64_cmd', 'lli'))
     config.substitutions.append(('%mlir_native_utils_lib_dir', config.mlir_lib_dir))
----------------
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" .


================
Comment at: mlir/test/Integration/lit.local.cfg:14-15
+
+    config.substitutions.append(('%mlir_native_utils_lib_dir',
+        config.arm_emulator_utils_lib_dir or config.mlir_lib_dir))
+
----------------
[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`)


================
Comment at: mlir/test/Integration/lit.local.cfg:17
+
+    if config.arm_emulator_executable:
+        if not config.arm_emulator_lli_executable:
----------------
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.


================
Comment at: mlir/test/Integration/lit.local.cfg:31
+        emulation_cmd = emulation_cmd + ' ' + lli_cmd
+        # These two substitutions are equivalent but '%lli_aarch64_cmd' should
+        # be used for integration tests that run exclusively on AArch64
----------------
> # These two substitutions are equivalent

I think that this applies to the whole block, because you basically have:
```lang=python
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.
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148929/new/

https://reviews.llvm.org/D148929