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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
Herald added a subscriber: Joonsoo.

> This is a summary of the pending items that I would suggest addressing separately:
>  <...>

Great, thanks for working on this! I've already started looking at possible solutions (https://reviews.llvm.org/D73702) on my side.



================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:154
 
-  // Extract an LLVM IR dialect type.
-  LLVM::LLVMType unwrap(Type type);
+  // Callbacks for customizing the type conversion.
+  LLVMTypeConverterCustomization customizations;
----------------
dcaballe wrote:
> rriddle wrote:
> > dcaballe wrote:
> > > rriddle wrote:
> > > > These should really be using ///
> > > No strong opinion but just for my understanding... if I remember correctly, `///` is for Doxygen purposes so it's only needed for public interfaces and members (?). That's why `//` is used for private members and .cpp documentation. Isn't that the case? At least that's what I've seen around.
> > > 
> > > LLVM Coding Standards seem a bit ambiguous here:
> > > https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
> > > 
> > > ```
> > > Don’t duplicate the documentation comment in the header file and in the implementation file. Put the documentation comments for public APIs into the header file. Documentation comments for private APIs can go to the implementation file. In any case, implementation files can include additional comments (not necessarily in Doxygen markup) to explain implementation details as needed.
> > > ```
> > I prefer to keep comment style consistent to make the code base uniform, and less error-prone. Refactoring can often make things public that were private, and private that were public. By having a consistent style, there isn't a need to even think about any of that stuff. So for the sake of MLIR I've been trying to make sure that everyone is consistent.
> Ok, that's reasonable. I'll change that before committing. Thanks!
Indeed, we have an (implicit) convention that top-level definitions and class members are commented with `///` (although not all of the code base follows, yet). Let's document it in https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide, we already have some restrictions on top of LLVM's style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802