jhuber6 marked an inline comment as done.
jhuber6 added inline comments.
================
Comment at: openmp/runtime/test/tasking/kmp_taskloop.c:100
th_counter[i] = 0;
- #pragma omp parallel num_threads(N)
+ #pragma omp parallel // num_threads(N)
{
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > jhuber6 wrote:
> > > jdoerfert wrote:
> > > > jhuber6 wrote:
> > > > > jdoerfert wrote:
> > > > > > jhuber6 wrote:
> > > > > > > AndreyChurbanov wrote:
> > > > > > > > jhuber6 wrote:
> > > > > > > > > jhuber6 wrote:
> > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > jhuber6 wrote:
> > > > > > > > > > > > I am not entirely sure why, but commenting this out
> > > > > > > > > > > > causes the problem to go away. I tried adding proper
> > > > > > > > > > > > names to the forward-declared functions but since clang
> > > > > > > > > > > > already knew I had something called ident_t, I couldn't
> > > > > > > > > > > > declare a new struct with the same name.
> > > > > > > > > > > This is not good. The difference should only be that the
> > > > > > > > > > > `kmpc_fork_call` has a different argument, right? Does
> > > > > > > > > > > the segfault happen at compile or runtime?
> > > > > > > > > > >
> > > > > > > > > > > You can just use the ident_t clang created, right? Did
> > > > > > > > > > > you print the function names requested by clang as we
> > > > > > > > > > > discussed?
> > > > > > > > > > I added an assertion and debug statements. If I try to
> > > > > > > > > > declare a struct named "Ident_t" I get the following error
> > > > > > > > > > message in the seg-fault. I think the seg-fault is
> > > > > > > > > > compile-time.
> > > > > > > > > >
> > > > > > > > > > Found OpenMP runtime function __kmpc_global_thread_num with
> > > > > > > > > > type i32 (%struct.ident_t.0*). Expected type is i32
> > > > > > > > > > (%struct.ident_t*)
> > > > > > > > > > clang:
> > > > > > > > > > /home/jhuber/Documents/llvm-project/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:124:
> > > > > > > > > > static llvm::Function*
> > > > > > > > > > llvm::OpenMPIRBuilder::getOrCreateRuntimeFunction(llvm::Module&,
> > > > > > > > > > llvm::omp::RuntimeFunction): Assertion `FnTy ==
> > > > > > > > > > Fn->getFunctionType() && "Found OpenMP runtime function has
> > > > > > > > > > mismatched types"' failed.
> > > > > > > > > I'm not sure if there's a way around this without changing
> > > > > > > > > the getOrCreateRuntimeFunction method to return a
> > > > > > > > > FunctionCallee and removing the assertion. Clang doesn't know
> > > > > > > > > about the ident_t struct when it's compiling the file, but
> > > > > > > > > when its doing the codegen it sees two structs with the same
> > > > > > > > > name and creates a new name. So when it gets the types it
> > > > > > > > > says that ident_t and ident_t.0 don't match. As you said the
> > > > > > > > > old version got around this by adding a bitcasting
> > > > > > > > > instruction so it knew how to turn it into an ident_t pointer.
> > > > > > > > Note that this change breaks the test on any system with more
> > > > > > > > that 4 procs. Because array th_counter[4] is indexed by thread
> > > > > > > > number which can easily be greater than 3 if number of threads
> > > > > > > > is not limited.
> > > > > > > The problem was that the num_threads clause required an implicit
> > > > > > > call to kmpc_global_thread_num so it could be passed to
> > > > > > > kmpc_push_num_threads. The types of the implicit function and the
> > > > > > > forward declaration then wouldn't match up. I added another
> > > > > > > forward declaration to explicitly call kmpc_push_num_threads. Is
> > > > > > > this a sufficient solution?
> > > > > > We need this to work with num_threads(8).
> > > > > >
> > > > > > > Clang doesn't know about the ident_t struct when it's compiling
> > > > > > > the file, but when its doing the codegen it sees two structs with
> > > > > > > the same name and creates a new name.
> > > > > >
> > > > > > Where are the two structs coming from? We should have one. If clang
> > > > > > introduces one it needs to use the one from OMPKindes.def instead.
> > > > > > Is that a fix?
> > > > > The first struct is the one that I'm assuming comes from the OpenMP
> > > > > CodeGen that places the Ident_t struct in the IR file. if I declare a
> > > > > struct also named ident_t in the C source file it most likely will
> > > > > see that there's two structs with the same name and call the second
> > > > > one "ident_t.0" internally. The other ident_t struct is only known
> > > > > once clang generates the LLVM IR so I can't just use "ident_t" nor
> > > > > can I declare a struct with the same name.
> > > > 1) Either Clang needs to use the `llvm::omp::types::Ident` *or* Clang
> > > > needs to define `llvm::omp::types::Ident` and we do not do it via
> > > > `__OMP_STRUCT_TYPE(Ident, ident_t, Int32, Int32, Int32, Int32,
> > > > Int8Ptr)`. I would prefer the first solution.
> > > >
> > > > 2) `OMPConstants.cpp` does pick up an existing struct type with the
> > > > same name if present. That is, probably not what we want because it
> > > > clashes with user types.
> > > >
> > > > 3) We can still return a `FunctionCallee` with the Function* and the
> > > > expected type (as defined by OMPKinds.def) to mimic the old behavior
> > > > for now.
> > > > Either Clang needs to use the `llvm::omp::types::Ident` *or* Clang
> > > > needs to define `llvm::omp::types::Ident` and we do not do it via
> > > > `__OMP_STRUCT_TYPE(Ident, ident_t, Int32, Int32, Int32, Int32,
> > > > Int8Ptr)`. I would prefer the first solution.
> > > I'm probably not understanding something correctly here. There's already
> > > an ident_t type declared in `CGOpenMPRuntime:1065` which it uses for
> > > generating the types for the runtime functions, but this isn't used until
> > > code generation so it can't be used while compiling the file. If we
> > > declare it up front then wouldn't that make ident_t a reserved keyword?
> > >
> > > > We can still return a `FunctionCallee` with the `Function*` and the
> > > > expected type (as defined by OMPKinds.def) to mimic the old behavior
> > > > for now.
> > > A FunctionCallee can be generated from a Function * but not vice-versa,
> > > right? This would require changing the code in OpenMPIRBuilder.
> > In CGOpenMPRuntime.cpp there is:
> > `IdentTy = CGM.getTypes().ConvertRecordDeclType(RD);`
> > what happens if you replace that with:
> > `IdentTy = llvm::omp::types::Ident;`
> > ?
> >
> > ---
> >
> > FunctionCallee is just a Function* and the "expected type". You can go from
> > FunctionCallee to Function by just picking the proper member. You'd need to
> > change the Builder in two ways: 1) return FunctionCallee(Fn, ExpectedType);
> > 2) where it is used extract the Function * from the FunctionCallee. You
> > might want to use a helper if you go this route.
> It causes all the OpenMP tests to fail, not sure why exactly. I'll look into
> it more tomorrow.
I'm not exactly sure why, but if you move the
`llvm::omp::types:InitializeTypes(CGM.getModule())` statement before where
`IdenTy` is assigned it causes some of the tests to fail without changing
anything else. Any clue why?
Instead I changed `llvm::Type *CGOpenMPRuntime::getIdentTyPointerTy()` in
`CGOpenMPRuntime:1594` to return `llvm::omp::types::Ident->getPointerTo()`
which is the only place `IdentTy` is used. But it has the same problem.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80222/new/
https://reviews.llvm.org/D80222
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
