Page MenuHomePhabricator

[OpenMP][FIX] Consistently use OpenMPIRBuilder if requested
ClosedPublic

Authored by jdoerfert on Jun 29 2020, 4:44 PM.

Details

Summary

When we use the OpenMPIRBuilder for the parallel region we need to also
use it to get the thread ID (among other things) in the body. This is
because CGOpenMPRuntime::getThreadID() and
CGOpenMPRuntime::emitUpdateLocation implicitly assumes that if they are
called from within a parallel region there is a certain structure to the
code and certain members of the OMPRegionInfo are initialized. It might
make sense to initialize them even if we use the OpenMPIRBuilder but we
would preferably get rid of such state instead.

Bug reported by Anchu Rajendran Sudhakumari.

Depends on D82470.

Diff Detail

Event Timeline

jdoerfert created this revision.Jun 29 2020, 4:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 4:44 PM
anchu-rajendran accepted this revision.Jun 30 2020, 9:02 PM

LGTM :). Thanks!

clang/lib/CodeGen/CGOpenMPRuntime.cpp
1544

Nit: I think its better to add comments on the implicit assumptions in clang code / on why initializations here is not required if OMPIRBuilder is not used.

This revision is now accepted and ready to land.Jun 30 2020, 9:02 PM
This revision was landed with ongoing or failed builds.Jul 30 2020, 8:21 AM
This revision was automatically updated to reflect the committed changes.

Hello, I have a twice-daily auto-bisecting multi-stage cron job running on Fedora 32 (x86-64) that has identified this commit as failing my first stage test (release + no asserts). Is this expected? Can we get a quick fix or revert this?

FAIL: Clang :: OpenMP/irbuilder_nested_parallel_for.c (13233 of 67864)

  • TEST 'Clang :: OpenMP/irbuilder_nested_parallel_for.c' FAILED ****

Script:

: 'RUN: at line 2'; /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-enable-irbuilder -x c++ -emit-llvm /home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -o - | /tmp/_update_lc/r/bin/FileCheck --check-prefixes=CHECK /home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c

: 'RUN: at line 3'; /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -fopenmp -fopenmp-enable-irbuilder -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -debug-info-kind=limited -std=c++11 -verify /home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c -emit-llvm -o - | /tmp/_update_lc/r/bin/FileCheck --check-prefixes=CHECK-DEBUG /home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c

Exit Code: 1

Command Output (stderr):

/home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c:74:22: error: CHECK-DEBUG-NEXT: is not on the line after the previous match
// CHECK-DEBUG-NEXT: [[OMP_GLOBAL_THREAD_NUM:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @12), !dbg !{{[0-9]*}}

^

<stdin>:252:2: note: 'next' match was here
%omp_global_thread_num1 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @12), !dbg !52
^
<stdin>:224:105: note: previous match ended here
call void @llvm.dbg.declare(metadata double* %b.addr, metadata !45, metadata !DIExpression()), !dbg !46

                        ^

<stdin>:225:1: note: non-matching line after previous match is here
%omp_global_thread_num = call i32 @__kmpc_global_thread_num(%struct.ident_t* @10), !dbg !47
^
/home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c:192:22: error: CHECK-DEBUG-NEXT: expected string not found in input
// CHECK-DEBUG-NEXT: [[OMP_GLOBAL_THREAD_NUM:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @25), !dbg !{{[0-9]*}}

^

<stdin>:381:2: note: scanning from here
%omp_global_thread_num = call i32 @__kmpc_global_thread_num(%struct.ident_t* @21), !dbg !83
^
<stdin>:381:19: note: possible intended match here
%omp_global_thread_num = call i32 @__kmpc_global_thread_num(%struct.ident_t* @21), !dbg !83

^

Input file: <stdin>
Check file: /home/dave/ro_s/lp/clang/test/OpenMP/irbuilder_nested_parallel_for.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<

  .
  .
  .
247:
248: omp.par.outlined.exit19.exitStub: ; preds = %omp.par.pre_finalize
249:  ret void
250:
251: omp.par.region: ; preds = %omp.par.entry
252:  %omp_global_thread_num1 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @12), !dbg !52

next:74 !~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: match on wrong line

253:  br label %omp_parallel
254:
255: omp_parallel: ; preds = %omp.par.region
256:  call void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* @12, i32 3, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i32*, double*, float**)* @_Z14parallel_for_1Pfid..omp_par to void (i32*, i32*, ...)*), i32* %a.addr, double* %b.addr, float** %r.addr), !dbg !53
257:  br label %omp.par.outlined.exit
  .
  .
  .
