[PATCH] D75733: Extract common code to deal with multidimensional vectors.

[Alex Zinenko via Phabricator via llvm-commits]

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

A couple of nits



================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:1169
 
+template <unsigned OpCount>
+static bool HandleMultidimensionalVectors(
----------------
Is it really worth it to add a template (and hence increase compile time and binary size) only to forward it to `SmallVector`? I'd just use `4` instead, this should cover the vast majority of cases.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:1170
+template <unsigned OpCount>
+static bool HandleMultidimensionalVectors(
+    Operation *op, ArrayRef<Value> operands, LLVMTypeConverter &typeConverter,
----------------
Please use `LogicalResult` instead of `bool`


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:1171
+static bool HandleMultidimensionalVectors(
+    Operation *op, ArrayRef<Value> operands, LLVMTypeConverter &typeConverter,
+    std::function<Value(LLVM::LLVMType, ArrayRef<Value>)> createOperand,
----------------
Can we use ValueRange instead of ArrayRef<Value>, this is more flexible and ValueRange is implicitly constructible from ArrayRef and other things.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:1189
+    SmallVector<Value, OpCount> extractedOperands;
+    for (auto operand : operands) {
+      extractedOperands.push_back(rewriter.create<LLVM::ExtractValueOp>(
----------------
Nit: drop trivial braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75733