[Mlir-commits] [mlir] [mlir][LowerToLLVM] Pass DataLayoutAnalysis to LowerToLLVMOptions in `ArithToLLVM`, `ControlFlowToLLVM`, and `VectorToLLVM` passes (PR #206380)

[Andrzej Warzyński]

================

----------------
banach-space wrote:

OK, sorry, I didn't notice the other RUN lines.

Personally, I am against mixing different lowering paths in one test file like this. In Vector, we have separated them, see:
* https://github.com/llvm/llvm-project/blob/4b0b8cbac0f567cdacfde0c81019fc6c0d27f251/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
* https://github.com/llvm/llvm-project/blob/4b0b8cbac0f567cdacfde0c81019fc6c0d27f251/mlir/test/Conversion/VectorToLLVM/vector-to-llvm-interface.mlir

Of course, you are merely working with a pre-existing set-up and trying to keep the changes to the required minimum. I have the following suggestion:
* Add a block comment before the new tests to more visibly sign-post that a distinct kind of logic is exercised.
* `CHECK-DERIVE` --> `CUSTOM_IDX_WIDTH` to make the prefix more descriptive. In particular, using `CHECK` in a custom prefix is IMHO redundant.

Btw, in ideal world, both paths should follow similar logic to configure the width of the index. The divergence is confusing. Would it be possible to fix that?

https://github.com/llvm/llvm-project/pull/206380