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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked an inline comment as done.
c-rhodes added a comment.

In D148929#4292386 <https://reviews.llvm.org/D148929#4292386>, @awarzynski wrote:

> 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.



================
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))
----------------
awarzynski wrote:
> 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" .
> 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.


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


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

https://reviews.llvm.org/D148929