[PATCH] D76353: [MLIR][LLVM] Make index type bitwidth configurable.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

Have you considered using `LLVMTypeConverterCustomization` for this? If you had and decided otherwise, I'm okay to land this after the comments are addressed.

IMO, it should be now possible to drop the the customization and use the new dispatch features available in TypeConverter.



================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h:64
+    bool emitCWrappers = false,
+    unsigned indexBitwidth = 0);
 
----------------
Nit: use the named constant instead


================
Comment at: mlir/lib/Conversion/GPUCommon/IndexIntrinsicsOpLowering.h:37
 
   static unsigned getIndexBitWidth(LLVMTypeConverter &type_converter) {
+    return type_converter.getIndexType()
----------------
Nit: while you are there, let's use camelBack for variable names.


================
Comment at: mlir/lib/Conversion/GPUCommon/IndexIntrinsicsOpLowering.h:38
   static unsigned getIndexBitWidth(LLVMTypeConverter &type_converter) {
-    auto dialect = type_converter.getDialect();
-    return dialect->getLLVMModule().getDataLayout().getPointerSizeInBits();
+    return type_converter.getIndexType()
+        .getUnderlyingType()
----------------
Can we just have an accessor in the type converter that returns the bitwidth without having to construct the type?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:136
   module = &llvmDialect->getLLVMModule();
+  if (indexBitwidth == kDeriveIndexBitwidthFromDataLayout) {
+    indexBitwidth = module->getDataLayout().getPointerSizeInBits();
----------------
Nit: drop trivial braces


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:139
+  }
+  llvm::errs() << "INDEX SIZE " << indexBitwidth << "\n";
 
----------------
leftover debug stmt


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:3084
+      *this, "index-bitwidth",
+      llvm::cl::desc("Bitwidth of the index type"),
+      llvm::cl::init(LLVMTypeConverter::kDeriveIndexBitwidthFromDataLayout)};
----------------
Consider including the value that leads to the machine word size being used, otherwise it may confuse people to have bitwidth=0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76353