Page MenuHomePhabricator

[OpenMP][OMPIRBuilder] Adding support for `omp atomic`
Needs ReviewPublic

Authored by fghanim on Mar 20 2021, 12:06 PM.

Details

Summary

This patch adds support for generating omp atomic for all different atomic clauses

Diff Detail

Event Timeline

fghanim created this revision.Mar 20 2021, 12:06 PM
fghanim requested review of this revision.Mar 20 2021, 12:06 PM
Herald added a project: Restricted Project. · View Herald Transcript

Could you please address the numerous clang-format/clang-tidy warnings?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2235–2236
2243–2244

Please remove all code in comments; The TODO should explain what there is todo.

2246

This if-chain could be done more elegantly with a switch.

2283

[nit] Unnecessary newline

2287
2302

[style] Please start sentences with a capital letter.

2392–2398

With a default case, no need to enumerate all cases.

fghanim added inline comments.Mar 23 2021, 3:11 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2246

Either:
1- do nested if-else
2- nested switch case
3- a mix of both.

4- use the magic of logical shifts & bit-wise and/or.s to do this, and get rid of all the messy cfg

switched from 1 to 3, more than happy to do 4

2392–2398

I intentionally listed all cases. If you want, i'll remove default which I added to future proof the code.

fghanim updated this revision to Diff 332801.Mar 23 2021, 3:12 PM
  • fixed clang-tidy errors
  • addressed reviewer comments
fghanim marked 5 inline comments as done.Mar 23 2021, 8:41 PM
fghanim updated this revision to Diff 332857.Mar 23 2021, 8:49 PM
  • more clang-tidy fixes

You can avoid the "clang-format not found in user's PATH; not linting file." warning using arc diff --nolint. It asks for a justification, it accepts every non-empty string, including a single space. Please consider fixing the other warnings.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2232

Consider making such helpers static functions instead of priviate methods, so they don't need to appear in the public header file.

2246

It is fine as-is, just the Capture case might justify a nested switch.

Not sure whether approach 4 would make it any better.

2392–2398

With a default case, the compiler does not warn about unhandled cases anymore. If you want both, you could do:

[...]
case AtomicRMWInst::Min:
  llvm_unreachable("Unsupported atomic update operation");
}
llvm_unreacheable("Unhandled atomic update operation");

Personally I think it just adds clutter lines. In case of some parts of clang's OpenMP implementation it fills entire screens of unsupported cases and there is no chance I miss handling the case during testing due to the llvm_unreachable.

Currently, return nullptr; is unreachable code.

2434–2438

Triple slashes are doxygen comments, which come before the function. If not a doxygen documentation, please use standard // comments.

2439

[style] No almost-always-auto in LLVM style

2471–2474

Debugging leftover?

SouraVX added a project: Restricted Project.Apr 2 2021, 9:49 AM

Thanks! @fghanim for the patch :)

I noticed following piece of code getting used in createAtomicRead, createAtomicWrite, createAtomicCapture, createAtomicUpdate.

if (!updateToLocation(Loc))
    return Loc.IP;
Type *XTy = X.Var->getType();
  assert(XTy->isPointerTy() && "OMP Atomic expects a pointer to target memory");
  Type *XElemTy = XTy->getPointerElementType();
  assert((XElemTy->isFloatingPointTy() || XElemTy->isIntegerTy() ||
          XElemTy->isPointerTy()) &&
         "OMP atomic read expected a scalar type");

Was wondering if this could be simplified or hoisted out in a tiny innocuous helper function ?

Also right now we have these 4 public interfaces with external front-end's. Can we simplify this into 2 public interfaces as createAtomicReadOrWrite and createAtomicCaptureOrUpdate ?
The rationale behind this proposition is that, these functions share a lot common code(at expense of 1 or 2 extra argument needed to handle the other case).
Can we do this ? Or doing such simplification will render the code more unreadable/complicated ?
Please Let me know WDYT ?
+ @AMDChirag

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
912

Can we use a better/descriptive name here ? Maybe TargetAtomicPtr ?
Other references should also benefit from descriptive name.

1015

This could also benefit from descriptive naming ?

