Page MenuHomePhabricator

[MLIR] Introduce std.alloca op
Needs ReviewPublic

Authored by bondhugula on Mon, Mar 23, 5:21 AM.

Details

Summary

Introduce the alloca op for stack memory allocation. When converting to the
LLVM dialect, this is lowered to an llvm.alloca.

Signed-off-by: Uday Bondhugula <uday@polymagelabs.com>

Diff Detail

Event Timeline

bondhugula created this revision.Mon, Mar 23, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 23, 5:21 AM
nicolasvasilache requested changes to this revision.Mon, Mar 23, 5:57 AM

The amount of copy-pasta is uncanny, is there a way to factor out the 90%+ common part?
Also, please use the assemblyFormat for parsing and printing,

This revision now requires changes to proceed.Mon, Mar 23, 5:57 AM

Also, please use the assemblyFormat for parsing and printing,

Can the assembly format support its parsing and printing? Both affine.apply and alloc don't use the assembly format and have identical operand syntax (affine.apply was recently migrated to ODS but not auto print / parse). All these three require two variadic operand lists. Let me know.

The amount of copy-pasta is uncanny, is there a way to factor out the 90%+ common part?
Also, please use the assemblyFormat for parsing and printing,

I was thinking the same. The duplication is unfortunate and it's all in StandardToLLVM - it's possible to factor it all out if we get rid of useAlloca because the AllocOp lowering is conditional on that. Should I remove useAlloca in this patch itself and mostly merge AllocOpLowering and AllocaOpLowering? Merging the parse/print methods is straightforward.

Share print/parse/verify b/w alloc/alloca.

Refactor ODS for alloc/alloca

Register canonicalization pattern as well

Harbormaster failed remote builds in B50259: Diff 252331!

The amount of copy-pasta is uncanny, is there a way to factor out the 90%+ common part?

I was thinking the same. The duplication is unfortunate and it's all in StandardToLLVM - it's possible to factor it all out if we get rid of useAlloca because the AllocOp lowering is conditional on that. Should I remove useAlloca in this patch itself and mostly merge AllocOpLowering and AllocaOpLowering? Merging the parse/print methods is straightforward.

@nicolasvasilache Done with the refactoring here (except for the llvm lowering - see question above).

rriddle added inline comments.Tue, Mar 24, 5:33 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
144

Use ValueRange instead of ArrayRef<Value> in builder mehtods.

260

nit: "alloca" -> alloca

264–266

Please use mlir code blocks for any inline code.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
250

llvm::is_one_of

bondhugula marked 5 inline comments as done.

Address comments from @rriddle

Thanks for the review!

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
250

Thanks!

Thanks for adding the alloca op! Really needed.
Not sure if you discussed this already but just a nit about the name: any plans on renaming alloca and alloc so that it's a bit clearer what they model? I find it a bit confusing right now. Some options that came to mind:
alloca -> salloc, salloca
alloc -> malloc, malloca, alloc

Thanks for adding the alloca op! Really needed.
Not sure if you discussed this already but just a nit about the name: any plans on renaming alloca and alloc so that it's a bit clearer what they model? I find it a bit confusing right now. Some options that came to mind:
alloca -> salloc, salloca
alloc -> malloc, malloca, alloc

alloca -> salloca sounds good to me! Note that the 'a' suffix in 'alloca' is for 'automatic' freeing (originating from early Unix's and BSDs and it has always meant allocating from the caller's stack). So, alloc -> malloca, alloca -> salloc would be inconsistent. Since 'alloc' currently doesn't specify where it's from the stack/heap and specifies it's explicitly freed via dealloc, we can leave it like that.

What is the s in salloc? Stack? Can we make it explicit then: stack_alloc?

Note that the 'a' suffix in 'alloca' is for 'automatic' freeing

Thanks for clarifying! I thought it was just a short for 'allocate' :)

Can we make it explicit then: stack_alloc?

stack_alloc sounds better to me, thanks.

Since 'alloc' currently doesn't specify where it's from the stack/heap and specifies it's explicitly freed via dealloc, we can leave it like that.

It sounds good. I think we are mapping 'alloc' to static allocation by using a flag in the llvm lowering. Maybe can create a simple pass to do a more proper alloc->static_alloc conversion in the future and leave 'alloc' only for heap allocation.

@bondhugula
Thanks for the refactoring!

Dropping the LLVM flag and refactoring the impl further sounds like the right thing to do IMO.
The unit test that uses the flag can easily be updated (or deprecated in favor of your test).
The internal use case we have for this will be easy to update.

@bondhugula
Thanks for the refactoring!

Dropping the LLVM flag and refactoring the impl further sounds like the right thing to do IMO.
The unit test that uses the flag can easily be updated (or deprecated in favor of your test).
The internal use case we have for this will be easy to update.

This sounds the best to me too. I'll do it in this patch.