This is an archive of the discontinued LLVM Phabricator instance.

[IR] Add an ImplicitLocOpBuilder helper class for building IR with the same loc.
ClosedPublic

Authored by lattner on Dec 22 2020, 10:35 AM.

Details

Summary

One common situation is to create a lot of IR at a well known location,
e.g. when doing a big rewrite from one dialect to another where you're expanding
ops out into lots of other ops.

For these sorts of situations, it is annoying to pass the location into
every create call. As we discused in a few threads on the forum, a way to help
with this is to produce a new sort of builder that holds a location and provides
it to each of the create<> calls automatically.

This patch implements an ImplicitLocOpBuilder class that does this. We've had
good experience with this in the CIRCT project, and it makes sense to upstream to
MLIR.

I picked a random pass to adopt it to show the impact, but I don't think there is
any particular need to force adopt it in the codebase.

Diff Detail

Event Timeline

lattner created this revision.Dec 22 2020, 10:35 AM
lattner requested review of this revision.Dec 22 2020, 10:35 AM
ftynse accepted this revision.Dec 22 2020, 2:17 PM

Looks great!

mlir/include/mlir/IR/ImplicitLocOpBuilder.h
2

Please add the license header

19–22

Are these necessary? We are now in the mlir:: namespace.

34

std::forward<T>(operand)?

This revision is now accepted and ready to land.Dec 22 2020, 2:17 PM
lattner updated this revision to Diff 313437.Dec 22 2020, 2:37 PM

Fixes AlexZ recommended.

Great catches, fixed. Thanks!

This revision was landed with ongoing or failed builds.Dec 22 2020, 2:47 PM
This revision was automatically updated to reflect the committed changes.
bondhugula added inline comments.
mlir/include/mlir/IR/ImplicitLocOpBuilder.h
22

It would have been good to mention that this should be passed by reference? With the location, this would have four fields.

118

This is missing a doc comment.

lattner added inline comments.Dec 26 2020, 5:49 PM
mlir/include/mlir/IR/ImplicitLocOpBuilder.h
22

I think this should be passed the same way as any builder. Builders that are copied take their state with them. If you pass by reference you maintain it. It is reasonable to splice off the OpBuilder subset of this (by value) if that is the desired behavior.