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

Thu Jan 16 06:01:12 PST 2020

nicolasvasilache added a comment.

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

No concerns on the approach on my end, except for the current copypasta :)
Thanks for doing this!

================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:252
+LLVM::LLVMType BarePtrTypeConverter::convertFunctionSignature(
+    FunctionType type, bool isVariadic,
+    LLVMTypeConverter::SignatureConversion &result) {
----------------
dcaballe wrote:
> 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.
+10 for refactoring: when things evolve we don't want to have to evolve 2 independent impls.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802