HomePhabricator

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

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

Description

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

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.

Reviewed By: JonChesterfield

Differential Revision: https://reviews.llvm.org/D74925

Details

Committed
jdoerfertFeb 25 2020, 2:07 PM
Reviewer
JonChesterfield
Differential Revision
D74925: [OpenMP][Opt] Combine `struct ident_t*` during deduplication
Parents
rG0906dca493b3: [WebAssembly] Simplify extract_vector lowering
Branches
Unknown
Tags
Unknown

Event Timeline

dexonsmith added inline comments.
/clang/test/OpenMP/PR44893.c
1

Can you change this to a -cc1 test (invoking %clang_cc1), instead of calling the driver?

3

Can you add some sort of positive test, related to where the crash was?

If this was a debug info related crash, maybe you can test that the debug info IR is reasonable (@aprantl may be able to help).

If not, please remove the -g from the command-line, and just test that the normal IR is reasonable.

jdoerfert marked 2 inline comments as done.Feb 25 2020, 10:12 PM
jdoerfert added inline comments.
/clang/test/OpenMP/PR44893.c
1

I don't think I can, or at least I did not figure out how without loosing the desired effect (see below). Is it bad to invoke clang here? I actually did a grep of %clang and found other tests doing it so I figured it was fine. What I "need" is -g and -fopenmp, if I can tell that to cc1 I'm happy to change it.

3

Can you add some sort of positive test, related to where the crash was?

I did, in deduplication.ll.

If this was a debug info related crash, maybe you can test that the debug info IR is reasonable (@aprantl may be able to help).

It was, kind of. With debug info enabled (-g) OpenMP code generation does some stuff that it does not without. It has actually nothing to do with debug info per se but with the location of the struct ident_t.

If not, please remove the -g from the command-line, and just test that the normal IR is reasonable.

I have the IR as a test (deduplication.ll). Dropping -g here does not make sense, if it is a problem I would drop the entire test instead.

aprantl added inline comments.Feb 27 2020, 8:31 AM
/clang/test/OpenMP/PR44893.c
3

A test with no CHECK lines also passes if you symlink /bin/true to clang :-)
It also doesn't indicate what is being relevant and when it comes to maintaining / updating the test we won't know what is important to keep.
The fact that there was a crash that wasn't hit by the test suite indicates that there was some behavior that wasn't tested before, so we should really try to check that behavior.

jdoerfert marked an inline comment as done.Mar 13 2020, 9:41 AM
jdoerfert added inline comments.
/clang/test/OpenMP/PR44893.c
3

A test with no CHECK lines also passes if you symlink /bin/true to clang :-)
It also doesn't indicate what is being relevant and when it comes to maintaining / updating the test we won't know what is important to keep.

This is a common pattern, except the %clang part maybe. I'm fine with removing this test as the actual issue is covered explicitly, see below. This was added as the original reproducer, not the minimal one (which is IR).

The fact that there was a crash that wasn't hit by the test suite indicates that there was some behavior that wasn't tested before, so we should really try to check that behavior.

> Can you add some sort of positive test, related to where the crash was?

I did, in deduplication.ll.

I did, in deduplication.ll