Page MenuHomePhabricator

[Clang][OpenMP] Emit dependent PreInits before directive.
ClosedPublic

Authored by Meinersbur on May 10 2021, 10:09 AM.

Details

Summary

The PreInits of a loop transformation (atm moment only tile) include the computation of the trip count. The trip count is needed by any loop-associated directives that consumes the transformation-generated loop. Hence, we must ensure that the PreInits of consumed loop transformations are emitted with the consuming directive.

This is done by addinging the inner loop transformation's PreInits to the outer loop-directive's PreInits. The outer loop-directive will consume the de-sugared AST such that the inner PreInits are not emitted twice. The PreInits of a loop transformation are still emitted directly if its generated loop(s) are not associated with another loop-associated directive.

Diff Detail

Event Timeline

Meinersbur created this revision.May 10 2021, 10:09 AM
Meinersbur requested review of this revision.May 10 2021, 10:09 AM
ABataev added inline comments.May 10 2021, 10:27 AM
clang/lib/AST/StmtOpenMP.cpp
128–129

Do we need bool return in the callback? I see that it returns false always.

134–136
while (auto *Dir = dyn_cast<OMPTileDirective>(CurStmt)) {
  if (OnTransformationCallback(Dir))
    return false;
  CurStmt = Dir->getTransformedStmt();
}
Meinersbur added inline comments.May 10 2021, 10:36 AM
clang/lib/AST/StmtOpenMP.cpp
128–129

I added it to be consistent with the other callback. Going to remove it.

134–136

With OMPUnrollDirective added, there are two conditions of the while loop. Of course, I can change the structure again with the unroll patch.

  • Address review comments
Meinersbur marked 2 inline comments as done.May 10 2021, 10:45 AM
  • Don't return false for void lambda
ABataev added inline comments.May 10 2021, 12:05 PM
clang/lib/Sema/SemaOpenMP.cpp
8977

No need for return false;

12611

Same, no need for return

  • Remove unused 'this' capture
  • Preserve relative order between OriginalInits and interleaved transformation's PreInits
ABataev added inline comments.May 14 2021, 7:18 AM
clang/lib/Sema/SemaOpenMP.cpp
12579–12581

SmallVector<SmallVector<llvm::PointerUnion<Stmt *, Decl *>>> OriginalEmits(1);

Meinersbur marked 2 inline comments as done.
  • Rebase
  • Address review
Meinersbur marked an inline comment as done.May 14 2021, 2:17 PM
ABataev added inline comments.May 14 2021, 2:25 PM
clang/lib/Sema/SemaOpenMP.cpp
12579–12580

Why still std::vector?

Meinersbur added inline comments.May 14 2021, 2:40 PM
clang/lib/Sema/SemaOpenMP.cpp
12579–12580

I didn't notice that the vector type was changed, only notices the added argument for one pre-allocated element.

The motivation for std::vector is lower memory requirement. SmallVectorImpl cannot be used because it it is nested in the template. SmallVector will pre-allocate a large amount of memory (times 4) and expensive to move when the outer SmallVector has to be resized.

However, I could also use SmallVector<T, 0>.

  • Merge branch 'arcpatch-D102180' into HEAD
  • Use SmallVector<T,0>
This revision is now accepted and ready to land.Wed, Jun 2, 7:03 AM
phosek added a subscriber: phosek.Wed, Jun 2, 6:29 PM

OpenMP/tile_codegen_tile_for.cpp is failing on all our bots, would it be possible to revert the change?

FAIL: Clang :: OpenMP/tile_codegen_tile_for.cpp (9951 of 28034)
******************** TEST 'Clang :: OpenMP/tile_codegen_tile_for.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';   /opt/s/w/ir/x/w/staging/llvm_build/bin/clang -cc1 -internal-isystem /opt/s/w/ir/x/w/staging/llvm_build/lib/clang/13.0.0/include -nostdsysteminc -verify -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=51 -emit-llvm /opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp -o - | /opt/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --allow-unused-prefixes /opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp --check-prefix=IR
: 'RUN: at line 5';   /opt/s/w/ir/x/w/staging/llvm_build/bin/clang -cc1 -internal-isystem /opt/s/w/ir/x/w/staging/llvm_build/lib/clang/13.0.0/include -nostdsysteminc -verify -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=51 -emit-pch -o /opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/OpenMP/Output/tile_codegen_tile_for.cpp.tmp /opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp
: 'RUN: at line 6';   /opt/s/w/ir/x/w/staging/llvm_build/bin/clang -cc1 -internal-isystem /opt/s/w/ir/x/w/staging/llvm_build/lib/clang/13.0.0/include -nostdsysteminc -verify -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=51 -include-pch /opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/OpenMP/Output/tile_codegen_tile_for.cpp.tmp -emit-llvm /opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp -o - | /opt/s/w/ir/x/w/staging/llvm_build/bin/FileCheck --allow-unused-prefixes /opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp --check-prefix=IR
--
Exit Code: 1

Command Output (stderr):
--
/opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp:250:8: error: IR: expected string not found in input
// IR: ![[META1:[0-9]+]] = !{!"clang version {{[^"]*}}"}
       ^
<stdin>:260:36: note: scanning from here
!0 = !{i32 1, !"wchar_size", i32 4}
                                   ^
<stdin>:261:1: note: possible intended match here
!1 = !{!"Fuchsia clang version 13.0.0 (https://llvm.googlesource.com/a/llvm-project 07a6beb402150d25ec7c93a5747520ac2804731d)"}
^

Input file: <stdin>
Check file: /opt/s/w/ir/x/w/llvm-project/clang/test/OpenMP/tile_codegen_tile_for.cpp

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

Input was:
<<<<<<
             .
             .
             .
           255: attributes #2 = { convergent nounwind } 
           256:  
           257: !llvm.module.flags = !{!0} 
           258: !llvm.ident = !{!1} 
           259:  
           260: !0 = !{i32 1, !"wchar_size", i32 4} 
check:250'0                                        X error: no match found
           261: !1 = !{!"Fuchsia clang version 13.0.0 (https://llvm.googlesource.com/a/llvm-project 07a6beb402150d25ec7c93a5747520ac2804731d)"} 
check:250'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:250'1     ?                                                                                                                                possible intended match
           262: !2 = distinct !{!2, !3} 
check:250'0     ~~~~~~~~~~~~~~~~~~~~~~~~
           263: !3 = !{!"llvm.loop.mustprogress"} 
check:250'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           264: !4 = distinct !{!4, !3} 
check:250'0     ~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>

--

********************

Instead of reverting, I fixed the test in rG64e5a3bbdde2.