This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] unittests: use an MLIRContext declared in parsePoly
ClosedPublic

Authored by arjunp on Feb 8 2022, 7:58 AM.

Details

Summary

Use an MLIRContext declared in a single place in the parsePoly function that almost all Presburger unit tests use for parsing sets. This function is only used in tests.

This saves us from having to declare and pass a new MLIRContext in every test.

Diff Detail

Event Timeline

arjunp created this revision.Feb 8 2022, 7:58 AM
arjunp requested review of this revision.Feb 8 2022, 7:58 AM

This LGTM, but I would wait for @bondhugula to review in case he has any concerns with unit tests having a static MLIRContext.

bondhugula requested changes to this revision.Feb 18 2022, 5:04 PM

Please don't leave the commit summary empty in such cases -- please always add a line as to the rationale.

This revision now requires changes to proceed.Feb 18 2022, 5:04 PM
bondhugula added inline comments.
mlir/unittests/Analysis/Presburger/Utils.h
28

Switch to static MLIRContext *looks* fine to me. I don't have specific insights. @mehdi_amini, any comments?

arjunp edited the summary of this revision. (Show Details)Feb 19 2022, 1:24 AM

Actually, I realised there is no reason for this to be static. I don't think we face any issue if different IntegerPolyhedrons even in the same test are constructed from different MLIRContexts. Is the problem only with making this static? (Also, what is the problem with that?)

The fixture could work, but I see some downside with each possible approach:

a) defining one single test fixture with parsePoly as member function and an MLIRContext member in the Utils.h -- this has the disadvantage that all test groups like IntegerPolyhedronTest, PresburgerSetTest, will have to be changed to the same name.

b) defining one single fixture as above and public inheriting from it with different names. I think this might work but it's a bit weird.

c) copy pasting the same parsePoly/MLIRContext fixture in each test file -- too much code duplication.

arjunp updated this revision to Diff 410250.Feb 21 2022, 2:04 AM

Made the MLIRContext non-static.

Actually, I realised there is no reason for this to be static. I don't think we face any issue if different IntegerPolyhedrons even in the same test are constructed from different MLIRContexts. Is the problem only with making this static? (Also, what is the problem with that?)

MLIRContext is a pretty heavy class -- won't its default ctor not entail a lot of overhead? See how many DenseMap's, unique ptrs, and thread pool related stuff MLIRContextImpl has.

On my machine (-O0 -g -ftrapv).

Runtime before patch (MLIRContext for each test): ~0.345s
Runtime with non-static context (MLIRContext for each poly): ~0.385s
Runtime with a single static context: ~0.315s

It seems to be a matter of +-10% performance.

mehdi_amini accepted this revision.Feb 21 2022, 11:43 AM
mehdi_amini added inline comments.
mlir/unittests/Analysis/Presburger/Utils.h
28

Just a nit:

MLIRContext context(mlir::MLIRContext::Threading::DISABLED);
arjunp updated this revision to Diff 410353.Feb 21 2022, 11:46 AM

Address Mehdi's comment

arjunp retitled this revision from [MLIR][Presburger] unittests: use a single static MLIRContext in parsePoly to [MLIR][Presburger] unittests: use an MLIRContext declared in parsePoly.Feb 21 2022, 11:48 AM
arjunp edited the summary of this revision. (Show Details)
bondhugula accepted this revision.Feb 23 2022, 6:07 AM
This revision is now accepted and ready to land.Feb 23 2022, 6:07 AM
arjunp updated this revision to Diff 410813.Feb 23 2022, 7:01 AM

Rebase and remove remaining MLIRContexts

This revision was landed with ongoing or failed builds.Feb 23 2022, 7:07 AM
This revision was automatically updated to reflect the committed changes.