Generate static function for matching the type/attribute to reduce the
memory footprint.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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? |
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. |
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? |
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! |
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? |
mlir/tools/mlir-tblgen/RewriterGen.cpp | ||
---|---|---|
1645 | Oh, it's because Constraint doesn't provide a default constructor. |
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!
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
I wouldn't make these functions inline. Also document these.