If we deduplicate OpenMP runtime calls we have multiple ident_t* that
represent information like source location. So far, we simply kept the
one used by the replacement call. However, as exposed by PR44893, that
can cause problems if we have stack allocated ident_t objects. While
we need to revisit the use of these as well, it is clear that we
eventually want to merge source location information in some way. With
this patch we add the infrastructure to do so but without doing the
actual merge. Instead we pick a global ident_t from the replaced
calls, if possible, or create a new one with an unknown location
instead.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Cool. Can we reasonably add the reproducible from 44893 to a regression suite, in addition to these IR tests? It's written in C so would need to be under clang's tests.
This looks like a good fix for the reported bug. I don't see why this failure mode would be unique to ident_t though, at least from re-reading deduplicateRuntimeCalls - is it a feature of the current set of runtime calls that can be deduplicated?
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
---|---|---|
191–193 | Probably don't want to drop the private annotation here |
Done.
This looks like a good fix for the reported bug. I don't see why this failure mode would be unique to ident_t though, at least from re-reading deduplicateRuntimeCalls - is it a feature of the current set of runtime calls that can be deduplicated?
Good catch. Added enough code to be future proof wrt. miscompiles. Also added an extra test in IR.
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
---|---|---|
191–193 | I actually do. The new and some of the below functions should be exposed. |
Probably don't want to drop the private annotation here