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

[Federico Bruzzone]

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

----------------
FedericoBruzzone wrote:

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

No worries at all :D

> Personally, I am against mixing different lowering paths in one test file like this. In Vector, we have separated them

How about trying to adopt the same approach in this PR? I’m happy to do that.

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

Thanks a lot for the suggestions. If we prefer this approach, I can go ahead with it. Let me know if you prefer to split them up...

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

I had commented on this aspect in the previous reply (genuinely specking), here is what I was saying:

> [Re `convert-to-llvm`](https://github.com/llvm/llvm-project/pull/206380#discussion_r3713490311): the static implementation builds its type converter in `initialize()`, where the IR, and hence `dlti.dl_spec`, isn't available yet, so it silently keeps `i64` even when another pass in the same pipeline derives `i32` from the DL. It's not a deliberate choice, isn't it?

In any case, I want to point out that if this wasn't intentional, I don't think it's an easy fix. I believe the code needs a bit of refactoring. That goes beyond the scope of this PR. But I'd be happy to follow up if this PR isn't what was intended. Let me know 🫶

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