Page MenuHomePhabricator

[OpenMP] Introduce the OpenMP-IR-Builder
ClosedPublic

Authored by jdoerfert on Nov 3 2019, 10:57 PM.

Details

Summary

This is the initial patch for the OpenMP-IR-Builder, as discussed on the
mailing list ([1] and later) and at the US Dev Meeting'19.

The design is similar to D61953 but:

  • placed in llvm/lib/IR/ next to IRBuilder, for lack of a better location.
  • in a non-WIP status, with proper documentation and working.
  • using a OpenMPKinds.def file to manage lists of directives, runtime functions, types, ..., similar to the current Clang implementation.
  • restricted to handle only (simple) barriers, to implement most #pragma omp barrier directives and most implicit barriers.
  • properly hooked into Clang to be used if possible.
  • compatible with the remaining code generation.

The plan is to have multiple people working on moving logic from Clang
here once the initial scaffolding (=this patch) landed.

[1] http://lists.flang-compiler.org/pipermail/flang-dev_lists.flang-compiler.org/2019-May/000197.html

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ABataev added inline comments.Nov 11 2019, 1:14 PM
llvm/include/llvm/IR/OpenMPConstants.h
57 ↗(On Diff #228754)

Why do you use 0x7FFFFFFF as the largest value?

jdoerfert marked 2 inline comments as done.Nov 11 2019, 1:26 PM
jdoerfert added inline comments.
llvm/include/llvm/IR/OpenMPConstants.h
57 ↗(On Diff #228754)

Because it is a valid upper bound. We don't have all values in OMP_IDENT_FLAG but if you want to come up with a tighter bound, please feel free.

llvm/lib/IR/OpenMPIRBuilder.cpp
49–51 ↗(On Diff #228754)

If we want attributes to be optional, then yes. If we say they are mandatory, then not.

Meinersbur added inline comments.Nov 11 2019, 5:11 PM
llvm/include/llvm/IR/OpenMPKinds.def
186 ↗(On Diff #227902)

Do you know of an example of a non-llvm compiler using this libomp?

gcc (using libomp's gomp compatibility layer), icc (as libomp was initially donated by Intel).

I don't understand why it even matters if there are other compilers using libomp. Every LLVM runtime library can be built stand-alone.
With constant values being determined during compiler bootstrapping, programs built on one computer would be potentially ABI-incompatible with a runtime library on another. Think about updating your compiler-rt/libomp/libc++ on you computer causing all existing binaries on the system to crash because constants changed in the updated compiler's bootstrapping process.

The only use case I know that does this is are operating system's syscall tables. Linux's reference is unistd.h which is platform-specific and Windows generates the table during its build process. Therefore on Windows, system calls can only be done through ntdll. Even on Linux one should use the system's libc instead of directly invoking a system call.

jdoerfert updated this revision to Diff 228792.Nov 11 2019, 6:25 PM

Add cancel_barrier functionality + test, move everything to "Frontend"

Uncertainty over the handling of constant data between clang and libopenmp not withstanding, I think this is good to go.

llvm/include/llvm/IR/OpenMPKinds.def
186 ↗(On Diff #227902)

Thanks. GCC and ICC would presumably be happier with the magic numbers stored with openmp then (though with the move to a monorepo that's a little less persuasive).

When constants that affect the ABI change, the result won't work with existing software regardless of whether the compiler or the library contains the change. Either the new compiler builds things that don't work with the old library, or the new library doesn't work with things built by the old compiler. The two have to agree on the ABI.

At present, openmp does the moral equivalent of #include OpenMPKinds.def from clang. Moving the constants to libomp means clang will do the equivalent of #include OpenMPKinds.def from openmp. Breaking that dependency means making a new subproject that just holds/generates the constants, that both depend on, which seems more hassle than it's worth.

I'd like to generate this header as part of the clang build (though ultimately don't care that much if it's generated as part of the openmp build) because it's going to become increasingly challenging to read as non-nvptx architectures are introduced. Likewise it would be useful to generate the interface.h for deviceRTL (or equivalently a set of unit tests checking the function types) from the same source to ensure it matches and that's not economically feasible within the C preprocessor.

ABataev added inline comments.Nov 12 2019, 8:33 AM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
179 ↗(On Diff #228792)

auto *

228 ↗(On Diff #228792)

Maybe add an assert when the cancellation version is requested but the cancellation block is not set? Instead of the generating simple version of barrier.

jdoerfert marked 3 inline comments as done.Nov 12 2019, 1:34 PM
jdoerfert added inline comments.
llvm/include/llvm/IR/OpenMPKinds.def
186 ↗(On Diff #227902)

I am unsure how this conversation evolved and what you want me to do now.

I repeat what I said before:

This does neither change the constants, our usage of them, nor the fact that we have them defined in multiple places, just one of the places (now llvm, before clang) changed.

llvm/lib/Frontend/OpenMPIRBuilder.cpp
179 ↗(On Diff #228792)

Sure.

228 ↗(On Diff #228792)

The interface doesn't work that way as we do not know here if the cancellation was requested except if the block was set. That is basically the flag (and I expect it to continue to be that way).

ABataev added inline comments.Nov 12 2019, 1:40 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and if it set to true, always emit cancel barrier, otherwise always emit simple barrier? And add an assertion for non-set cancellation block or even accept it as a parameter here.

Also, what if we have inner exception handling in the region? Will you handle the cleanup correctly in case of the cancelation barrier?

jdoerfert marked an inline comment as done.Nov 12 2019, 1:57 PM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

Maybe instead of ForceSimpleBarrier add a flag EmitCancelBarrier and if it set to true, always emit cancel barrier, otherwise always emit simple barrier? And add an assertion for non-set cancellation block or even accept it as a parameter here.

What is the difference in moving some of the boolean logic to the caller? Also, we have test to verify we get cancellation barriers if we need them, both unit tests and clang lit tests.

Also, what if we have inner exception handling in the region? Will you handle the cleanup correctly in case of the cancelation barrier?

I think so. Right now through the code in clang that does the set up of the cancellation block, later through callbacks but we only need that for regions where we actually go out of some scope, e.g., parallel.

ABataev added inline comments.Nov 12 2019, 2:09 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)
  1. I'm just thinking about future users of thus interface. It woild be good if we could provide safe interface for all the users, not only clang.
  2. Exit out of the OpenMP region is not allowed. But you may have inner try...catch or just simple compound statement with local vars that require constructors/destructors. And the cancellation barrier may exit out of these regions. And you need to call all required destructors. You'd better to think about it now, not later.
jdoerfert marked an inline comment as done.Nov 12 2019, 2:30 PM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)
  1. [...] You'd better to think about it now, not later.

First, I do think about it now and I hope this was not an insinuation to suggest otherwise.

  1. I'm just thinking about future users of thus interface. It woild be good if we could provide safe interface for all the users, not only clang.
  2. Exit out of the OpenMP region is not allowed. But you may have inner try...catch or just simple compound statement with local vars that require constructors/destructors. And the cancellation barrier may exit out of these regions. And you need to call all required destructors.

Generally speaking, we shall not add features that we cannot use or test with the assumption we will use them in the future. This is suggested by the LLVM best practices. If you have specific changes in mind that are testable and better than what I suggested so far, please bring them forward. You can also bring forward suggestions on how it might look in the future but without a real use case now it is not practical to block a review based on that, given that we can change the interface once the time has come.

I said before, we will need callbacks for destructors, actual handling of cancellation blocks, and there are various other features missing right now. Nevertheless, we cannot build them into the current interface, or even try to prepare for all of them, while keeping the patches small and concise.

ABataev added inline comments.Nov 12 2019, 2:57 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

It won't work for clang, I'm afraid. You need a list of desructors here. But clang uses recursive codegen and it is very hard to walk over the call tree and gather all required destructors into a list. At least, it will require significant rework in clang frontend.
Instead of generating the branch to cancellation block in the builder, I would suggest to call a single callback function provided by the frontend, which will generate correct branch over a chain of the destructor blocks. In this case, you won't need this cancellation block at all. This is what I meant when said that you need to think about this problem right now. That current solution is not very suitable for the use in the frontend.

jdoerfert marked an inline comment as done.Nov 12 2019, 3:07 PM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

It won't work for clang,

It won't work in the future or it does not work now? If the latter, do you have a mwe to show the problem?

ABataev added inline comments.Nov 12 2019, 3:44 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)
  1. Both.
  2. What is mwe? Sure, will simple test tomorrow.
jdoerfert marked an inline comment as done.Nov 12 2019, 5:25 PM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

both what?
A simple test is what I wanted, thx.

ABataev added inline comments.Nov 12 2019, 6:17 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

Both - it won't work now and in tbe future it is going to be very hard to adapt clang to this interface.

228 ↗(On Diff #228792)

I mean, handling of the cleanups.

ABataev added inline comments.Nov 13 2019, 7:10 AM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

As an example, you can take a look at the code in clang/test/OpenMP/cancel_codegen_cleanup.cpp test. It is very simple. The simplest version of the same code will something like this:

struct Obj {
  int a;
  Obj();
  ~Obj();
};

void foo() {
      #pragma omp for
      for (int i=0; i<1000; i++) {
            if(i==100) {
                Obj obj;
                #pragma omp cancel for
            }
        }
}

The object obj won't be deleted correctly with your scheme.

jdoerfert marked an inline comment as done.Nov 13 2019, 5:33 PM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

How did you run/compare this to come to the conclusion it does not work?

I run it with the OpenMPIRBuilder for barriers enabled (D69922 + -fopenmp-enable-irbuilder) and without, here is the full diff:

-declare dso_local void @__kmpc_barrier(%struct.ident_t*, i32)
+declare void @__kmpc_barrier(%struct.ident_t*, i32)

I don't see what you mean by it doesn't work, looks fine to me.


The above notwithstanding, if you have examples that expose problems with this patch, please let me know.

ABataev added inline comments.Nov 13 2019, 5:52 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

Try this one:

struct Obj {
  int a;
  Obj();
  ~Obj();
};

void foo() {
      #pragma omp parallel
      for (int i=0; i<1000; i++) {
            if(i==100) {
                Obj obj;
                #pragma omp cancel parallel
                #pragma omp barrier
            }
        }
}
jdoerfert marked an inline comment as done.Nov 13 2019, 7:04 PM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

Same result, cancel semantic is unaffected. Are you trying these?

ABataev added inline comments.Nov 13 2019, 7:28 PM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

There must be different code for _kmpc_cancel_barrier call and further processing. Will try to check with your patch tomorrow.

ABataev added inline comments.Nov 14 2019, 10:50 AM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
200 ↗(On Diff #228792)

Maybe split emission ща __kmpc_barrier and __kmpc_cancel_barrier functions into 2 independent functions fo the frontends? Rather than rely on the boolean flags?

228 ↗(On Diff #228792)

Ok, I see, you're using the block that jumps through the cleanups. Ok, this seems good.

jdoerfert marked an inline comment as done.Nov 14 2019, 10:56 AM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
200 ↗(On Diff #228792)

The frontend doesn't know necessarily when a barrier is issued if it is a cancellable, arguably it shouldn't need to know at all

I copied the flags from clang but I will look into removing them eventually (and to add a TODO in the meantime).

jdoerfert marked an inline comment as done.Nov 14 2019, 11:03 AM
jdoerfert added inline comments.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
228 ↗(On Diff #228792)

Yes. I also used the same logic in the generic solution (D70258) that will work with both the old code gen and the new one, e.g., D70109.

ABataev added inline comments.Nov 21 2019, 9:12 AM
llvm/lib/Frontend/OpenMPIRBuilder.cpp
152–155 ↗(On Diff #228792)

better to use streams

169–174 ↗(On Diff #228792)

Will it work with the late outlined parallel regions?

jdoerfert marked 4 inline comments as done.Nov 21 2019, 9:45 AM

Anything else?

llvm/lib/Frontend/OpenMPIRBuilder.cpp
152–155 ↗(On Diff #228792)

Sure.

169–174 ↗(On Diff #228792)

I'll remove the caching here. No need for it given that we will deduplicate anyway.

llvm/include/llvm/IR/OpenMPKinds.def
186 ↗(On Diff #227902)

Apologies for the digression. There is no change to the status quo so considering how to improve matters later cannot be blocking.

jdoerfert marked 4 inline comments as done.

use streams, remove caching

@ABataev @JonChesterfield anything else blocking this patch?

I'd very much like this to land soon. It's the prereq for a lot of other patches and the code looks good.

It's tricky to test the infra before the users are landed so the unit test is particularly appreciated.

rogfer01 added inline comments.Nov 27 2019, 11:06 PM
llvm/include/llvm/Frontend/OpenMPKinds.def
165 ↗(On Diff #230928)

As we migrate, we will end with a significant number of interfaces here.

@jdoerfert what do you think about adding a comment with their C prototype before each one like we do in clang/lib/CodeGen/CGOpenMPRuntime.cpp?

Something like this

// void __kmpc_barrier(ident_t *loc, kmp_int32 global_tid);
__OMP_RTL(__kmpc_barrier, false, Void, IdentPtr, Int32)
// kmp_int32 __kmpc_cancel_barrier(ident_t *loc, kmp_int32
// global_tid)
__OMP_RTL(__kmpc_cancel_barrier, false, Int32, IdentPtr, Int32)
...
jdoerfert marked an inline comment as done.Nov 28 2019, 10:44 AM

I'd very much like this to land soon. It's the prereq for a lot of other patches and the code looks good.

It's tricky to test the infra before the users are landed so the unit test is particularly appreciated.

I'm confused. Was this a review? I'm waiting for a decision here so we can move on and improve on this instead of me modifying it inp-lace two comments at a time.

llvm/include/llvm/Frontend/OpenMPKinds.def
165 ↗(On Diff #230928)

I'm fine with this but I doubt it'll help much (compared to the lines we have that show name and types).

If you want this to happen you should create a patch do add comments for the ones we have here, and others can way in. If there is agreement to apply it, we will do so and continue that tradition from then on. Does that sound good?

rogfer01 added inline comments.Nov 28 2019, 11:52 AM
llvm/include/llvm/Frontend/OpenMPKinds.def
165 ↗(On Diff #230928)

Sounds reasonable to me. Thanks!

JonChesterfield accepted this revision.Dec 1 2019, 6:09 PM

I'm confused. Was this a review? I'm waiting for a decision here so we can move on and improve on this instead of me modifying it inp-lace two comments at a time.

Explicitly marked as accepted. Patch has looked good for a while and even has other people building on it.

This revision is now accepted and ready to land.Dec 1 2019, 6:09 PM

Adjust path

ABataev added inline comments.Dec 11 2019, 11:18 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
74

Extra comments?

93

Here too.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
175–178

Are you sure it will work correctly with late outlining?

187–189

Clang drops insert point after some code, like call of exit() etc. That's why we need to check it in the frontend, otherwise, the compiler crashes.

Unit tests: fail. 60698 tests passed, 1 failed and 726 were skipped.

failed: LLVM-Unit.Frontend/_/LLVMFrontendTests/OpenMPIRBuilderTest.CreateCancelBarrier

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

jdoerfert marked 6 inline comments as done.Dec 11 2019, 11:40 AM
jdoerfert added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
74

I don't know what you want to tell me.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
175–178

Yes.

jdoerfert marked 2 inline comments as done.

Remove call caching in anticipation of D69930

ABataev added inline comments.Dec 11 2019, 11:58 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
74

I mean, you have this ///{ here. Do you need it?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
175–178

Ah, the code fro threadid changed already, probably missed it.

jdoerfert marked 2 inline comments as done.Dec 11 2019, 12:17 PM
jdoerfert added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
74

Yes, to help the doxygen documentation.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
175–178

I removed the TODOs now, including the caching.

This revision was automatically updated to reflect the committed changes.