Page MenuHomePhabricator

[mlir][ods] AttrOrTypeGen uses Class
ClosedPublic

Authored by Mogball on Nov 11 2021, 2:19 PM.

Details

Summary

AttrOrType def generator uses Class code gen helper,
instead of naked raw_ostream.

Depends on D113714 and D114807

Diff Detail

Event Timeline

Mogball created this revision.Nov 11 2021, 2:19 PM
Mogball requested review of this revision.Nov 11 2021, 2:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 11 2021, 2:19 PM
rriddle added inline comments.Nov 15 2021, 12:12 AM
mlir/include/mlir/TableGen/Class.h
245–252

I'm getting a slight code smell here that we are slowly building towards a full C++ specification API.

462

Prefer accepting const Twine & instead of std::string. It accepts many more parameter types (e.g. StringRef, std::string, llvm::formatv, etc.), and can easily be converted to string.

537–538

Not sure this comment is true given that we have isPublic below.

546

Missing comment here?

551

nit: Drop the const, we don't generally add them.

Also please document these fields.

694

This needs some documentation.

mlir/include/mlir/TableGen/Format.h
54

nit: I'd use ArrayRef here instead.

mlir/lib/TableGen/AttrOrTypeDef.cpp
115–116

Can we just put the vector on the class and return ArrayRef instead?

mlir/lib/TableGen/Class.cpp
362–371

This needs some documentation.

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
161
Mogball marked 10 inline comments as done.Nov 15 2021, 1:38 PM
Mogball added inline comments.
mlir/include/mlir/TableGen/Class.h
245–252

I agree. I'm not necessarily happy about it, but it absolutely is more maintainable than writing to a naked raw_ostream.

462

Good point, but what's your opinion on the stringify function I added? I wanted to elide copying std::string when it's passed in directly, and I've had some issues with const Twine &; for example,

void one(const Twine &value = "");
void one(bool value = false);

Confusingly, one("C string") calls one(bool)

Mogball updated this revision to Diff 387388.Nov 15 2021, 1:40 PM
Mogball marked 2 inline comments as done.

Review comments, and also adding more documentation.

rriddle added inline comments.Nov 17 2021, 12:37 AM
mlir/include/mlir/TableGen/Class.h
430
462

Which methods do we have that takes bool or Twine? If it isn't a lot of methods, we can always just add an overload specialized for const char */StringRef.

but what's your opinion on the stringify function I added?

It's giving me an off feeling, but that may be that we generally try not to use std::string directly.

mlir/lib/TableGen/Class.cpp
203–205

AFAIR we rely on the compiler to warn/error about missing cases.

301–302

Please switch this to an if/else to make it easier to read.

313

Please use proper foreach loops for these, the llvm::for_each doesn't add much.

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
211–216

I'd just use a proper foreach loop here, using llvm::for_each doesn't really add much.

431

semi unrelated, but the name "reindent" is throwing me off. It doesn't read like something that is emitted to the stream, I keep expecting it to return a reindented string. Can we rename that to something better?

462–463

?

511–515

raw_string_ostream would likely be cleaner here.

627

Drop the consts, we don't really add them unless really necessary (i.e. if they add value).

mlir/tools/mlir-tblgen/OpClass.h
26

Can you document what this does? The doc on the other finalize was quite nice.

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

Same comment about () here and elsewhere.

852

Can we drop the ()?

875–876

Can we drop the ()? We generally don't wrap unless necessary.

1950–1953

Is this right, AFAICT Method::Declaration & declaration is always false.

rriddle added inline comments.Nov 17 2021, 12:39 AM
mlir/include/mlir/TableGen/Class.h
430

Why is this defined outside of the namespace?

Mogball added inline comments.Nov 17 2021, 8:31 AM
mlir/include/mlir/TableGen/Class.h
462

I went for the stringify route because occasionally in ODS we create std::string and maybe something takes ownership of it. I got tired of debugging segmentation faults because .str() wasn't being called somewhere and a StringRef or Twine or format object was outliving its value.

Mogball updated this revision to Diff 388112.Nov 17 2021, 10:36 PM
Mogball marked 14 inline comments as done.

Review comments.

mlir/include/mlir/TableGen/Class.h
430

I didn't know about this! But also, it doesn't seem to work because the functions need to be constexpr (and there are asserts in them...)

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
431

I agree. I initially thought the same.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1950–1953

This actually expands to declaration ? Method::Declaration : Method::None. If that's not intuitive, I can remove it.

rriddle added inline comments.Nov 17 2021, 11:23 PM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1950–1953

Are you implying Method::Declaration & declaration is the same as declaration ? Method::Declaration : Method::None?

https://godbolt.org/z/rnjhbe77G

rriddle added inline comments.Nov 18 2021, 1:21 AM
mlir/include/mlir/TableGen/Class.h
443–446

Please remove this, the code that this creates is not readable.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1950–1953

I see now, you've defined a custom operator& for that. Please remove that, it creates a hacky difference in behavior that is not what someone what naturally expect.

rriddle added inline comments.Nov 18 2021, 1:23 AM
mlir/include/mlir/TableGen/Class.h
443–446

In general, please avoid overloading operators like this. It creates code that is difficult to read/maintain just for the sake of saving a line or two. Prefer writing clean/readable code over "tricky" code.

Mogball marked 6 inline comments as done.Nov 18 2021, 1:41 PM
Mogball added inline comments.
mlir/include/mlir/TableGen/Class.h
443–446

Very well.

FYI I originally contemplated:

props |= Method::Static & ~(isStatic - 1u)

And so I thought that this would be "more readable" than that...

Mogball updated this revision to Diff 388317.Nov 18 2021, 1:56 PM
Mogball marked an inline comment as done.

Review comments and fixing build (hopefully)

Mogball updated this revision to Diff 388344.Nov 18 2021, 3:57 PM

Fixing build .-.

jpienaar added inline comments.Nov 23 2021, 4:04 PM
llvm/include/llvm/ADT/STLExtras.h
1017 ↗(On Diff #388344)

Sorry haven't gotten to this review yet. Could we perhaps extract out the changes to ADT into parent change for separate review?

Mogball added inline comments.Nov 30 2021, 5:03 AM
llvm/include/llvm/ADT/STLExtras.h
1017 ↗(On Diff #388344)

Sure thing

Mogball updated this revision to Diff 390747.Nov 30 2021, 9:41 AM

Splitting out changes to STLExtras.h

Mogball edited the summary of this revision. (Show Details)Nov 30 2021, 9:41 AM
Mogball marked an inline comment as done.
rriddle accepted this revision.Nov 30 2021, 4:06 PM

LGTM, I think we can just land this and iterate from there.

This revision is now accepted and ready to land.Nov 30 2021, 4:06 PM
This revision was automatically updated to reflect the committed changes.