Page MenuHomePhabricator

[MLIR] Replace OpBuilder(Block) with atBlockBegin and atBlockEnd
ClosedPublic

Authored by tpopp on Mar 30 2020, 7:57 AM.

Details

Summary

OpBuilder(Block) is specifically replaced with OpBuilder::atBlockEnd(Block);

This is to make insertion behavior clear due to there being no one
correct answer for which location in a block the default insertion
point should be.

Diff Detail

Event Timeline

tpopp created this revision.Mar 30 2020, 7:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
ftynse added a subscriber: ftynse.Mar 30 2020, 9:49 AM

Could we rather have static constructors that make it explicit what the insertion point is?

Could we rather have static constructors that make it explicit what the insertion point is?

Static builders (assuming you mean OpBuilder b = OpBuilder::Get(body, body->begin()) ) and an explicit insertion point are orthogonal right? Or do you mean OpBuilder::AtBlockBegin(body)?

Indeed, I mean adding OpBuilder::atBlockBeginand OpBuilder::atBlockEnd and removing the default constructor. Deferring to @rriddle though :)

I prefer the explicit version as well.
In the past I tried to change OpBuilder(Operation *) in a similar way by forcing an enum: OpBuilder builder(op, OpBuilder::After) / OpBuilder builder(op, OpBuilder::Before), but a factory method is good as well.

I'm +1 on the explicit factory methods

tpopp updated this revision to Diff 253798.Mar 31 2020, 1:11 AM

Create AtBlockBegin and AtBlockEnd. Remove OpBuilder(Block).

tpopp retitled this revision from [MLIR] Make OpBuilder(Block) default to inserting at Block::begin(). to [MLIR] Replace OpBuilder(Block) with AtBlockBegin and AtBlockEnd.Mar 31 2020, 1:13 AM
tpopp edited the summary of this revision. (Show Details)
ftynse added inline comments.Mar 31 2020, 1:40 AM
mlir/include/mlir/IR/Builders.h
198

Nit: MLIR's convention for function names is to start with a small letter.

tpopp updated this revision to Diff 253835.Mar 31 2020, 4:21 AM

Follow naming convention for atBlockBegin and atBlockEnd.

tpopp retitled this revision from [MLIR] Replace OpBuilder(Block) with AtBlockBegin and AtBlockEnd to [MLIR] Replace OpBuilder(Block) withatBlockBegin and atBlockEnd.Mar 31 2020, 4:21 AM
tpopp edited the summary of this revision. (Show Details)
tpopp retitled this revision from [MLIR] Replace OpBuilder(Block) withatBlockBegin and atBlockEnd to [MLIR] Replace OpBuilder(Block) with atBlockBegin and atBlockEnd.
ftynse accepted this revision.Mar 31 2020, 4:26 AM
tpopp marked an inline comment as done.Mar 31 2020, 4:29 AM
rriddle accepted this revision.Mar 31 2020, 11:48 AM
rriddle added inline comments.
mlir/include/mlir/IR/Builders.h
196

This description doesn't really describe this function.

This revision is now accepted and ready to land.Mar 31 2020, 11:48 AM
This revision was automatically updated to reflect the committed changes.