This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] introduce MPInt to support fast arbitrary precision in Presburger
ClosedPublic

Authored by arjunp on Jun 29 2022, 5:38 AM.

Details

Summary

This uses an int64_t-based fastpath for the common case and falls back to
SlowMPInt to handle the rare cases where larger numbers occur.
It uses __builtin_* for performance through the support in LLVM MathExtras.

Using this in the Presburger library results in a minor performance
*improvement* over any commit hash before sequence of patches
starting at d5e31cf38adfc2c240fb9717989792537cc9e819.

Diff Detail

Event Timeline

arjunp created this revision.Jun 29 2022, 5:38 AM
arjunp requested review of this revision.Jun 29 2022, 5:38 AM
arjunp edited the summary of this revision. (Show Details)Jun 29 2022, 5:40 AM
arjunp updated this revision to Diff 440985.Jun 29 2022, 6:25 AM

Fix windows build by removing unnecessary include.

arjunp updated this revision to Diff 441011.Jun 29 2022, 7:41 AM
  • remove unused include
  • add missing always_inlines
  • Fix performance regression due to switching to llvm MathExtras (see top of file)
arjunp updated this revision to Diff 441018.Jun 29 2022, 7:57 AM

Add some more missing always_inlines

Groverkss added inline comments.Jun 30 2022, 5:43 AM
mlir/include/mlir/Analysis/Presburger/MPInt.h
74–75

significantly impact in what ways? better or worse?

385

Did you plan to fix this before landing this patch?

mlir/unittests/Analysis/Presburger/MPIntTest.cpp
2

nit: please make this the right length.

arjunp updated this revision to Diff 441402.Jun 30 2022, 7:56 AM
arjunp marked 2 inline comments as done.
  • use the terms small/large uniformly
  • address kunwar's comments
  • remove getAsAP and just use the cast
  • simplify mod, floorDiv, ceilDiv implementations
  • add assert messages
  • deduplicate division overflow checking
  • add some more comments
arjunp added inline comments.Jul 1 2022, 5:08 AM
mlir/include/mlir/Analysis/Presburger/MPInt.h
385

Yes, fixed, thanks

Groverkss added inline comments.Jul 1 2022, 9:04 AM
mlir/include/mlir/Analysis/Presburger/MPInt.h
111–127

I think these asserts should be more descriptive. For example, "Cannot call getSmall on a large value".

arjunp updated this revision to Diff 441710.Jul 1 2022, 9:26 AM
  • add more tests
  • deduplicate the tests using googletest features
arjunp updated this revision to Diff 441737.Jul 1 2022, 10:31 AM

Use LLVM_ATTRIBUTE_ALWAYS_INLINE instead of attributes directly.

arjunp updated this revision to Diff 441778.Jul 1 2022, 1:02 PM

fix typo

arjunp updated this revision to Diff 441900.Jul 2 2022, 12:48 PM
  • improve assert error
arjunp marked 2 inline comments as done.Jul 2 2022, 1:13 PM
arjunp updated this revision to Diff 441903.Jul 2 2022, 1:18 PM
  • format
Groverkss accepted this revision.Jul 5 2022, 8:58 AM

LGTM.

This revision is now accepted and ready to land.Jul 5 2022, 8:58 AM
arjunp updated this revision to Diff 442522.Jul 6 2022, 5:04 AM

fix comment

arjunp updated this revision to Diff 442523.Jul 6 2022, 5:07 AM

I accidentally pushed an outdated diff. Revert to diff 441903.

arjunp updated this revision to Diff 442524.Jul 6 2022, 5:08 AM
  • really fix comment
ftynse accepted this revision.Jul 11 2022, 2:05 AM
ftynse added inline comments.
mlir/include/mlir/Analysis/Presburger/MPInt.h
28–33

Have you considered proposing LLVM to add the attribute instead?

arjunp added inline comments.Jul 11 2022, 3:32 AM
mlir/include/mlir/Analysis/Presburger/MPInt.h
28–33

Yes, will send a patch afterwards.

This is breaking a buildbot. https://lab.llvm.org/buildbot/#/builders/172/builds/14767 Can you have a look or revert?

Taking a look.

This error is:

C:\PROGRA~1\MICROS~4\2022\COMMUN~1\VC\Tools\MSVC\1431~1.311\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\tools\mlir\lib\Analysis\Presburger -IC:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\lib\Analysis\Presburger -IC:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\include -IC:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\llvm\include -IC:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include -IC:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\tools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Fotools\mlir\lib\Analysis\Presburger\CMakeFiles\obj.MLIRPresburger.dir\MPInt.cpp.obj /Fdtools\mlir\lib\Analysis\Presburger\CMakeFiles\obj.MLIRPresburger.dir\ /FS -c C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\lib\Analysis\Presburger\MPInt.cpp
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\lib\Analysis\Presburger\MPInt.cpp(15): error C2039: 'hash_value': is not a member of 'mlir::presburger'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(24): note: see declaration of 'mlir::presburger'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\lib\Analysis\Presburger\MPInt.cpp(16): error C2248: 'mlir::presburger::MPInt::isSmall': cannot access private member declared in class 'mlir::presburger::MPInt'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(108): note: see declaration of 'mlir::presburger::MPInt::isSmall'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(75): note: see declaration of 'mlir::presburger::MPInt'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\lib\Analysis\Presburger\MPInt.cpp(17): error C2248: 'mlir::presburger::MPInt::valSmall': cannot access private member declared in class 'mlir::presburger::MPInt'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(78): note: see declaration of 'mlir::presburger::MPInt::valSmall'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(75): note: see declaration of 'mlir::presburger::MPInt'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\lib\Analysis\Presburger\MPInt.cpp(18): error C2248: 'mlir::presburger::MPInt::valLarge': cannot access private member declared in class 'mlir::presburger::MPInt'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(79): note: see declaration of 'mlir::presburger::MPInt::valLarge'
C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\mlir\include\mlir/Analysis/Presburger/MPInt.h(75): note: see declaration of 'mlir::presburger::MPInt'

From here:

llvm::hash_code mlir::presburger::hash_value(const MPInt &x) {
  if (x.isSmall())
[...]
}

MPInt::isSmall is indeed private, but hash_value also declared as friend of MPInt. However, friend llvm::hash_code hash_value(const MPInt &x) is declared without namespace, so I am not sure what the compiler is supposed to make it a forward declaration of.

It should be fixed already: https://lab.llvm.org/buildbot/#/builders/172/builds/14775

The friend declaration becomes part of the innermost enclosing namespace:

Names introduced by friend declarations within a non-local class X become members of the innermost enclosing namespace of X, but they do not become visible to ordinary name lookup (neither unqualified nor qualified) unless a matching declaration is provided at namespace scope, either before or after the class definition.

(from https://en.cppreference.com/w/cpp/language/namespace)

Strangely it works under clang++ with no warning whereas g++ errors out: https://godbolt.org/z/59T35h6cW

mehdi_amini added inline comments.Aug 1 2022, 8:55 AM
mlir/include/mlir/Analysis/Presburger/MPInt.h
50

Why these "always inline"?

Can these be made static inline instead?

arjunp added inline comments.Aug 4 2022, 10:05 AM
mlir/include/mlir/Analysis/Presburger/MPInt.h
50

It is important for performance reasons that these actually get inlined. See the comment block at lines 27-32.