This is an archive of the discontinued LLVM Phabricator instance.

[mlir][matchers] Add m_Op(StringRef) and m_Attr matchers
ClosedPublic

Authored by devajithvs on Mar 30 2023, 1:12 PM.

Details

Summary

This patch introduces support for m_Op with a StringRef argument and m_Attr matchers. These matchers will be very useful for mlir-query that is being developed currently.

Submitting this patch separately to reduce the final patch size and make it easier to upstream mlir-query.

Diff Detail

Event Timeline

devajithvs created this revision.Mar 30 2023, 1:12 PM
devajithvs requested review of this revision.Mar 30 2023, 1:12 PM
rriddle added inline comments.Mar 30 2023, 1:16 PM
mlir/include/mlir/IR/Matchers.h
292–294

I'd likely name this m_Attr, with an optional binding capture of the attribute itself.

296–299

This should likely be named m_Op, given it does the same thing (in a non-templated way).

This comment was removed by devajithvs.
  • Renamed m_AttrName -> m_Attr, with an optional binding capture of the attribute.
  • Renamed m_Name -> m_Op
devajithvs retitled this revision from [mlir][matchers] Add new m_Name and m_AttrName matchers with test case to [mlir][matchers] Add m_Op(StringRef) and m_Attr matchers.Mar 30 2023, 4:12 PM
rriddle accepted this revision.Mar 30 2023, 4:15 PM

Looks good to me, nice!

mlir/include/mlir/IR/Matchers.h
56–61

Slight preference here (I know the style in this file is all over the place).

106–107

Can you shift these to the bottom of the class? Also, please use camelCase for variable names here and throughout the patch (I know this file is all over the place, some of this code is *old*)

118–119

Can you use getAttrOfType<AttrT> here instead? It does the same thing, and that would remove the double lookup.

mlir/test/lib/IR/TestMatchers.cpp
155–157
This revision is now accepted and ready to land.Mar 30 2023, 4:15 PM

Thanks @rriddle, done!
Am I correct in assuming that struct names remain in the same format (snake_case)?

devajithvs edited the summary of this revision. (Show Details)Mar 30 2023, 4:55 PM

Thanks @rriddle, done!
Am I correct in assuming that struct names remain in the same format (snake_case)?

I couldn't remember if they were all snake_case, but just checked and it's also inconsistent in this file, can you use the proper naming convention for those (i.e. CamelCase)?

devajithvs edited the summary of this revision. (Show Details)Mar 30 2023, 5:01 PM

Updated struct names to CamelCase

Fix clang-format issues

Not sure why build on windows fails.

If you click on the link and search for error: you can easily find it: ******************** TEST 'Flang :: Driver/omp-frontend-forwarding.f90' FAILED ********************. Seems like unrelated to your patch.

If you click on the link and search for error: you can easily find it: ******************** TEST 'Flang :: Driver/omp-frontend-forwarding.f90' FAILED ********************. Seems like unrelated to your patch.

Thanks. Yeah, it seems unrelated, I tried rebuilding and it still failed. Maybe, I'll try again.

devajithvs updated this revision to Diff 511204.Apr 5 2023, 1:53 PM

Modify an inline comment in the patch to trigger proper rebuild

devajithvs updated this revision to Diff 511678.Apr 7 2023, 6:21 AM
devajithvs requested review of this revision.Apr 11 2023, 7:47 AM
devajithvs marked an inline comment as done.

Can someone help me land this patch if it's ok?

devajithvs marked 5 inline comments as done.Apr 11 2023, 7:48 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 11 2023, 2:16 PM
This revision was automatically updated to reflect the committed changes.