This is an archive of the discontinued LLVM Phabricator instance.

[ODS] Make the getType() method on a OneResult instruction return a specific type.
ClosedPublic

Authored by lattner on Dec 23 2020, 6:19 PM.

Details

Summary

Implement Bug 46698, making ODS synthesize a getType() method that returns a
specific C++ class for OneResult methods where we know that class. This eliminates
a common source of casts in things like:

myOp.getType().cast<FIRRTLType>().getPassive()

because we know that myOp always returns a FIRRTLType. This also encourages
op authors to type their results more tightly (which is also good for
verification).

I chose to implement this by splitting the OneResult trait into itself plus a
OneTypedResult trait, given that many things are using hasTrait<OneResult>
to conditionalize various logic.

While this changes makes many many ops get more specific getType() results, it
is generally drop-in compatible with the previous behavior because 'x.cast<T>()'
is allowed when x is already known to be a T. The one exception to this is that
we need declarations of the types used by ops, which is why a couple headers
needed additional #includes.

I updated a few things in tree to remove the now-redundant .cast<>'s, but there
are probably many more than can be removed.

Diff Detail

Event Timeline

lattner created this revision.Dec 23 2020, 6:19 PM
lattner requested review of this revision.Dec 23 2020, 6:19 PM
jpienaar accepted this revision.Dec 23 2020, 6:42 PM
jpienaar added a subscriber: jpienaar.

Nice, seems a good reduction

mlir/include/mlir/Dialect/ArmNeon/ArmNeonDialect.h
16

Why is this needed?

mlir/include/mlir/IR/OpBase.td
179

I was actually thinking of something else:

CastableType (better name welcome as always :))

This is one where the type constraint is a isa and you have the specific type. That probably does not subsume this - as one could have a more specific type but the isa would be a one of variant (which is possible due to variadic isa now) or least lax. But seems like a useful class where the predicate and the cppClassName are both known.

mlir/include/mlir/IR/OpDefinition.h
626

As you are here :) One space after period only (same below).

mlir/lib/Dialect/Vector/VectorOps.cpp
852

Nit: clang-format only changed parts to make these easier (you don't have to change, just for next one)

This revision is now accepted and ready to land.Dec 23 2020, 6:42 PM

Landed in 9eb3e564d3b, thanks!