Page MenuHomePhabricator

Static verifier for type/attribute in DRR
ClosedPublic

Authored by Chia-hungDuan on Sep 21 2021, 2:02 PM.

Details

Summary

Generate static function for matching the type/attribute to reduce the
memory footprint.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Sep 21 2021, 2:02 PM
Chia-hungDuan requested review of this revision.Sep 21 2021, 2:02 PM
Chia-hungDuan added inline comments.Sep 21 2021, 2:11 PM
mlir/tools/mlir-tblgen/RewriterGen.cpp
743

I'm considering if we should have static function for attribute in DRR. The point is in the DRR code, we generate

"auto tblgen_attr = {0}->getAttrOfType<{1}>(\"{2}\");"

tblgen_attr will get the derived type rather than the base class mlir::Attribute and many attribute constraint function are having argument with derived type. For example,

in tensorflow/compiler/mlir/hlo/lib/utils/hlo_utils.{td|cc}

// Constraint that Attr has values [0, 1, ...].
def IdentityBroadcastDims : AttrConstraint<
    CPred<"hlo::IsSequenceStartingWith0($_self)">>;

bool IsSequenceStartingWith0(DenseIntElementsAttr attr)

Of course we can do the cast in the utility function, just not sure which way is better. In perspective of supporting Commutative, focusing on type may be enough at beginning.

BTW, in ODS, type is always mlir::Attribute.

jpienaar added inline comments.Oct 1 2021, 7:41 AM
mlir/tools/mlir-tblgen/RewriterGen.cpp
743

In the first part you say both get derived attribute type, that sounds good then.

For types, it is a TypeAttr storage wise I'm assuming, but else I'm missing how type and derived attribute interplay here. And also how that affects commutative.

Are you saying ODS and DRR casts differently and so reusing between them is difficult at the moment?

Chia-hungDuan added inline comments.Oct 1 2021, 10:43 AM
mlir/tools/mlir-tblgen/RewriterGen.cpp
743

It's not difficult and I'm just thinking if we can use Attribute it'll be easier and it'll align the predicate format with the cases in ODS.

PS. As mentioned here, https://llvm.discourse.group/t/self-will-be-the-base-type-of-the-entity-that-attaches-to-a-predicate/4367/3. we can use template to solve this issue.

Mogball requested changes to this revision.Nov 8 2021, 10:10 AM
Mogball added a subscriber: Mogball.

Looks fine overall.

mlir/include/mlir/TableGen/CodeGenHelpers.h
137

I wouldn't make these functions inline. Also document these.

147–148

Document this

mlir/include/mlir/TableGen/Pattern.h
117

This could probably be public.

mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
51

OpDef constraints are collected inside here, but pattern ones are collected outside this class and then passing in through emitConstraintMethods which seems inconsistent

82–83

Don't mark ArrayRef as const, and drop llvm::

84

name the raw_ostream as something else (maybe raw_os) and then name this one os

mlir/tools/mlir-tblgen/RewriterGen.cpp
679

Isn't operandIndex passed into the function?

1645

Since you've added DenseMapInfo for the other classes, why not add one for Constraint as well so that you don't have to use opaque pointers?

This revision now requires changes to proceed.Nov 8 2021, 10:10 AM
Chia-hungDuan marked 6 inline comments as done.Nov 8 2021, 12:46 PM
Chia-hungDuan added inline comments.
mlir/include/mlir/TableGen/Pattern.h
117

This is private in DagNode, do you think we need to change both of them to public?

mlir/tools/mlir-tblgen/CodeGenHelpers.cpp
51

That's true. I moved out this collection process. I think maybe we can have further refactoring on this architecture.

mlir/tools/mlir-tblgen/RewriterGen.cpp
679

Nice catch!

Chia-hungDuan marked 2 inline comments as done.

Address review comments

Chia-hungDuan added inline comments.Nov 8 2021, 12:58 PM
mlir/tools/mlir-tblgen/RewriterGen.cpp
1645

Sorry, missed this comment.

I think I was thinking this before but I forgot the exact reason now... Maybe it's just trying to align the style in StaticMatcherHelper. I think this can be refactored along with your change, what do you think?

fixed minor format

Mogball accepted this revision.Nov 8 2021, 1:27 PM
Mogball added inline comments.
mlir/tools/mlir-tblgen/RewriterGen.cpp
1645

Oh, it's because Constraint doesn't provide a default constructor.

This revision is now accepted and ready to land.Nov 8 2021, 1:27 PM
This revision was landed with ongoing or failed builds.Nov 8 2021, 1:40 PM
This revision was automatically updated to reflect the committed changes.
quinnp added a subscriber: quinnp.Feb 23 2022, 2:04 PM

Hi @Chia-hungDuan, while doing PowerPC Linux release testing for LLVM 14.0.0-rc1 I encountered some differing object files between Phase2 and Phase3 of the bootstrap build:

# Comparing Phase 2 and Phase 3 files
file ArithmeticOps.cpp.o differs between phase 2 and phase 3
file TestPatterns.cpp.o differs between phase 2 and phase 3

I used git bisect to find that this patch was the first instance of the differing files. We are hoping to fix this for the LLVM 14.0.0 release. I've created this issue in the llvm project. It would be great if you could take a look at it. Thanks!

Hi @Chia-hungDuan, while doing PowerPC Linux release testing for LLVM 14.0.0-rc1 I encountered some differing object files between Phase2 and Phase3 of the bootstrap build:

# Comparing Phase 2 and Phase 3 files
file ArithmeticOps.cpp.o differs between phase 2 and phase 3
file TestPatterns.cpp.o differs between phase 2 and phase 3

I used git bisect to find that this patch was the first instance of the differing files. We are hoping to fix this for the LLVM 14.0.0 release. I've created this issue in the llvm project. It would be great if you could take a look at it. Thanks!

Hi Quinn, thanks for letting me know. I'm checking it

I commented on the issue on GitHub : I don’t quite get how a change in MLIR can have any impact here: I suspect it is just triggering something else or it is non deterministic

Thanks, just saw it. I don't quite understand either. Let's move the discussion there