Open kiranktp opened 1 year ago
@llvm/issue-subscribers-clang-codegen
@llvm/issue-subscribers-openmp
Verified with trunk: https://godbolt.org/z/K4Mob595f
Does it meet the requirements for the canonical loop nest?
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?
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.
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?
It is not a type issue, jb is not a outer loop invariant. Crashing is not good, though, need to emit error message.
@alexey-bataev Are you planning to fix this issue?
No, you can do it.
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:
So the compiler should be allowing the test case to pass?
By no means should the compiler crash, so definitely it is an issue.
By no means should the compiler crash, so definitely it is an issue.
Definitely.
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[^1], not just since Open 5.1.
[^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.
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
- 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.
@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.
Am, I still think the crash should be fixed, the compiler still crashes
The crash should definitely be fixed. So the compiler should be throwing an error about the variable right.
https://github.com/llvm/llvm-project/pull/101305 (apologies for my confusion about GitHub issue numbers vs. PR numbers...)
Variable
jb
defined in outer loop causing the Clang to abort: Initial investigation showed that the variablejb
was not registered inLocalDeclMap
.