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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked 7 inline comments as done.
bondhugula added a comment.

Thanks for the review.



================
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();
----------------
ftynse wrote:
> Can't you just rely on the Optional result being None to signal that aligned alloc is not required?
We still use aligned_alloc if the elt size is > 16 bytes (in spite of no alignment attribute). The code would otherwise crash since the loads/stores won't have alignment guaranteed (with malloc) but the they would be lowered by LLVM via aligned load/stores. (LLVM in the absence of alignment attributes on load/store would assume ABI alignment which for say vector<8xf32> elt types would be 256-bit boundaries). 


================
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;
----------------
ftynse wrote:
> Prefer early return
Thanks. 


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1427
+      uint64_t constEltSizeBytes = 0;
+      auto isMallocAlignmentSufficient = [&]() {
+        if (auto vectorType = elementType.template dyn_cast<VectorType>())
----------------
ftynse wrote:
> I wouldn't add a lambda that is only called once immediately after its definition.
Hmm... this is just for better readability - it gives a name / auto documents a code block without the need to outline it into a function or add an explicit comment. I've seen this as a standard practice.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1485
+      if (useAlignedAlloc)
+        // aligned_alloc(size_t alignment, size_t size)
+        callArgTypes.push_back(getIndexType());
----------------
ftynse wrote:
> 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`.
Sure, thanks. 


================
Comment at: mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp:1589
+  // This is the alignment malloc typically provides.
+  constexpr static unsigned kMallocAlignment = 16;
 };
----------------
ftynse wrote:
> I am a bit concerned about hardcoding the "typical" value. Can we make it parametric instead?
I had sort of a similar concern. But 16 bytes is pretty much what glibc malloc gives on nearly every system we have (on probably really old ones, it was perhaps 8 bytes). Did you want a pass flag and then letting 16 be the default - that would be too much plumbing (just like alignedAlloc). This is already a parameter of sorts. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77528