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

[Hugo Trachino]

nujaa wrote:

Alright. Thanks both for reverting, pointing it out and the motivation. I could not find guidelines on how to revert a commit without going through the MR review process. Do I simply push the revert commit without waiting for approval ?

Before I submit any fix, I would like to make sure of a few things : 
To answer @joker-eph ,
>I'm confused because you're only changing the `ArithToLLVMConversionPass`

If I am not mistaken, `convert-to-llvm` calls `populateConvertToLLVMConversionPatterns` implemented by a `ConvertToLLVMPatternInterface`  extended for Arith behind the `ArithToLLVMDialectInterface`. This patch does adds a call to populateCeilFloorDivExpandOpsPatterns to both `ArithToLLVMConversionPass` (the deprecated pass) AND `ArithToLLVMDialectInterface::populateConvertToLLVMConversionPatterns` (the preferred one). I am not sure if I should do something else to mention it is part of convert-to-llvm.

> wouldn't adding this in "ConvertToLLVMPass.cpp" instead also work?

I think `ConvertToLLVMPass.cpp` implements a shared interface among dialects to lower to LLVM and should not implement a requirement for some ops of a given dialect. 
@banach-space , do you maybe mean adding CeilFloorDivExpandOpsPatterns to `populateArithToLLVMConversionPatterns` which is called in both `ArithToLLVMConversionPass` AND `ArithToLLVMDialectInterface::populateConvertToLLVMConversionPatterns` ? I agree it does factorize the code. Although, `populateArithToLLVMConversionPatterns` calls specifically lowerings of arith ops to LLVM of the type `%OpName%Lowering`. Whereas CeilFloorDivExpandOpsPatterns is not an op lowering to LLVM per se. I am convincible on this, you have the last word.

Thanks again all for your patience.

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