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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

This looks good to me. I don't fully understand the problem @rriddle mentioned with `replaceUsesOfWith`, and I wonder if we could have a solution that also avoids the need for the placeholder operation.



================
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);
 
----------------
dcaballe wrote:
> dcaballe wrote:
> > 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.  
> I tried using `replaceAllUsesWith` but it didn't work. The mapping from 'from' to 'to' is needed. I don't see any other option. Any suggestions?
@rriddle could you please elaborate on how exactly it is broken?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:571
 
-    // Convert the original function arguments. Struct arguments are promoted to
-    // pointer to struct arguments to allow calling external functions with
-    // various ABIs (e.g. compiled from C/C++ on platform X).
+  // Convert input FuncOp to a new FuncOp in LLVM dialect by using the
+  // LLVMTypeConverter provided to this legalization pattern.
----------------
Nit: convert input FuncOp to LLVMFuncOp


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:662
+    // have a uniform representation.
+    if (!newFuncOp.getBody().empty()) {
+      Block *firstBlock = &newFuncOp.getBody().front();
----------------
Could we exit early instead?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:679
+              rewriter, funcLoc, lowering, memrefType, arg);
+          rewriter.replaceUsesOfWith(placeHolder, desc);
+        }
----------------
Should we also erase the placeholder op?

This sounds there's a missing feature in the rewriter (or the IR in general) to replace all uses except the given ones...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802