376:  call void @llvm.dbg.declare(metadata float** %r.addr, metadata !77, metadata !DIExpression()), !dbg !78
377:  store i32 %a, i32* %a.addr, align 4
378:  call void @llvm.dbg.declare(metadata i32* %a.addr, metadata !79, metadata !DIExpression()), !dbg !80
379:  store double %b, double* %b.addr, align 8
380:  call void @llvm.dbg.declare(metadata double* %b.addr, metadata !81, metadata !DIExpression()), !dbg !82
381:  %omp_global_thread_num = call i32 @__kmpc_global_thread_num(%struct.ident_t* @21), !dbg !83

next:192'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
next:192'1 ? possible intended match

382:  br label %omp_parallel

next:192'0 ~~~~~~~~~~~~~~~~~~~~~~~

383:

next:192'0 ~

384: omp_parallel: ; preds = %entry

next:192'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

385:  call void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* @21, i32 3, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i32*, double*, float**)* @_Z14parallel_for_2Pfid..omp_par.4 to void (i32*, i32*, ...)*), i32* %a.addr, double* %b.addr, float** %r.addr), !dbg !84

next:192'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

386:  br label %omp.par.outlined.exit211

next:192'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.
.
.

Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..


Failed Tests (1):

Clang :: OpenMP/irbuilder_nested_parallel_for.c

Testing Time: 124.68s

Unsupported      : 10711
Passed           : 57049
Expectedly Failed:   103
Failed           :     1

@davezarzycki The bots reported this as well, didn't build MLIR locally, this should have been fixed by 4d83aa4771d84940626d86c883193af390812281

hoyFB added a subscriber: hoyFB.Aug 2 2020, 1:24 AM

@davezarzycki The bots reported this as well, didn't build MLIR locally, this should have been fixed by 4d83aa4771d84940626d86c883193af390812281

I was seeing the same issue with what @davezarzycki has encountered. Not sure why this is related to MLIR. The failure is cause by this line:

74: // CHECK-DEBUG-NEXT:    [[OMP_GLOBAL_THREAD_NUM:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* **@12**), !dbg !{{[0-9]*}}

where @12 is supposed to be defined as

@11 = private unnamed_addr constant [90 x i8] c";llvm-project/clang/test/OpenMP/irbuilder_nested_parallel_for.c;parallel_for_1;85;1;;\00", align 1
@12 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([90 x i8], [90 x i8]* @11, i32 0, i32 0) }, align 8

The exact match of @12 seems a bit fragile to me since global data names could be changed from time to time. Can you please make it bit more robust? Thanks.

@davezarzycki The bots reported this as well, didn't build MLIR locally, this should have been fixed by 4d83aa4771d84940626d86c883193af390812281

I was seeing the same issue with what @davezarzycki has encountered. Not sure why this is related to MLIR. The failure is cause by this line:

74: // CHECK-DEBUG-NEXT:    [[OMP_GLOBAL_THREAD_NUM:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* **@12**), !dbg !{{[0-9]*}}

where @12 is supposed to be defined as

@11 = private unnamed_addr constant [90 x i8] c";llvm-project/clang/test/OpenMP/irbuilder_nested_parallel_for.c;parallel_for_1;85;1;;\00", align 1
@12 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([90 x i8], [90 x i8]* @11, i32 0, i32 0) }, align 8

The exact match of @12 seems a bit fragile to me since global data names could be changed from time to time. Can you please make it bit more robust? Thank

I just created D85099 to eliminate the problem permanently. Feel free to take a look.

hoyFB added a comment.Aug 2 2020, 2:09 PM

@davezarzycki The bots reported this as well, didn't build MLIR locally, this should have been fixed by 4d83aa4771d84940626d86c883193af390812281

I was seeing the same issue with what @davezarzycki has encountered. Not sure why this is related to MLIR. The failure is cause by this line:

74: // CHECK-DEBUG-NEXT:    [[OMP_GLOBAL_THREAD_NUM:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* **@12**), !dbg !{{[0-9]*}}

where @12 is supposed to be defined as

@11 = private unnamed_addr constant [90 x i8] c";llvm-project/clang/test/OpenMP/irbuilder_nested_parallel_for.c;parallel_for_1;85;1;;\00", align 1
@12 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([90 x i8], [90 x i8]* @11, i32 0, i32 0) }, align 8

The exact match of @12 seems a bit fragile to me since global data names could be changed from time to time. Can you please make it bit more robust? Thank

I just created D85099 to eliminate the problem permanently. Feel free to take a look.

Thanks for the quick turnaround!