Page MenuHomePhabricator

[OpenMP][OMPIRBuilder] Use the source (=directory + filename) for locations
ClosedPublic

Authored by jdoerfert on Aug 13 2020, 2:38 PM.

Diff Detail

Event Timeline

jdoerfert created this revision.Aug 13 2020, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2020, 2:38 PM
jdoerfert requested review of this revision.Aug 13 2020, 2:38 PM
lebedev.ri accepted this revision.Aug 13 2020, 2:42 PM

Thanks. Seems to fix the test for me FWIW, but

  1. why wasn't this caught by anyone and all of the bots?
  2. what is the actual underlying problem, if any?
This revision is now accepted and ready to land.Aug 13 2020, 2:42 PM
jdoerfert planned changes to this revision.Aug 13 2020, 3:45 PM

Something is fishy here. Now the pre-merge tests fail... this should accept strictly more... I have to look into this, best guess: some non-determinism

Thanks. Seems to fix the test for me FWIW, but

  1. why wasn't this caught by anyone and all of the bots?

For what it's worth, I have been affected by this and was drafting a note to D82822 today to ask about the status.

  1. what is the actual underlying problem, if any?

Root cause:
We sometimes, depending on the invocation and potentially other factors, use a full path and sometimes not:
So wen I run this with clang locally I get this:

@98 = private unnamed_addr constant [98 x i8] c";/data/src/llvm-project/clang/test/OpenMP/irbuilder_nested_parallel_for.c;parallel_for_2;294;16;;\00", align 1                                                                                               
@100 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([98 x i8], [98 x i8]* @98, i32 0, i32 0) }, align 8
@101 = private unnamed_addr constant [92 x i8] c";src/llvm-project/clang/test/OpenMP/irbuilder_nested_parallel_for.c;parallel_for_2;294;16;;\00", align 1
@103 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([92 x i8], [92 x i8]* @101, i32 0, i32 0) }, align 8

Note the 6 missing characters /data/ in the second string. Because they are missing, we generate two globals for this. When I run the same test with the update script I get one global as they agree on the location.

Looking for the location that strips the path now.

jdoerfert updated this revision to Diff 285551.Aug 13 2020, 8:36 PM

Resolve the root cause

This revision is now accepted and ready to land.Aug 13 2020, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2020, 8:36 PM
jdoerfert retitled this revision from [OpenMP][NFC] Update check lines after D85099 to [OpenMP][OMPIRBuilder] Use the source (=directory + filename) for locations.Aug 13 2020, 8:36 PM

still lg, i guess

clang/test/OpenMP/irbuilder_nested_parallel_for.c
258 ↗(On Diff #285551)

Please precommit test regeneration first.

This revision was landed with ongoing or failed builds.Aug 14 2020, 7:06 AM
This revision was automatically updated to reflect the committed changes.