This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Opt] Combine `struct ident_t*` during deduplication
ClosedPublic

Authored by jdoerfert on Feb 20 2020, 12:23 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jdoerfert created this revision.Feb 20 2020, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 12:23 PM

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

Probably don't want to drop the private annotation here

jdoerfert marked an inline comment as done.

Add tests, addressed comments

Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 10:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

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

I actually do. The new and some of the below functions should be exposed.

JonChesterfield accepted this revision.Feb 24 2020, 4:40 PM

Nice. Thanks!

This revision is now accepted and ready to land.Feb 24 2020, 4:40 PM
This revision was automatically updated to reflect the committed changes.