[Clang][OpenMP] Clang aborts for collapse clause with imprefect nested loops

[kiranktp]

Variable jb defined in outer loop causing the Clang to abort:
Initial investigation showed that the variable jb was not registered in LocalDeclMap.

$ cat -n test.c
1  void func( double *A, int N, int M, int NB ) {
2  #pragma omp parallel
3      {
4          int nblks = (N-1)/NB;
5          int lnb = ((N-1)/NB)*NB;
6  #pragma omp for collapse(2)
7          for (int jblk = 0 ; jblk < nblks ; jblk++ ) {
8              **int jb = (jblk == nblks - 1 ? lnb : NB);**
9              for ( int jk = 0 ; jk < jb ; jk++ ) {
10              }
11          }
12      }
13  }
$ clang --version
clang version 16.0.0 (https://github.com/llvm/llvm-project.git a28f3f8e48cb3935d413d3b9c0fcd5f72dcd5b8a)
...
$ clang -c -fopenmp test.c
DeclRefExpr for Decl not entered in LocalDeclMap?
UNREACHABLE executed at /home/user/llvm-flang-src/llvm-project/clang/lib/CodeGen/CGExpr.cpp:2842!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: /home/user/llvm-flang-src/llvm-project/install_rel/bin/clang -c -fopenmp test.c
1.      <eof> parser at end of file
2.      Per-file LLVM IR generation
3.      test.c:1:6: Generating code for declaration 'func'
4.      test.c:3:5: LLVM IR generation of compound statement ('{}')
	#0 0x00007f10ccd3de63 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/user/llvm-flang-src/llvm-project/llvm/lib/Support/Unix/Signals.inc:567:13
 #1 0x00007f10ccd3bc4e llvm::sys::RunSignalHandlers() /home/user/llvm-flang-src/llvm-project/llvm/lib/Support/Signals.cpp:105:18
	#2 0x00007f10ccd3d28d llvm::sys::CleanupOnSignal(unsigned long) /home/user/llvm-flang-src/llvm-project/llvm/lib/Support/Unix/Signals.inc:0:3
	#3 0x00007f10ccc4833e (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) /home/user/llvm-flang-src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:77:5
	#4 0x00007f10ccc48590 CrashRecoverySignalHandler(int) /home/user/llvm-flang-src/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:398:1
	#5 0x00007f10cc5d0520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
	#6 0x00007f10cc624a7c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
	#7 0x00007f10cc624a7c __pthread_kill_internal ./nptl/pthread_kill.c:78:10
	#8 0x00007f10cc624a7c pthread_kill ./nptl/pthread_kill.c:89:10
	#9 0x00007f10cc5d0476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
#10 0x00007f10cc5b67f3 abort ./stdlib/abort.c:81:7
#11 0x00007f10ccc5d7f1 (/home/user/llvm-flang-src/llvm-project/install_rel/bin/../lib/libLLVMSupport.so.16git+0x1747f1)
#12 0x00007f10d0cf0468 clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(clang::DeclRefExpr const*) /home/user/llvm-flang-src/llvm-project/clang/lib/CodeGen/CGExpr.cpp:0:0
#13 0x00007f10d0ce2541 clang::CodeGen::CodeGenFunction::EmitLValue(clang::Expr const*) /home/user/llvm-flang-src/llvm-project/clang/lib/CodeGen/CGExpr.cpp:0:12
#14 0x00007f10d0cec9ab clang::CodeGen::CodeGenFunction::EmitCheckedLValue(clang::Expr const*, clang::CodeGen::CodeGenFunction::TypeCheckKind) /home/user/llvm-flang-src/llvm-project/clang/lib/CodeGen/CGExpr.cpp:1241:8
		......

[llvmbot]

@llvm/issue-subscribers-clang-codegen

[llvmbot]

@llvm/issue-subscribers-openmp

[jdoerfert]

Verified with trunk: https://godbolt.org/z/K4Mob595f

[alexey-bataev]

Does it meet the requirements for the canonical loop nest?

[kiranktp]

As i understand there is a change in the standard from 5.0 to 5.1

OpenMP 5.0: Restrictions under "2.9.2 Worksharing-Loop Construct":

---

...
� If a collapse clause is specified, exactly one loop must occur in the region at each nesting
level up to the number of loops specified by the parameter of the collapse clause.
...

---

IMO, It means the loop has to be perfectly nested loop.

Where as this restriction has been removed in OpenMP 5.1: Restrictions under "2.11.4 Worksharing-Loop Construct".

There is a corresponding code change in clang as well:
lib/Sema/SemaOpenMP.cpp:
9569 /// Called on a for stmt to check itself and nested loops (if any).
9570 /// \return Returns 0 if one of the collapsed stmts is not canonical for loop,
9571 /// number of collapsed loops otherwise.
9572 static unsigned
9573 checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
9574 Expr *OrderedLoopCountExpr, Stmt *AStmt, Sema &SemaRef,
9575 DSAStackTy &DSA,
9576 Sema::VarsWithInheritedDSAType &VarsWithImplicitDSA,
9577 OMPLoopBasedDirective::HelperExprs &Built) {
9578 unsigned NestedLoopCount = 1;
9579 bool SupportsNonPerfectlyNested = (SemaRef.LangOpts.OpenMP >= 50) &&
9580 !isOpenMPLoopTransformationDirective(DKind);

I means, Non perfectly nested loops are allowed.
My understanding correct?

All GNU based compilers and intel proprietary compiler throw an error for this test case:

error: not enough perfectly nested loops before 'int
Where as all clang based compiler allow this and crashes later during symbol resolution.

What should compiler do for this test case?

[alexey-bataev]

It is not only nob-perfectly nested. According to 4.4.1 Canonical Loop Nest Form, lb/ub (jb in this example) "Expressions of a type compatible with the type of var that are loop invariant with respect to the outermost loop.". In this example jb is not outermost loop invariant.

[nvarj]

According to this "Expressions of a type compatible with the type of var that are loop invariant with respect to the outermost loop." statement from the standard, type of lb or ub used in nested loops should be compatible with type of var that is a loop invariant in outermost loop. Here type of jb and type of var that are loop invariant with respect to outermost loop are compatible. Both are of type integer. Hence the test case should be valid right?

[alexey-bataev]

It is not a type issue, jb is not a outer loop invariant. Crashing is not good, though, need to emit error message.

[nvarj]

@alexey-bataev Are you planning to fix this issue?

[alexey-bataev]

No, you can do it.

[Meinersbur]

If a collapse clause is specified, exactly one loop must occur in the region at each nesting level up to the number of loops specified by the parameter of the collapse clause.

I don't think this ever prohibited imperfectly nested loops. The restriction requires that there are at least as many loops as specified by the collapse clause. Nothing is said about whether they are perfectly or imperfectly nested.

The feature history of 5.0 even mentions it explicitly:

[nvarj]

So the compiler should be allowing the test case to pass?

[shiltian]

By no means should the compiler crash, so definitely it is an issue.

[alexey-bataev]

By no means should the compiler crash, so definitely it is an issue.

Definitely.

[Meinersbur]

So the compiler should be allowing the test case to pass?

My comment was to argue that the input was already valid OpenMP since 5.0¹, not just since Open 5.1.

Footnotes

1. If it was not, it would be a crash on invalid input, which could be argued is a lower priority issue. It's not difficult to make Clang crash on arbitrary inputs. It's a moot point since we also want to support OpenMP 5.1 anyway. ↩

[alexey-bataev]

So the compiler should be allowing the test case to pass?

My comment was to argue that the input was already valid OpenMP since 5.01, not just since Open 5.1.

Footnotes

1. If it was not, it would be a crash on invalid input, which could be argued is a lower priority issue. It's not difficult to make Clang crash on arbitrary inputs. It's a moot point since we also want to support OpenMP 5.1 anyway. ↩

What do you mean by "valid"? That it should not crash the compiler? In this case it should be "valid" as soon as multi-collapsing is supported. The fact here that the input is "invalid" because lb is not outer loop invariant. But it does not mean that the compiler should crash.

[Meinersbur]

@alexey-bataev Correct, I missed that the crashing case was referring to a loop-variant local variable that is not a loop iteratation variable. I was only trying address the misunderstanding that collapsing non-perfectly loops was new in OpenMP 5.1.

[alexey-bataev]

Am, I still think the crash should be fixed, the compiler still crashes

[nvarj]

The crash should definitely be fixed. So the compiler should be throwing an error about the variable right.

[jtb20]

#101305 (apologies for my confusion about GitHub issue numbers vs. PR numbers...)