[PATCH] D72802: [mlir] Introduce bare ptr calling convention for MemRefs in LLVM dialect

[Diego Caballero via Phabricator via llvm-commits]

dcaballe marked 3 inline comments as done.
dcaballe added a comment.

Thanks, River! Some comments inline.
I think I can refactor some of the code in the patterns but I'd like to know if there is any major concern with this approach before moving forward.



================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:324
   /// Replace all the uses of the block argument `from` with value `to`.
-  void replaceUsesOfBlockArgument(BlockArgument from, Value to);
+  void replaceUsesOfWith(Value from, Value to);
 
----------------
rriddle wrote:
> This function is already broken, I'd rather not expand its scope until its fixed.
It's currently being used. Why is it broken?
Maybe I could just replace all the uses (lines 683 and 687) with other utilities but I thought adding a mapping from 'from' to 'to' to pattern-rewriter would be necessary.  


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:252
+LLVM::LLVMType BarePtrTypeConverter::convertFunctionSignature(
+    FunctionType type, bool isVariadic,
+    LLVMTypeConverter::SignatureConversion &result) {
----------------
rriddle wrote:
> This looks like a lot of code duplication.
We discussed that concern in our initial thread (https://github.com/tensorflow/mlir/issues/309#issuecomment-568791098). However, I think that keeping a separate pattern, as suggested, despite some code duplication might be worth it to keep each pattern simple. Otherwise, we would end up with a complex pattern with many special cases.

I could try to refactor some of this code to protected members in LLVMTypeConverter but I think it's going to look a bit odd since the differences between the two patterns are small and scattered.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:645
+    TypeConverter::SignatureConversion result(funcOp.getNumArguments());
+    auto llvmType = lowering.convertFunctionSignature(
+        funcOp.getType(), varargsAttr && varargsAttr.getValue(), result);
----------------
rriddle wrote:
> Same in this pattern.
Same, I could try to refactor some code to a common base class for both patterns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802