fghanim marked 5 inline comments as done.Apr 5 2021, 2:44 PM

Thanks! @fghanim for the patch :)

Sure! :)

I noticed following piece of code getting used in createAtomicRead, createAtomicWrite, createAtomicCapture, createAtomicUpdate.

if (!updateToLocation(Loc))
    return Loc.IP;
Type *XTy = X.Var->getType();
  assert(XTy->isPointerTy() && "OMP Atomic expects a pointer to target memory");
  Type *XElemTy = XTy->getPointerElementType();
  assert((XElemTy->isFloatingPointTy() || XElemTy->isIntegerTy() ||
          XElemTy->isPointerTy()) &&
         "OMP atomic read expected a scalar type");

Was wondering if this could be simplified or hoisted out in a tiny innocuous helper function ?

The return returns from createOMPAtomicXXX calls, and each of XTy and XElemTy are used inside the function. Also, We are checking different kinds of errors between different atomic clauses, and issuing different error assert messages based on the error, .

Also right now we have these 4 public interfaces with external front-end's. Can we simplify this into 2 public interfaces as createAtomicReadOrWrite and createAtomicCaptureOrUpdate ?
The rationale behind this proposition is that, these functions share a lot common code(at expense of 1 or 2 extra argument needed to handle the other case).
Can we do this ? Or doing such simplification will render the code more unreadable/complicated ?
Please Let me know WDYT ?

I don't think we are simplifying things by doing that, because it will cause unnecessary confusion. two parts:

  • Regarding atomic read and atomic write, there is nothing in common between the two (other than the initial checks). While at first glance, they may look similar, they are not.
  • Regarding atomic update and atomic capture, I already implemented the parts that are common between the two separately into emitAtomicUpdate

In general, it is better for someone handling an atomic read, to just call createOMPAtomicRead, than call something that is also doing writes. more intuitive.

+ @AMDChirag

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
912

These names are picked to match the naming used in the OMP standard documents for atomic:
https://www.openmp.org/spec-html/5.0/openmpsu95.html

1015

These names are picked to match the naming used in the OMP standard documents for atomic:
https://www.openmp.org/spec-html/5.0/openmpsu95.html

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2232

I call emitFlush() which is private

2392–2398

The issue here is that, these cases are either not supported in omp (min,max,..), or should be handled by the cmpExch (fadd, fsub,exch). I worry if i don't explicitly list them as unreachable somebody might try to implement them.

2471–2474

Yes. Thanks for catching that. removed

fghanim updated this revision to Diff 335333.Apr 5 2021, 2:45 PM
fghanim marked an inline comment as done.
  • addressing more reviewer comments
  • Fixing nits + clang-format errors
fghanim updated this revision to Diff 336705.Sun, Apr 11, 3:03 PM

fixing some clang-tidy issues

Thanks for the patch! FYI, I'm working on the FE part of compare and compare capture.

Sorry for the delay.

Consider starting all sentences in comments with capital letters.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
885–887

Could you insert empty lines, at least separate description from the \param list.

Consider surrounding code with \code \endcode (https://www.doxygen.nl/manual/commands.html#cmdcode)

: expr is any legal operation is not a comment in C/C++. Did you mean to capitalize expr?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2282

Capitalize "TODO" to make them stand out (some IDEs recognize these)

2315

There is no test coverage for this non-integer case.

2321

Use a common convention for naming IR registers.

2415

Please completely remove commented code.

2462

"X.Temp" could be more descriptive.

Also, they should conform to the lowercase convention

2476

Use lower case for register names

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2187

Please add

EXPECT_FALSE(verifyModule(*M, &errs()));

to each tests case to verify its wellformedness.

2191–2192
fghanim updated this revision to Diff 343505.Thu, May 6, 2:31 PM
fghanim marked 8 inline comments as done.
  • Rebasing
  • Adding test cases
  • Addressing reviewers comments

Thanks for the patch! FYI, I'm working on the FE part of compare and compare capture.

Sure! :)
Let me know if I can help.

Sorry for the delay.

No worries. Thanks for the comments.

fghanim updated this revision to Diff 343610.Fri, May 7, 1:30 AM
  • clang-tidy + clang-format fixes