[Mlir-commits] [mlir] [MLIR][Arith] Add ExpandOps to convertArithToLLVM (PR #117305)

[Andrzej Warzyński]

banach-space wrote:

> If I am not mistaken, convert-to-llvm calls populateConvertToLLVMConversionPatterns implemented by a ConvertToLLVMPatternInterface extended for Arith behind the ArithToLLVMDialectInterface. 

OK, this is what we were missing :) And apologies, I forgot how `-convert-to-llvm` was wired-up. 

Like you said, `ConvertToLLVM` invokes these `populateConvertToLLVMConversionPatterns` methods for every Dialect that implements [ConvertToLLVMPatternInterface](https://github.com/llvm/llvm-project/blob/52b9d0beb6225d1ffe89e2921774f0df0b3e7969/mlir/include/mlir/Conversion/ConvertToLLVM/ToLLVMInterface.h#L24-L46):
https://github.com/llvm/llvm-project/blob/52b9d0beb6225d1ffe89e2921774f0df0b3e7969/mlir/lib/Conversion/ConvertToLLVM/ConvertToLLVMPass.cpp#L118-L122

And that's what's being updated here. But you are also updating the `-convert-arith-to-llvm` path, right? In summary:
* By updating [ArithToLLVMDialectInterface](https://github.com/llvm/llvm-project/blob/52b9d0beb6225d1ffe89e2921774f0df0b3e7969/mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp#L522-L535), you are updating `-convert-to-llvm`. 
* By updating [ArithToLLVMConversionPass](https://github.com/llvm/llvm-project/blob/52b9d0beb6225d1ffe89e2921774f0df0b3e7969/mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp#L494-L513), you keep `-convert-artih-to-llvm` in sync with `-convert-to-llvm`?

Is this accurate?

> Arith Floor and Ceil ops would not get lowered when running --convert-arith-to-llvm.

Considering Mehdi's comment re this path being deprecated in favour of `-convert-to-llvm`, shouldn't this comment focus on `-convert-to-llvm` instead? Also, some element of this discussion should be included in the summary. We shouldn't be required to scan PR discussions to understand the rationale behind a particular change - that's what summaries are for ;-)

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