[PATCH] D77528: [MLIR] Add support to use aligned_alloc to lower AllocOp from std to llvm

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1404
+  static Optional<int64_t> getAllocationAlignment(Operation *op,
+                                                  bool &useAlignedAlloc) {
+    auto elementType = cast<AllocLikeOp>(op).getType().getElementType();
----------------
Can't you just rely on the Optional result being None to signal that aligned alloc is not required?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1425
+    // if the alignment needed is more than what malloc can provide.
+    if (!allocOp.alignment()) {
+      uint64_t constEltSizeBytes = 0;
----------------
Prefer early return


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1427
+      uint64_t constEltSizeBytes = 0;
+      auto isMallocAlignmentSufficient = [&]() {
+        if (auto vectorType = elementType.template dyn_cast<VectorType>())
----------------
I wouldn't add a lambda that is only called once immediately after its definition.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1485
+      if (useAlignedAlloc)
+        // aligned_alloc(size_t alignment, size_t size)
+        callArgTypes.push_back(getIndexType());
----------------
Nit: putting the comment above `if` makes it less surprising in terms of indentation.  Otherwise, it feels like there is incorrect indentation because of elided braces and the actual statement does not belong to the`if`.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1589
+  // This is the alignment malloc typically provides.
+  constexpr static unsigned kMallocAlignment = 16;
 };
----------------
I am a bit concerned about hardcoding the "typical" value. Can we make it parametric instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528