This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ods] Unique attribute, successor, region constraints
ClosedPublic

Authored by Mogball on Nov 5 2021, 10:15 PM.

Details

Summary

With -Os turned on, results in 2-5% binary size reduction
(depends on the original binary). Without it, the binary size
is essentially unchanged.

Depends on D113128

Diff Detail

Event Timeline

Mogball created this revision.Nov 5 2021, 10:15 PM
Mogball requested review of this revision.Nov 5 2021, 10:15 PM
Mogball added inline comments.Nov 5 2021, 10:29 PM
mlir/include/mlir/TableGen/CodeGenHelpers.h
108–109

Heh.

Mogball edited the summary of this revision. (Show Details)Nov 5 2021, 10:32 PM
Mogball updated this revision to Diff 385524.Nov 8 2021, 9:15 AM

Fixing some comments and cleaning up code

Mogball updated this revision to Diff 385527.Nov 8 2021, 9:18 AM

Actually updated the patch this time

jpienaar added inline comments.Nov 8 2021, 4:53 PM
mlir/include/mlir/TableGen/Predicate.h
66

We mostly try to not expose the underlying Tablegen types directly here, why is this needed? (And could it be avoided)

Mogball added inline comments.Nov 8 2021, 5:13 PM
mlir/include/mlir/TableGen/Predicate.h
66

Needed for ConstraintMap. I think I can get rid of this.

Mogball updated this revision to Diff 385931.Nov 9 2021, 12:22 PM

Rebase and review comments

Mogball marked an inline comment as done.Nov 9 2021, 12:24 PM
Mogball added a reviewer: Chia-hungDuan.
rriddle requested changes to this revision.Nov 9 2021, 12:32 PM
rriddle added inline comments.
mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
211–216

The tombstone shouldn't be the same as the empty key.

221–226

Is this providing additional uniquing power over the def pointer? This is now doing string comparison. I would rather expose the raw pointer than do this.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2338

Why?

This revision now requires changes to proceed.Nov 9 2021, 12:32 PM
Mogball marked an inline comment as done.Nov 9 2021, 1:00 PM
Mogball added inline comments.
mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
221–226

Yes it is. Constraints are being uniqued by their predicate (pointer value) and description (string value). I'm using this ConstraintUniquer to do it all at once, instead of whatever's going on in StaticVerifierFunctionEmitter::emitConstraintMethods

Chia-hungDuan added inline comments.Nov 9 2021, 2:28 PM
mlir/include/mlir/TableGen/CodeGenHelpers.h
104

Why do we need the "Local" here?

159–160

Do we need to change the name for this? It seems a little bit confusing with the other one.

205

How about let's have an enum class for this?

mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
193–194

Can we concatenate them?

234–240

Sorry, I'm not following. Can you give me an example that we can't turn attribute verifier into a function? We fill the placehold $_self and $_op. The remaining case is $_builder, can we just find if we see that?

Mogball marked 3 inline comments as done.Nov 9 2021, 2:53 PM
Mogball added inline comments.
mlir/include/mlir/TableGen/CodeGenHelpers.h
104

The constraint maps were called localType/AttrConstraints so I stuck the name in here too. I can probably drop that qualifier.

159–160

Oh, yeah probably. I totally missed this.

205

kind is just a string to be printed as part of the name. "attr", "type", "successor", or "region". An enum would only be used inside this class and it would needlessly complicate calling this function.

mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
234–240

Under OpDefinitionsGen.cpp, the function populateSubstitutions replaces variables that reference operands, results, or attributes with calls to get them, e.g. $myAttr becomes (*this)->getAttr(myAttrAttrName()) which is not possible to do generically inside a static function. I.e., op attribute, successor, and region constraints can reference other operands, results, or attributes of the op.

I checked for usages of this for successor and region constraints and found none (because most people use SizedRegion or AnySuccessor). But I can't be sure that not a single attribute constraint uses this feature.

Chia-hungDuan added inline comments.Nov 9 2021, 3:20 PM
mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
234–240

Got it. I would suggest that adding comments at "Constraint Getters". Because only attribute has Optional return value.

Mogball updated this revision to Diff 386013.Nov 9 2021, 4:43 PM
Mogball marked 7 inline comments as done.

Review comments, and changing Constraint constructors to allow usage with
DenseMapInfo

Mogball marked an inline comment as done.Nov 9 2021, 4:44 PM
Chia-hungDuan added inline comments.Nov 9 2021, 5:29 PM
mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
256–257

One space between functions

311–314

Not sure if the coding style say it but I think may the braces at loop level may look better

Mogball updated this revision to Diff 386661.Nov 11 2021, 2:13 PM
Mogball marked 2 inline comments as done.

Review comments

This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2021, 5:04 PM
This revision was automatically updated to reflect the committed changes.