[PATCH] D143325: [Driver] Add -mllvm= as an alias for -mllvm

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This patch broke the Flang buildbot, e.g. 
https://lab.llvm.org/buildbot/#/builders/172/builds/23305

The tests
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/driver-help.f90
https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/driver-help-hidden.f90

They check the entire output of `-help` and need to be updated for new options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143325/new/

https://reviews.llvm.org/D143325

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124823: [OpenMPIRBuilder] Introduce OMPRegionInfo managing the stack of OpenMP region constructs.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.
Herald added a reviewer: nicolasvasilache.
Herald added a reviewer: dcaballe.
Herald added subscribers: Moerafaat, zero9178.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124823/new/

https://reviews.llvm.org/D124823

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137338: Fix dupe word typos

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Changes in `/polly/` look good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137338/new/

https://reviews.llvm.org/D137338

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136784: [Clang] Improve diagnostic message for loop hint pragma

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/Parse/ParsePragma.cpp:1306
   StringRef Str = PragmaName.getIdentifierInfo()->getName();
   std::string ClangLoopStr = (llvm::Twine("clang loop ") + Str).str();
   return std::string(llvm::StringSwitch(Str)

`ClangLoopStr` is unused now


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136784/new/

https://reviews.llvm.org/D136784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131919: Move googletest to the third-party directory

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Semi-OT: `polly\lib\External` has 3 more third-party libraries. Two of them 
have been heavily modified in-tree, the third has just a custom CMakeLists.txt.
Should these eventually also be moved?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131919/new/

https://reviews.llvm.org/D131919

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131526: [OMPIRBuilder] Add support for safelen clause

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3042
+
+  if (!(Simdlen == nullptr && Safelen == nullptr)) {
+// If both simdlen and safelen clauses are specified, the value of the

Consider simplifying this expression



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3048
+// parameter. Therefore, use safelen only in the absence of simdlen.
+ConstantInt *VectorizeWidth = Simdlen == nullptr ? Safelen : Simdlen;
 addLoopMetadata(

psoni2628 wrote:
> Meinersbur wrote:
> > `safelen` should not mean the same as `llvm.loop.vectorize.width`. 
> > `safelen` could be unreasonably large to use as SIMD width or a 
> > non-power-of-2.
> > 
> > That being said, it's what `CGStmtOpenMP.cpp` does as well and I don't know 
> > any better way.
> IMO, this is more of a semantic analysis problem. For example, if it not 
> legal to have a `safelen` that is a non-power-of-2, then Clang should not let 
> this value proceed from semantic analysis. Maybe we could add a check in 
> `clang/lib/Sema/SemaOpenMP.cpp`, and fix the problem for both OMPIRBuilder 
> and the existing codegen support in `CGStmtOpenMP.cpp` in a different patch?
I think it's not a responsibility of Clang, but the optimizer. For instance, 
the information that "infinite" vector width is good (#pragma clang loop 
vectorize(assume_safety)` and `#pragma omp simd` without `safelen`) is passed 
as metadata (`addSimdMetadata`), but there is not metadata for conveying that 
only a specific vector length is safe.

There are other issues than just power-of-2, such as
 * too-large safelen (e.g. 128 when the target only has simd instructions of 
length 4). 
 * A vector width of 2 having better performance even though `safelen(2)` is 
used.
 * The LoopVectorize will not vectorize at all if it cannot conclude the 
legality of it when only `!{!"llvm.loop.vectorize.width", i32 4}` is specified, 
but not also the information that the user guarantees that it is safe to do so.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131526/new/

https://reviews.llvm.org/D131526

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131526: [OMPIRBuilder] Add support for safelen clause

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3029-3032
+addLoopMetadata(
+CanonicalLoop,
+MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.parallel_accesses"),
+  AccessGroup}));

Instead of calling `addLoopMetadata` repeatedly, consider collecting metadata 
in a `SmallVector` and call `addLoopMetadata` only once at the end.
```
SmallVector MDList;
if (Safelen == nullptr) {
  ... 
  MDList.push_back(MDNode::get(...));
}
...
addLoopMetadata(CanonicalLoop, MDList);
```



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3048
+// parameter. Therefore, use safelen only in the absence of simdlen.
+ConstantInt *VectorizeWidth = Simdlen == nullptr ? Safelen : Simdlen;
 addLoopMetadata(

`safelen` should not mean the same as `llvm.loop.vectorize.width`. `safelen` 
could be unreasonably large to use as SIMD width or a non-power-of-2.

That being said, it's what `CGStmtOpenMP.cpp` does as well and I don't know any 
better way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131526/new/

https://reviews.llvm.org/D131526

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129131: Remove uses of llvm_shutdown

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: polly/lib/External/isl/interface/extract_interface.cc:590
delete Clang;
-   llvm::llvm_shutdown();
 

This file is imported from the upstream project 
(https://repo.or.cz/isl.git/blob/295cf91923295ca694e9e4433a0b818150421bee:/interface/extract_interface.cc#l590)
 and this change will be lost when synchronizing with it. However, this file is 
also not used within LLVM. I recommend to just keep as-is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129131/new/

https://reviews.llvm.org/D129131

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129992: [clang][OpenMP] Add IRBuilder support for taskgroup

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5207-5209
+  if (T.clauses().size() > 0)
+return false;
+  return true;

simplification


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129992/new/

https://reviews.llvm.org/D129992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129992: [clang][OpenMP] Add IRBuilder support for taskgroup

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5209
+  OMPLexicalScope Scope(*this, S, OMPD_unknown);
+  if (CGM.getLangOpts().OpenMPIRBuilder && S.clauses().size() == 0) {
+llvm::OpenMPIRBuilder &OMPBuilder = CGM.getOpenMPRuntime().getOMPBuilder();

Instead of `S.clauses().size() == 0`, could we make it more explicit and 
extendable by adding a function like `isSupportedbyOpenMPIRBuilder(S)`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129992/new/

https://reviews.llvm.org/D129992

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129149/new/

https://reviews.llvm.org/D129149

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Why not have `simdlen` an argument to `applySimd` (which can be null if not 
used)? In this case it happens to be just adding some metadata, but for others 
such as `unrollLoopPartial` it does not make sense to have the unroll factor 
set by a different method. It would also allow better consistency checks if 
passed as a parameter.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2596
   // Check for unsupported clauses
-  if (!S.clauses().empty()) {
-// Currently no clause is supported
-return false;
+  for (OMPClause *C : S.clauses()) {
+// Currently only simdlen clause is supported

psoni2628 wrote:
> shraiysh wrote:
> > psoni2628 wrote:
> > > shraiysh wrote:
> > > > psoni2628 wrote:
> > > > > psoni2628 wrote:
> > > > > > arnamoy10 wrote:
> > > > > > > I am just wondering whether we should have a check to make sure 
> > > > > > > that we are processing the clauses of only `simd` directive here. 
> > > > > > > Because the function takes a general `OMPExecutableDirective` as 
> > > > > > > argument 
> > > > > > That's a fair point. I guess `isSupportedByOpenMPIRBuilder` could 
> > > > > > be used for other directive types other than simd, even though it's 
> > > > > > not right now.
> > > > > Would it make more sense to only guard the checking of clauses with a 
> > > > > check for `OMPSimdDirective`, or the whole thing? I believe even the 
> > > > > code below, which checks for an ordered directive, is also 
> > > > > specifically for `simd`?
> > > > > 
> > > > > 
> > > > > Example of guarding the whole thing:
> > > > > 
> > > > > ```
> > > > >   if(dyn_cast(S)) {
> > > > > // Check for unsupported clauses
> > > > > for (OMPClause *C : S.clauses()) {
> > > > >   // Currently only simdlen clause is supported
> > > > >   if (dyn_cast(C))
> > > > > continue;
> > > > >   else
> > > > > return false;
> > > > > }
> > > > > 
> > > > > // Check if we have a statement with the ordered directive.
> > > > > // Visit the statement hierarchy to find a compound statement
> > > > > // with a ordered directive in it.
> > > > > if (const auto *CanonLoop = 
> > > > > dyn_cast(S.getRawStmt())) {
> > > > >   if (const Stmt *SyntacticalLoop = CanonLoop->getLoopStmt()) {
> > > > > for (const Stmt *SubStmt : SyntacticalLoop->children()) {
> > > > >   if (!SubStmt)
> > > > > continue;
> > > > >   if (const CompoundStmt *CS = 
> > > > > dyn_cast(SubStmt)) {
> > > > > for (const Stmt *CSSubStmt : CS->children()) {
> > > > >   if (!CSSubStmt)
> > > > > continue;
> > > > >   if (isa(CSSubStmt)) {
> > > > > return false;
> > > > >   }
> > > > > }
> > > > >   }
> > > > > }
> > > > >   }
> > > > > }
> > > > >   }
> > > > > ```
> > > > Can we instead have separate `isSupportedByOpenMPIRBuilder` for every 
> > > > directive to avoid bloating the function with checks and if conditions?
> > > > ```
> > > > static bool isSupportedByOpenMPIRBuilder(const OMPSimdDirective &S) 
> > > > {...}
> > > > void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) 
> > > > {...}
> > > > 
> > > > static bool isSupportedByOpenMPIRBuilder(const OMPOrderedDirective &S) 
> > > > {...}
> > > > void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective 
> > > > &S) {...}
> > > > 
> > > > static bool isSupportedByOpenMPIRBuilder(const OMPTaskDirective &S) 
> > > > {...}
> > > > void CodeGenFunction::EmitOMPOrderedDirective(const OMPTaskDirective 
> > > > &S) {...}
> > > > ```
> > > It was decided in D114379 to use `OMPExecutableDirective` in order to 
> > > allow this function to be reused for constructs such as `for simd`. Do 
> > > you wish to undo this now, and instead specialize the function?
> > I wasn't aware of the earlier discussion about this. However it seems like 
> > it was suggested to try either splitting the function or merging it with 
> > slight preference for merging (I didn't understand how that would help 
> > constructs like `for simd` because I haven't worked with `for simd` much 
> > and will take others' word for it).
> > 
> > The suggested code change in previous reply - checking of clauses with a 
> > check for OMPSimdDirective - this seems like it would eventually become a 
> > huge function because of such checks. 
> > 
> > @Meinersbur it would be great if you could please advise on this - if 
> > keeping the checks for all executable directives in the same function will 
> > be helpful in the long run - allowing us to reuse checks or should we split 
> > it? A third alternative would be to have both functions, one specialized 
> > for SIMD and one for ExecutableConstruct, where the latter might handle 
> > combined constructs like `for simd` etc. Would that arrangement work?
> I agree with you that having a general functio

[PATCH] D126291: [flang][Driver] Update link job on windows

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I looked into Google Benchmark because it also has its main in a .lib library 
and how it handles it. Turns out it is using cmake which by default always adds 
`/subsystem:console` unless setting 
 WIN32_EXECUTABLE 
=ON.

I tested the updated patch again it is failing:

  $ ":" "RUN: at line 16"
  $ "c:\users\meinersbur\build\llvm-project\release\bin\flang-new.exe" "-###" 
"-flang-experimental-exec" "--ld-path=/usr/bin/ld" 
"C:\Users\meinersbur\src\llvm-project\flang\test\Driver/Inputs/hello.f90"
  $ "c:\users\meinersbur\build\llvm-project\release\bin\filecheck.exe" 
"C:\Users\meinersbur\src\llvm-project\flang\test\Driver\linker-flags.f90"
  # command stderr:
  
C:\Users\meinersbur\src\llvm-project\flang\test\Driver\linker-flags.f90:26:16: 
error: CHECK-LABEL: expected string not found in input
  ! CHECK-LABEL: "/usr/bin/ld"
 ^
  :7:136: note: scanning from here
   "c:\\users\\meinersbur\\build\\llvm-project\\release\\bin\\flang-new" "-fc1" 
"-triple" "x86_64-pc-windows-msvc19.32.31329" "-emit-obj" "-o" 
"C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\lit-tmp-0rwi220l\\hello-bbdcc9.o" 
"C:\\Users\\meinersbur\\src\\llvm-project\\flang\\test\\Driver/Inputs/hello.f90"

Presumably the feature 'windows-msvc' is not registered. 
`print(config.available_features)` results in:

  {'asserts', 'x86-registered-target', 'nvptx-registered-target', 
'system-windows'}



In D126291#3555639 , @awarzynski 
wrote:

> In D126291#391 , @mmuetzel 
> wrote:
>
>> ISTR, I somewhere read that Windows isn't a supported host currently. Is 
>> this no longer the case?)
>
> Windows is supported: https://lab.llvm.org/buildbot/#/builders/172 :)

The bot just compiles flang and runs the unit-test, none of which actually try 
to compile, link & run a Fortan program. Until recently the flang driver would 
not compile itself, but delegate it to gfortran anyway.




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:140
+// defined in flang's runtime libraries.
+if (TC.getTriple().isKnownWindowsMSVCEnvironment())
+  CmdArgs.push_back("/subsystem:console");

mmuetzel wrote:
> awarzynski wrote:
> > mmuetzel wrote:
> > > Is the MSVC toolchain used anywhere else but on Windows?
> > No: 
> > https://github.com/llvm/llvm-project/blob/5fee1799f4d8da59c251e2d04172fc2f387cbe54/llvm/include/llvm/ADT/Triple.h#L576-L578
> >  ;-) 
> In that case, this argument could probably be added unconditionally.
How do you mean this? `/subsystem:console` is a `link.exe` flag, also supported 
by `lld-link.exe` sind it tries to be an msvc-compatible wrapper. It is not a 
valid flag for eg GNU ld even in msys.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126291/new/

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D126291#3552573 , @rovka 
wrote://italic text//

> I don't really know why `link.exe` doesn't work. According to the docs 
> ,
>  it should find `main` and choose the subsystem on its own. The only hunch I 
> have is that maybe it doesn't work because `main` is in a library as opposed 
> to one of the object files, but I'm still going through the docs trying to 
> figure this out. I don't have a lot of experience with `link.exe` so I might 
> be missing something obvious.

It does work when passing `/SUBSYSTEM:CONSOLE` explicitly. Indeed maybe the 
autoselect only looks into symbols found in object file. Maybe pass 
`/SUBSYSTEM:CONSOLE` as well to the linker command? `llvm-link` should accept 
it as well. AFAIU there is no other subsystem Flang supports anyway. Even if, 
it requires changes to `Fortran_main.lib`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126291/new/

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

While the test passes now, I still get `LINK : fatal error LNK1561: entry point 
must be defined` when trying to actually link. Isn't it expected to not work 
yet?

Linking with `lld-link.exe` does work. objdump says there is a `_QQmain` 
symbol, why does lld consider this to be the entry point?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126291/new/

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126291: [flang][Driver] Update link job on windows

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: flang/test/Driver/flang-linker-flags-windows.f90:16
+! Linker invocation to generate the executable
+! CHECK-LABEL:  lld-link.exe
+! CHECK-NOT: libcmt

This is failing for me. Instead of `lld-link.exe`, it is using `link.exe` (the 
Microsoft linker).

Without the `-###` flag, the link command is failing:
```
flang-new version 15.0.0 (C:/Users/meinersbur/src/llvm-project/clang 
09fdf5f6f5e70ecb7d95cdbb98442c998a55ce23)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: c:\users\meinersbur\build\llvm-project\release\bin
 "c:\\users\\meinersbur\\build\\llvm-project\\release\\bin\\flang-new" -fc1 
-triple x86_64-pc-windows-msvc19.32.31329 -emit-obj -o 
"C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\hello-ae4145.o" 
"C:\\Users\\meinersbur\\src\\llvm-project\\flang\\test\\Driver/Inputs/hello.f90"
 "C:\\Program Files\\Microsoft Visual 
Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.32.31326\\bin\\Hostx64\\x64\\link.exe"
 -out:a.exe "-libpath:c:\\users\\meinersbur\\build\\llvm-project\\release\\lib" 
Fortran_main.lib FortranRuntime.lib FortranDecimal.lib -nologo 
"C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\hello-ae4145.o"
LINK : fatal error LNK1561: entry point must be defined
flang-new: error: linker command failed with exit code 1561 (use -v to see 
invocation)
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126291/new/

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124606: Use `-text` git attribute instead of `text eol=...'

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D124606#3479793 , @ilya-biryukov 
wrote:

> LGTM

This and the commit (@MForster ) was too hasty. There should have been time for 
people discussing D124563  and D97625 
 to react, and should have been added as 
reviewers. I didn't even see @ilya-biryukov participating in the other 
discussions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124606/new/

https://reviews.llvm.org/D124606

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D124563#3478627 , @smeenai wrote:

> If I check out this commit and then check out the previous commit, 
> `clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp` and 
> `clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected`
>  become modified in my working directory; their line endings are changed from 
> CRLF to LF. That seems undesirable.

In the current top-of-tree, the gitattributes line endings and the actual line 
endings contradict each other, leading to such inconsistencies.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D124563#3478625 , @smeenai wrote:

> I *think* this would mean that if you're on Windows and have `core.autocrlf` 
> set to `input`, when you commit changes to this files, Git will convert them 
> back to LF line endings. Not 100% sure of that though.

As far as I recall this was done because there are many ways how the line 
endings can be accidentally changed (`core.autocrlf` being one of then, others 
are `clang-format` or text editors that do not retain line endings) and 
committed. Removing the files from `.gitignore` undo the intended line ending 
pin from D97625 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Regarding `crlf.cpp`(`.expected`), adding the modified changes to a commit 
indeed solved the problem I described in 
https://reviews.llvm.org/rGac5f7be6a8688955a282becf00eebc542238a86b#1080897 and 
I was working with that so far. Still don't know why Linux git doesn't do the 
same?

Don't know about the other files, but according to 
https://github.com/git/git/commit/af6e0fe3a589d58bfd508c1c6ccbeb38586eb06b, 
this seems the right thing to do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D118409#3474131 , 
@kiranchandramohan wrote:

> I think the CI is complaining about some clang-format issue in  
> clang/test/OpenMP/critical_codegen_attr.cpp

clang-format should not be applied on Clang tests, many of them have 
significant whitespace (eg. the preprocessor tests). In this case, all that 
clang-format suggests is

  diff --git a/clang/test/OpenMP/critical_codegen.cpp 
b/clang/test/OpenMP/critical_codegen.cpp
  index 41454b6dd1b4..d472325ff8a2 100644
  --- a/clang/test/OpenMP/critical_codegen.cpp
  +++ b/clang/test/OpenMP/critical_codegen.cpp
  @@ -22,7 +22,10 @@
  -void foo() { extern void mayThrow(); mayThrow(); }
  +void foo() {
  +  extern void mayThrow();
  +  mayThrow();
  +}

These lines were not changed in this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118409/new/

https://reviews.llvm.org/D118409

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5571-5579
+  {
+OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP,
+   *FiniBB);
+EmitStmt(CS->getCapturedStmt());
+  }
+
+  if (Builder.saveIP().isSet())

kiranchandramohan wrote:
> Why is it not possible to use `OMPBuilderCBHelpers::EmitOMPInlinedRegionBody` 
> here?
`EmitOMPInlinedRegionBody`  calls `splitBBWithSuffix`, which has already been 
done in this lambda.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:26-58
+/// Move the instruction after an InsertPoint to the beginning of another
+/// BasicBlock.
+///
+/// The instructions after \p IP are moved to the beginning of \p New which 
must
+/// not have any PHINodes. If \p CreateBranch is true, a branch instruction to
+/// \p New will be added such that there is no semantic change. Otherwise, the
+/// \p IP insert block remains degenerate and it is up to the caller to insert 
a

kiranchandramohan wrote:
> I guess these are already in from your other patch 
> (https://reviews.llvm.org/D114413).
Yes, now public because also used in CodegenOpenMP.

These utility functions avoid all the workarounds that 
`splitBasicBlock`/`SplitBlock` do not all degenerate blocks, such as inserting 
temporary terminators or ContinuationBB (There may be insertion points 
referencing those temporary terminators!!).



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:103
+  llvm::BasicBlock *continuationBlock =
+  splitBB(builder, true, "omp.region.cont");
+  llvm::BasicBlock *sourceBlock = builder.GetInsertBlock();

kiranchandramohan wrote:
> Would this "omp.region.cont" be a mostly empty block in most cases? I guess 
> it is not a problem since llvm will remove these blocks.
> 
> The IR generated for 
> ```
>   omp.parallel {
> omp.barrier
> omp.terminator
>   }
> ```
> is the following. Notice the empty (only a branch) `omp.region.cont` block. 
> Previously we had this only at the source side `omp.par.region`.
> 
> ```
> omp.par.entry:
>   %tid.addr.local = alloca i32, align 4
>   %0 = load i32, i32* %tid.addr, align 4
>   store i32 %0, i32* %tid.addr.local, align 4
>   %tid = load i32, i32* %tid.addr.local, align 4
>   br label %omp.par.region
> 
> omp.par.region:   ; preds = %omp.par.entry
>   br label %omp.par.region1
> 
> omp.par.region1:  ; preds = %omp.par.region
>   %omp_global_thread_num2 = call i32 
> @__kmpc_global_thread_num(%struct.ident_t* @4)
>   call void @__kmpc_barrier(%struct.ident_t* @3, i32 %omp_global_thread_num2)
>   br label %omp.region.cont, !dbg !12
> 
> omp.region.cont:  ; preds = %omp.par.region1
>   br label %omp.par.pre_finalize
> 
> omp.par.pre_finalize: ; preds = %omp.region.cont
>   br label %omp.par.outlined.exit.exitStub
> ```
Empty BBs should not be considered an issue since they are removed by 
SimplifyCFG anyway. Why would it be? Correctness should be the priority. 

What makes the current code so fragile are the conditions that depend on the 
current insert point. Invoking it in a different manner causes an untested code 
path to be taken. For instance, an insert point becomes invalided that did not 
under any previous test because nobody yet inserted new BBs in a callback. One 
if condition inside the callback and everything breaks.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118409/new/

https://reviews.llvm.org/D118409

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118409/new/

https://reviews.llvm.org/D118409

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

[Michael Kruse via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ec501da76fc: [OpenMP] Refactor OMPScheduleType enum. 
(authored by Meinersbur).

Changed prior to commit:
  https://reviews.llvm.org/D123403?vs=422862&id=423442#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123403/new/

https://reviews.llvm.org/D123403

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/irbuilder_for_unsigned_auto.c
  clang/test/OpenMP/irbuilder_for_unsigned_dynamic.c
  clang/test/OpenMP/irbuilder_for_unsigned_dynamic_chunked.c
  clang/test/OpenMP/irbuilder_for_unsigned_runtime.c
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/test/Target/LLVMIR/openmp-llvm.mlir

Index: mlir/test/Target/LLVMIR/openmp-llvm.mlir
===
--- mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -657,7 +657,7 @@
 llvm.func @test_omp_wsloop_runtime_simd(%lb : i64, %ub : i64, %step : i64) -> () {
   omp.wsloop schedule(runtime, simd)
   for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 47
+// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741871
 // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
 // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
 // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
@@ -674,7 +674,7 @@
 llvm.func @test_omp_wsloop_guided_simd(%lb : i64, %ub : i64, %step : i64) -> () {
   omp.wsloop schedule(guided, simd)
   for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 46
+// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741870
 // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
 // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
 // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
@@ -788,7 +788,7 @@
 llvm.func @test_omp_wsloop_dynamic_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(dynamic) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741891, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 67, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
@@ -806,7 +806,7 @@
 llvm.func @test_omp_wsloop_auto_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(auto) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741894, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 70, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
@@ -824,7 +824,7 @@
 llvm.func @test_omp_wsloop_runtime_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(runtime) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741893, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 69, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
@@ -842,7 +842,7 @@
 llvm.func @test_omp_wsloop_guided_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(guided) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741892, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 68, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icm

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 422862.
Meinersbur added a comment.

- address review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123403/new/

https://reviews.llvm.org/D123403

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/irbuilder_for_unsigned_auto.c
  clang/test/OpenMP/irbuilder_for_unsigned_dynamic.c
  clang/test/OpenMP/irbuilder_for_unsigned_dynamic_chunked.c
  clang/test/OpenMP/irbuilder_for_unsigned_runtime.c
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/test/Target/LLVMIR/openmp-llvm.mlir

Index: mlir/test/Target/LLVMIR/openmp-llvm.mlir
===
--- mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -657,7 +657,7 @@
 llvm.func @test_omp_wsloop_runtime_simd(%lb : i64, %ub : i64, %step : i64) -> () {
   omp.wsloop schedule(runtime, simd)
   for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 47
+// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741871
 // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
 // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
 // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
@@ -674,7 +674,7 @@
 llvm.func @test_omp_wsloop_guided_simd(%lb : i64, %ub : i64, %step : i64) -> () {
   omp.wsloop schedule(guided, simd)
   for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 46
+// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741870
 // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
 // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
 // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
@@ -788,7 +788,7 @@
 llvm.func @test_omp_wsloop_dynamic_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(dynamic) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741891, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 67, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
@@ -806,7 +806,7 @@
 llvm.func @test_omp_wsloop_auto_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(auto) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741894, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 70, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
@@ -824,7 +824,7 @@
 llvm.func @test_omp_wsloop_runtime_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(runtime) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741893, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 69, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
@@ -842,7 +842,7 @@
 llvm.func @test_omp_wsloop_guided_ordered(%lb : i64, %ub : i64, %step : i64) -> () {
  omp.wsloop schedule(guided) ordered(0)
  for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741892, i64 1, i64 %{{.*}}, i64 1, i64 1)
+  // CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 68, i64 1, i64 %{{.*}}, i64 1, i64 1)
   // CHECK: call void @__kmpc_dispatch_fini_8u
   // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
   // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPTo

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked 7 inline comments as done.
Meinersbur added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:135
+  UnorderedGuidedSimd = BaseGuidedSimd | ModifierUnordered,   // (46)
+  UnorderedRuntimeSimd = BaseRuntimeSimd | ModifierUnordered, // (47)
+

peixin wrote:
> Why not using the following to be consistent with the name in kmp.h?
> StaticBalancedChunked 
> GuidedSimd
> RuntimeSimd
As mentioned in the summary. to avoid confusion by not using the original name. 
`StaticBalancedChunked` could mean either the algorithm to use (now 
`BaseStaticBalancedChunked`, as in `omp_sched_t`/`enum kmp_sched`), or that 
algorithm with the unordered flag set (now `UnorderedStaticBalancedChunked `). 
I would the former because that's how the enum is structured.

The name in `kmp.h` for it is actually `kmp_sch_static_balanced_chunked`. `sch` 
for "unordered"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123403/new/

https://reviews.llvm.org/D123403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: peixin, tianshilei1992, jdoerfert, sriharikrishna, 
Leporacanthicus, kiranchandramohan, clementval.
Meinersbur added a project: OpenMP.
Herald added subscribers: awarzynski, sdasgup3, wenzhicui, wrengr, 
Chia-hungDuan, ormris, dcaballe, cota, teijeong, rdzhabarov, tatianashp, 
msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, 
arpith-jacob, antiagainst, shauheen, rriddle, mehdi_amini, guansong, hiraditya, 
yaxunl.
Herald added a reviewer: ftynse.
Herald added a project: All.
Meinersbur requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, 
stephenneuendorffer, nicolasvasilache.
Herald added projects: clang, MLIR, LLVM.

The OMPScheduleType enum stores the constants from libomp's internal sched_type 
in kmp.h and are used by several kmp API functions. The enum values have an 
internal structure, namely each scheduling algorithm (e.g.) exists in four 
variants: unordered, orderend, normerge unordered, and nomerge ordered.

This patch (basically a followup to D114940 ) 
splits the "ordered" and "nomerge" bits into separate flags, as was already 
done for the "monotonic" and "nonmonotonic", so we can apply bit flags 
operations on them. It also now contains all possible combinations according to 
kmp's sched_type. Deriving of the OMPScheduleType enum from clause parameters 
has been moved form MLIR's OpenMPToLLVMIRTranslation.cpp to OpenMPIRBuilder to 
make available for clang as well. Since the primary purpose of the flag is the 
binary interface to libomp, it has been made more private to LLVMFrontend. The 
primary interface for generating worksharing-loop using OpenMPIRBuilder code 
becomes `applyWorkshareLoop` which derives the OMPScheduleType automatically 
and calls the appropriate emitter function.

While this is mostly a NFC refactor, it still applies the following functional 
changes:

- The logic from OpenMPToLLVMIRTranslation to derive the OMPScheduleType also 
applies to clang. Most notably, it now applies the nonmonotonic flag for 
non-static schedules by default.
- In OpenMPToLLVMIRTranslation, the nonmonotonic default flag was previously 
not applied if the simd modifier was used. I assume this was a bug, since the 
effect was due to `loop.schedule_modifier()` returning 
`mlir::omp::ScheduleModifier::none` instead of `llvm::Optional::None`.
- In OpenMPToLLVMIRTranslation, the nonmonotonic default flag was set even if 
ordered was specified, in breach to what the comment before citing the OpenMP 
specification says. I assume this was an oversight.

The ordered flag with parameter was not considered in this patch. Changes will 
need to be made (e.g. adding/modifying function parameters) when support for it 
is added. The lengthy names of the enum values can be discussed, for the moment 
this is avoiding reusing previously existing enum value names such as 
`StaticChunked` to avoid confusion.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123403

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/irbuilder_for_unsigned_auto.c
  clang/test/OpenMP/irbuilder_for_unsigned_dynamic.c
  clang/test/OpenMP/irbuilder_for_unsigned_dynamic_chunked.c
  clang/test/OpenMP/irbuilder_for_unsigned_runtime.c
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/test/Target/LLVMIR/openmp-llvm.mlir

Index: mlir/test/Target/LLVMIR/openmp-llvm.mlir
===
--- mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -657,7 +657,7 @@
 llvm.func @test_omp_wsloop_runtime_simd(%lb : i64, %ub : i64, %step : i64) -> () {
   omp.wsloop schedule(runtime, simd)
   for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 47
+// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741871
 // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
 // CHECK: %[[cond:.*]] = icmp ne i32 %[[continue]], 0
 // CHECK  br i1 %[[cond]], label %omp_loop.header{{.*}}, label %omp_loop.exit{{.*}}
@@ -674,7 +674,7 @@
 llvm.func @test_omp_wsloop_guided_simd(%lb : i64, %ub : i64, %step : i64) -> () {
   omp.wsloop schedule(guided, simd)
   for (%iv) : i64 = (%lb) to (%ub) step (%step) {
-// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 46
+// CHECK: call void @__kmpc_dispatch_init_8u(%struct.ident_t* @{{.*}}, i32 %{{.*}}, i32 1073741870
 // CHECK: %[[continue:.*]] = call i32 @__kmpc_dispatch_next_8u
 // CHECK: %[[con

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

[Michael Kruse via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc082ca16f123: [OpenMPIRBuilder] Detect and fix ambiguous 
InsertPoints for createSections. (authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117835/new/

https://reviews.llvm.org/D117835

Files:
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/test/Target/LLVMIR/openmp-llvm.mlir

Index: mlir/test/Target/LLVMIR/openmp-llvm.mlir
===
--- mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -1854,6 +1854,9 @@
 
 // CHECK-LABEL: @omp_sections_trivial
 llvm.func @omp_sections_trivial() -> () {
+  // CHECK:   br label %[[ENTRY:[a-zA-Z_.]+]]
+
+  // CHECK: [[ENTRY]]:
   // CHECK:   br label %[[PREHEADER:.*]]
 
   // CHECK: [[PREHEADER]]:
Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -72,6 +72,21 @@
 return allocaInsertPoint;
 
   // Otherwise, insert to the entry block of the surrounding function.
+  // If the current IRBuilder InsertPoint is the function's entry, it cannot
+  // also be used for alloca insertion which would result in insertion order
+  // confusion. Create a new BasicBlock for the Builder and use the entry block
+  // for the allocs.
+  if (builder.GetInsertBlock() ==
+  &builder.GetInsertBlock()->getParent()->getEntryBlock()) {
+assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() &&
+   "Assuming end of basic block");
+llvm::BasicBlock *entryBB = llvm::BasicBlock::Create(
+builder.getContext(), "entry", builder.GetInsertBlock()->getParent(),
+builder.GetInsertBlock()->getNextNode());
+builder.CreateBr(entryBB);
+builder.SetInsertPoint(entryBB);
+  }
+
   llvm::BasicBlock &funcEntryBlock =
   builder.GetInsertBlock()->getParent()->getEntryBlock();
   return llvm::OpenMPIRBuilder::InsertPointTy(
@@ -255,23 +270,12 @@
   // TODO: Is the Parallel construct cancellable?
   bool isCancellable = false;
 
-  // Ensure that the BasicBlock for the the parallel region is sparate from the
-  // function entry which we may need to insert allocas.
-  if (builder.GetInsertBlock() ==
-  &builder.GetInsertBlock()->getParent()->getEntryBlock()) {
-assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() &&
-   "Assuming end of basic block");
-llvm::BasicBlock *entryBB =
-llvm::BasicBlock::Create(builder.getContext(), "parallel.entry",
- builder.GetInsertBlock()->getParent(),
- builder.GetInsertBlock()->getNextNode());
-builder.CreateBr(entryBB);
-builder.SetInsertPoint(entryBB);
-  }
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createParallel(
-  ompLoc, findAllocaInsertPoint(builder, moduleTranslation), bodyGenCB,
-  privCB, finiCB, ifCond, numThreads, pbKind, isCancellable));
+  ompLoc, allocaIP, bodyGenCB, privCB, finiCB, ifCond, numThreads, pbKind,
+  isCancellable));
 
   return bodyGenStatus;
 }
@@ -522,7 +526,6 @@
   SmallVector vecValues =
   moduleTranslation.lookupValues(orderedOp.depend_vec_vars());
 
-  llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   size_t indexVecValues = 0;
   while (indexVecValues < vecValues.size()) {
 SmallVector storeValues;
@@ -531,9 +534,11 @@
   storeValues.push_back(vecValues[indexVecValues]);
   indexVecValues++;
 }
+llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+findAllocaInsertPoint(builder, moduleTranslation);
+llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
 builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createOrderedDepend(
-ompLoc, findAllocaInsertPoint(builder, moduleTranslation), numLoops,
-storeValues, ".cnt.addr", isDependSource));
+ompLoc, allocaIP, numLoops, storeValues, ".cnt.addr", isDependSource));
   }
   return success();
 }
@@ -634,10 +639,12 @@
   // called for variables which have destructors/finalizers.
   auto finiCB = [&](InsertPointTy codeGenIP) {};
 
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createSections(
-  ompLo

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 420371.
Meinersbur marked an inline comment as done.
Meinersbur added a comment.

- Rebase
- Fix convertOmpAtomicUpdate, convertOmpAtomicCapture, and convertOmpOrdered


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117835/new/

https://reviews.llvm.org/D117835

Files:
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/test/Target/LLVMIR/openmp-llvm.mlir

Index: mlir/test/Target/LLVMIR/openmp-llvm.mlir
===
--- mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -1854,6 +1854,9 @@
 
 // CHECK-LABEL: @omp_sections_trivial
 llvm.func @omp_sections_trivial() -> () {
+  // CHECK:   br label %[[ENTRY:[a-zA-Z_.]+]]
+
+  // CHECK: [[ENTRY]]:
   // CHECK:   br label %[[PREHEADER:.*]]
 
   // CHECK: [[PREHEADER]]:
Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -72,6 +72,21 @@
 return allocaInsertPoint;
 
   // Otherwise, insert to the entry block of the surrounding function.
+  // If the current IRBuilder InsertPoint is the function's entry, it cannot
+  // also be used for alloca insertion which would result in insertion order
+  // confusion. Create a new BasicBlock for the Builder and use the entry block
+  // for the allocs.
+  if (builder.GetInsertBlock() ==
+  &builder.GetInsertBlock()->getParent()->getEntryBlock()) {
+assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() &&
+   "Assuming end of basic block");
+llvm::BasicBlock *entryBB = llvm::BasicBlock::Create(
+builder.getContext(), "entry", builder.GetInsertBlock()->getParent(),
+builder.GetInsertBlock()->getNextNode());
+builder.CreateBr(entryBB);
+builder.SetInsertPoint(entryBB);
+  }
+
   llvm::BasicBlock &funcEntryBlock =
   builder.GetInsertBlock()->getParent()->getEntryBlock();
   return llvm::OpenMPIRBuilder::InsertPointTy(
@@ -255,23 +270,12 @@
   // TODO: Is the Parallel construct cancellable?
   bool isCancellable = false;
 
-  // Ensure that the BasicBlock for the the parallel region is sparate from the
-  // function entry which we may need to insert allocas.
-  if (builder.GetInsertBlock() ==
-  &builder.GetInsertBlock()->getParent()->getEntryBlock()) {
-assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() &&
-   "Assuming end of basic block");
-llvm::BasicBlock *entryBB =
-llvm::BasicBlock::Create(builder.getContext(), "parallel.entry",
- builder.GetInsertBlock()->getParent(),
- builder.GetInsertBlock()->getNextNode());
-builder.CreateBr(entryBB);
-builder.SetInsertPoint(entryBB);
-  }
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createParallel(
-  ompLoc, findAllocaInsertPoint(builder, moduleTranslation), bodyGenCB,
-  privCB, finiCB, ifCond, numThreads, pbKind, isCancellable));
+  ompLoc, allocaIP, bodyGenCB, privCB, finiCB, ifCond, numThreads, pbKind,
+  isCancellable));
 
   return bodyGenStatus;
 }
@@ -522,7 +526,6 @@
   SmallVector vecValues =
   moduleTranslation.lookupValues(orderedOp.depend_vec_vars());
 
-  llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   size_t indexVecValues = 0;
   while (indexVecValues < vecValues.size()) {
 SmallVector storeValues;
@@ -531,9 +534,11 @@
   storeValues.push_back(vecValues[indexVecValues]);
   indexVecValues++;
 }
+llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+findAllocaInsertPoint(builder, moduleTranslation);
+llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
 builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createOrderedDepend(
-ompLoc, findAllocaInsertPoint(builder, moduleTranslation), numLoops,
-storeValues, ".cnt.addr", isDependSource));
+ompLoc, allocaIP, numLoops, storeValues, ".cnt.addr", isDependSource));
   }
   return success();
 }
@@ -634,10 +639,12 @@
   // called for variables which have destructors/finalizers.
   auto finiCB = [&](InsertPointTy codeGenIP) {};
 
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createSections(
-  ompLoc, findAllocaInsertPoint(builder, moduleTranslation), se

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked an inline comment as done.
Meinersbur added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:74-75
+  // for the allocs.
+  // TODO: Create a dedicated alloca BasicBlock at function creation such that
+  // we do not need to move the current InertPoint here.
+  if (builder.GetInsertBlock() ==

shraiysh wrote:
> [Suggestion] We probably don't need to do this, because if a conversion does 
> not require an alloca (for example, barrier), we will be creating a 
> basicblock unnecessarily. So, creating this on-demand seems okay to me.
Making decision based on the current position of an IRBuilder creates heaps of 
problems and unpredictability. Special cases like this one here need to be 
tested and maintained, and still easily introduce bugs. An unconditional 
dedicated alloca block avoids all these problems.

An extra basic block on the other side is insignificant. It will be gone as 
soon as simplify-cfg runs.

However, this seems to be controversial, so I removed the TODO for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117835/new/

https://reviews.llvm.org/D117835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.
Herald added a project: All.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118409/new/

https://reviews.llvm.org/D118409

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.
Herald added a project: All.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117835/new/

https://reviews.llvm.org/D117835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33774: [CodeGen] Make __attribute__(const) calls speculatable

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.
Herald added a project: All.

`speculable` implies no undefined behavior (so it can even be speculatively 
executed with arguments when the source code would not, but its result 
eventually discarded), but `const` does not.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D33774/new/

https://reviews.llvm.org/D33774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);

peixin wrote:
> Nit: integer -> 64-bit integer?
not necessarily, we do not require a specific integer size. For instance, 
`__kmpc_for_static_init_4u` takes a 32-bit integer. It is up to the applyXYZ 
function to zext/trunc it when necessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This patch also caused https://github.com/llvm/llvm-project/issues/54082. I 
noticed due to the revert making the openmp-offload-cuda-project 
 bot green again 
(https://lab.llvm.org/staging/#/builders/155/builds/2482)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118944: [OpenMP] Add Cuda path to linker wrapper tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Thanks for the fix




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8151-8152
  const char *LinkingOutput) const {
+  const auto &D = getToolChain().getDriver();
+  const auto TheTriple = getToolChain().getTriple();
+  auto OpenMPTCRange = C.getOffloadToolChains();

[style] Avoid "Almost Always Auto"



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8161-8162
+  if (CudaInstallation.isValid())
+CmdArgs.push_back(Args.MakeArgString(
+"-cuda-path=" + CudaInstallation.getInstallPath()));
+}

Since there is no `break`, would this potentially add multiple `--cuda-path`?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8183
   const ToolChain *TC = I.second;
-  const Driver &D = TC->getDriver();
+  const Driver &TCDriver = TC->getDriver();
   const ArgList &TCArgs = C.getArgsForToolChain(TC, "", 
Action::OFK_OpenMP);

[nit] unrelated rename



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8258-8261
+  // Construct the link job so we can wrap around it.
+  Linker->ConstructJob(C, JA, Output, Inputs, Args, LinkingOutput);
+  const auto &LinkCommand = C.getJobs().getJobs().back();
+

Is moving this relevant?



Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:102-103
 PtxasArgs("ptxas-args", cl::ZeroOrMore,
-cl::desc("Argument to pass to the ptxas invocation"),
-cl::cat(ClangLinkerWrapperCategory));
 

[nit] unrelated whitespace change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118944/new/

https://reviews.llvm.org/D118944

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118542: [Clang][OpenMPIRBuilder] Fix off-by-one error when dividing by stepsize.

[Michael Kruse via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8a9e4f245b66: [Clang][OpenMPIRBuilder] Fix off-by-one error 
when dividing by stepsize. (authored by Meinersbur).

Changed prior to commit:
  https://reviews.llvm.org/D118542?vs=404300&id=404755#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118542/new/

https://reviews.llvm.org/D118542

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/irbuilder_for_unsigned.c
  clang/test/OpenMP/irbuilder_for_unsigned_down.c
  clang/test/OpenMP/irbuilder_unroll_full.c
  clang/test/OpenMP/irbuilder_unroll_heuristic.c
  clang/test/OpenMP/irbuilder_unroll_partial_factor.c
  clang/test/OpenMP/irbuilder_unroll_partial_factor_for.c
  clang/test/OpenMP/irbuilder_unroll_partial_heuristic.c
  clang/test/OpenMP/irbuilder_unroll_partial_heuristic_constant_for.c
  clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
  clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
  clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c

Index: clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c
===
--- clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c
+++ clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c
@@ -154,7 +154,10 @@
 // CHECK-NEXT:%[[TMP7:.+]] = load i32, i32* %[[DOTSTART]], align 4
 // CHECK-NEXT:%[[SUB:.+]] = sub nsw i32 %[[TMP6]], %[[TMP7]]
 // CHECK-NEXT:%[[TMP8:.+]] = load i32, i32* %[[DOTSTEP]], align 4
-// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[SUB]], %[[TMP8]]
+// CHECK-NEXT:%[[SUB1:.+]] = sub i32 %[[TMP8]], 1
+// CHECK-NEXT:%[[ADD:.+]] = add i32 %[[SUB]], %[[SUB1]]
+// CHECK-NEXT:%[[TMP9:.+]] = load i32, i32* %[[DOTSTEP]], align 4
+// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[ADD]], %[[TMP9]]
 // CHECK-NEXT:br label %[[COND_END:.+]]
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_FALSE]]:
@@ -162,8 +165,8 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_END]]:
 // CHECK-NEXT:%[[COND:.+]] = phi i32 [ %[[DIV]], %[[COND_TRUE]] ], [ 0, %[[COND_FALSE]] ]
-// CHECK-NEXT:%[[TMP9:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
-// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP9]], align 4
+// CHECK-NEXT:%[[TMP10:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
+// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP10]], align 4
 // CHECK-NEXT:ret void
 // CHECK-NEXT:  }
 
Index: clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
===
--- clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
+++ clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
@@ -173,7 +173,10 @@
 // CHECK-NEXT:%[[TMP7:.+]] = load i32, i32* %[[DOTSTART]], align 4
 // CHECK-NEXT:%[[SUB:.+]] = sub nsw i32 %[[TMP6]], %[[TMP7]]
 // CHECK-NEXT:%[[TMP8:.+]] = load i32, i32* %[[DOTSTEP]], align 4
-// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[SUB]], %[[TMP8]]
+// CHECK-NEXT:%[[SUB1:.+]] = sub i32 %[[TMP8]], 1
+// CHECK-NEXT:%[[ADD:.+]] = add i32 %[[SUB]], %[[SUB1]]
+// CHECK-NEXT:%[[TMP9:.+]] = load i32, i32* %[[DOTSTEP]], align 4
+// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[ADD]], %[[TMP9]]
 // CHECK-NEXT:br label %[[COND_END:.+]]
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_FALSE]]:
@@ -181,8 +184,8 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_END]]:
 // CHECK-NEXT:%[[COND:.+]] = phi i32 [ %[[DIV]], %[[COND_TRUE]] ], [ 0, %[[COND_FALSE]] ]
-// CHECK-NEXT:%[[TMP9:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
-// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP9]], align 4
+// CHECK-NEXT:%[[TMP10:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
+// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP10]], align 4
 // CHECK-NEXT:ret void
 // CHECK-NEXT:  }
 
Index: clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
===
--- clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
+++ clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
@@ -206,7 +206,10 @@
 // CHECK-NEXT:%[[TMP10:.+]] = load i32, i32* %[[DOTSTART]], align 4
 // CHECK-NEXT:%[[SUB:.+]] = sub nsw i32 %[[TMP9]], %[[TMP10]]
 // CHECK-NEXT:%[[TMP11:.+]] = load i32, i32* %[[DOTSTEP]], align 4
-// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[SUB]], %[[TMP11]]
+// CHECK-NEXT:%[[SUB1:.+]] = sub i32 %[[TMP11]], 1
+// CHECK-NEXT:%[[ADD:.+]] = add i32 %[[SUB]], %[[SUB1]]
+// CHECK-NEXT:%[[TMP12:.+]] = load i32, i32* %[[DOTSTEP]], align 4
+// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[ADD]], %[[TMP12]]
 // CHECK-NEXT:br label %[[COND_END:.+]]
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_FALSE]]:
@@ -214,8 +217,8 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_END]]:
 // CHECK-NEXT:%[[COND:.+]] = phi i32 [ %[[DIV]],

[PATCH] D118542: [Clang][OpenMPIRBuilder] Fix off-by-one error when dividing by stepsize.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Pre-merge failures are unrelated. See 
https://github.com/llvm/llvm-project/issues/53467


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118542/new/

https://reviews.llvm.org/D118542

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@peixin Thanks for testing edge cases. You hit multiple issues:

1. Chunksize was i32, combining it with a i64 induction variable caused an 
error. Fixed the latest update of this patch.
2. OpenMPIRBuilder currently doesn't work really with exceptions. See D115216 
 for a start of a discussion with `#pragma 
omp parallel`. Support for irregular exits (exceptions, cancellation, 
destructors) out of CanonicalLoopInfo is what I was working on recently. Use 
`-fno-exceptions` to work around.
3. There is an off-by-one error that I already fixed in my development branch. 
Upstream patch here: D118542 

Result with these fixes for me is:

  lb: 18446744073709551615
  ub: 1844674407370955161
  step: 1844674407370955161
  18446744073709551615
  16602069666338596454
  14757395258967641293
  12912720851596686132
  11068046444225730971
  9223372036854775810
  7378697629483820649
  5534023222112865488
  3689348814741910327
  1844674407370955166

Note that this does involve `__kmpc_doacross_init` code in libomp you 
pointed-to in D116292 . This uses 
`__kmpc_for_static_init` calls of which there are 4 variants for 
(signed/unsigned x 32/64 bits). To do `__kmpc_doacross_init` correctly, it 
would also need at least have variants for signed/unsigned (or one working 
internally with signed 128 bits).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118542: [Clang][OpenMPIRBuilder] Fix off-by-one error when dividing by stepsize.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: kiranchandramohan, ftynse, peixin, jdoerfert, 
clementval, Leporacanthicus, kiranktp, arnamoy10, bryanpkc, Chuanfeng, 
AMDChirag, anchu-rajendran, SouraVX, fghanim, jdenny, MatsPetersson, ABataev.
Herald added subscribers: zzheng, guansong, yaxunl.
Meinersbur requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added a project: clang.

When the stepsize does not evenly divide the range's end, round-up to ensure 
that that last multiple of the stepsize before the reaching the upper boud is 
reached. For instance, the trip count of

  for (int i = 0; i < 7; i+=5)

is two (i=0 and i=5), not (7-0)/5 == 1.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118542

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/irbuilder_for_unsigned.c
  clang/test/OpenMP/irbuilder_for_unsigned_down.c
  clang/test/OpenMP/irbuilder_unroll_full.c
  clang/test/OpenMP/irbuilder_unroll_heuristic.c
  clang/test/OpenMP/irbuilder_unroll_partial_factor.c
  clang/test/OpenMP/irbuilder_unroll_partial_factor_for.c
  clang/test/OpenMP/irbuilder_unroll_partial_heuristic.c
  clang/test/OpenMP/irbuilder_unroll_partial_heuristic_constant_for.c
  clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
  clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
  clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c

Index: clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c
===
--- clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c
+++ clang/test/OpenMP/irbuilder_unroll_unroll_partial_heuristic.c
@@ -154,7 +154,10 @@
 // CHECK-NEXT:%[[TMP7:.+]] = load i32, i32* %[[DOTSTART]], align 4
 // CHECK-NEXT:%[[SUB:.+]] = sub nsw i32 %[[TMP6]], %[[TMP7]]
 // CHECK-NEXT:%[[TMP8:.+]] = load i32, i32* %[[DOTSTEP]], align 4
-// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[SUB]], %[[TMP8]]
+// CHECK-NEXT:%[[SUB1:.+]] = sub i32 %[[TMP8]], 1
+// CHECK-NEXT:%[[ADD:.+]] = add i32 %[[SUB]], %[[SUB1]]
+// CHECK-NEXT:%[[TMP9:.+]] = load i32, i32* %[[DOTSTEP]], align 4
+// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[ADD]], %[[TMP9]]
 // CHECK-NEXT:br label %[[COND_END:.+]]
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_FALSE]]:
@@ -162,8 +165,8 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_END]]:
 // CHECK-NEXT:%[[COND:.+]] = phi i32 [ %[[DIV]], %[[COND_TRUE]] ], [ 0, %[[COND_FALSE]] ]
-// CHECK-NEXT:%[[TMP9:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
-// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP9]], align 4
+// CHECK-NEXT:%[[TMP10:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
+// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP10]], align 4
 // CHECK-NEXT:ret void
 // CHECK-NEXT:  }
 
Index: clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
===
--- clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
+++ clang/test/OpenMP/irbuilder_unroll_unroll_partial_factor.c
@@ -173,7 +173,10 @@
 // CHECK-NEXT:%[[TMP7:.+]] = load i32, i32* %[[DOTSTART]], align 4
 // CHECK-NEXT:%[[SUB:.+]] = sub nsw i32 %[[TMP6]], %[[TMP7]]
 // CHECK-NEXT:%[[TMP8:.+]] = load i32, i32* %[[DOTSTEP]], align 4
-// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[SUB]], %[[TMP8]]
+// CHECK-NEXT:%[[SUB1:.+]] = sub i32 %[[TMP8]], 1
+// CHECK-NEXT:%[[ADD:.+]] = add i32 %[[SUB]], %[[SUB1]]
+// CHECK-NEXT:%[[TMP9:.+]] = load i32, i32* %[[DOTSTEP]], align 4
+// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[ADD]], %[[TMP9]]
 // CHECK-NEXT:br label %[[COND_END:.+]]
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_FALSE]]:
@@ -181,8 +184,8 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:  [[COND_END]]:
 // CHECK-NEXT:%[[COND:.+]] = phi i32 [ %[[DIV]], %[[COND_TRUE]] ], [ 0, %[[COND_FALSE]] ]
-// CHECK-NEXT:%[[TMP9:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
-// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP9]], align 4
+// CHECK-NEXT:%[[TMP10:.+]] = load i32*, i32** %[[DISTANCE_ADDR]], align 8
+// CHECK-NEXT:store i32 %[[COND]], i32* %[[TMP10]], align 4
 // CHECK-NEXT:ret void
 // CHECK-NEXT:  }
 
Index: clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
===
--- clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
+++ clang/test/OpenMP/irbuilder_unroll_partial_heuristic_runtime_for.c
@@ -206,7 +206,10 @@
 // CHECK-NEXT:%[[TMP10:.+]] = load i32, i32* %[[DOTSTART]], align 4
 // CHECK-NEXT:%[[SUB:.+]] = sub nsw i32 %[[TMP9]], %[[TMP10]]
 // CHECK-NEXT:%[[TMP11:.+]] = load i32, i32* %[[DOTSTEP]], align 4
-// CHECK-NEXT:%[[DIV:.+]] = udiv i32 %[[SUB]], %[[TMP11]]
+// CHECK-NEXT:%[[SUB1:.+]] = sub i32 %[[TMP11]], 1
+// CHECK-NEXT:%[[ADD:.+]] = add i32 %[[SUB]], %[[SUB1]]
+// CHECK-NEXT:%[[TMP12:.+]] = load i32, i32* %[[DOTSTEP]], alig

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: jdoerfert, kiranchandramohan, peixin, clementval, 
Leporacanthicus, kiranktp, AMDChirag, fghanim, jdenny, MatsPetersson, ftynse.
Herald added subscribers: awarzynski, sdasgup3, wenzhicui, wrengr, 
Chia-hungDuan, dcaballe, cota, teijeong, rdzhabarov, tatianashp, msifontes, 
jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, 
arpith-jacob, antiagainst, shauheen, rriddle, mehdi_amini, guansong, hiraditya, 
yaxunl.
Meinersbur requested review of this revision.
Herald added subscribers: llvm-commits, sstefan1, stephenneuendorffer, 
nicolasvasilache.
Herald added projects: MLIR, LLVM.

Follow-up on D117226  for createSections.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117835

Files:
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
  mlir/test/Target/LLVMIR/openmp-llvm.mlir

Index: mlir/test/Target/LLVMIR/openmp-llvm.mlir
===
--- mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -842,6 +842,9 @@
 
 // CHECK-LABEL: @omp_sections_trivial
 llvm.func @omp_sections_trivial() -> () {
+  // CHECK:   br label %[[ENTRY:[a-zA-Z_.]+]]
+
+  // CHECK: [[ENTRY]]:
   // CHECK:   br label %[[PREHEADER:.*]]
 
   // CHECK: [[PREHEADER]]:
Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -66,7 +66,24 @@
   if (walkResult.wasInterrupted())
 return allocaInsertPoint;
 
-  // Otherwise, insert to the entry block of the surrounding function.
+  // Otherwise, insert to the entry block of the surrounding function.s
+  // If the current IRBuilder InsertPoint is the function's entry, it cannot
+  // also be used for alloca insertion which would result in insertion order
+  // confusion. Create a new BasicBlock for the Builder and use the entry block
+  // for the allocs.
+  // TODO: Create a dedicated alloca BasicBlock at function creation such that
+  // we do not need to move the current InertPoint here.
+  if (builder.GetInsertBlock() ==
+  &builder.GetInsertBlock()->getParent()->getEntryBlock()) {
+assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() &&
+   "Assuming end of basic block");
+llvm::BasicBlock *entryBB = llvm::BasicBlock::Create(
+builder.getContext(), "entry", builder.GetInsertBlock()->getParent(),
+builder.GetInsertBlock()->getNextNode());
+builder.CreateBr(entryBB);
+builder.SetInsertPoint(entryBB);
+  }
+
   llvm::BasicBlock &funcEntryBlock =
   builder.GetInsertBlock()->getParent()->getEntryBlock();
   return llvm::OpenMPIRBuilder::InsertPointTy(
@@ -249,24 +266,13 @@
   // TODO: Is the Parallel construct cancellable?
   bool isCancellable = false;
 
-  // Ensure that the BasicBlock for the the parallel region is sparate from the
-  // function entry which we may need to insert allocas.
-  if (builder.GetInsertBlock() ==
-  &builder.GetInsertBlock()->getParent()->getEntryBlock()) {
-assert(builder.GetInsertPoint() == builder.GetInsertBlock()->end() &&
-   "Assuming end of basic block");
-llvm::BasicBlock *entryBB =
-llvm::BasicBlock::Create(builder.getContext(), "parallel.entry",
- builder.GetInsertBlock()->getParent(),
- builder.GetInsertBlock()->getNextNode());
-builder.CreateBr(entryBB);
-builder.SetInsertPoint(entryBB);
-  }
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(
   builder.saveIP(), builder.getCurrentDebugLocation());
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createParallel(
-  ompLoc, findAllocaInsertPoint(builder, moduleTranslation), bodyGenCB,
-  privCB, finiCB, ifCond, numThreads, pbKind, isCancellable));
+  ompLoc, allocaIP, bodyGenCB, privCB, finiCB, ifCond, numThreads, pbKind,
+  isCancellable));
 
   return bodyGenStatus;
 }
@@ -528,9 +534,10 @@
   storeValues.push_back(vecValues[indexVecValues]);
   indexVecValues++;
 }
+llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+findAllocaInsertPoint(builder, moduleTranslation);
 builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createOrderedDepend(
-ompLoc, findAllocaInsertPoint(builder, moduleTranslation), numLoops,
-storeValues, ".cnt.addr", isDependSource));
+ompLoc, allocaIP, numLoops, storeValues, ".cnt.addr", isDependSource));
   }
   return success();
 }
@@ -635,11 +642,13 @@
   // calle

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1691-1696
+  bool Found = false;
+  if (llvm::any_of(*LoopBody, [](Instruction &I) {
+return I.getMetadata("llvm.access.group") != nullptr;
+  }))
+Found = true;
+  EXPECT_TRUE(Found);

Can make it a one-liner.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for the patch.




Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1696
+  }
+  EXPECT_EQ(Found, true);
+}

It would also be possible to use `any_of` instead of a loop.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2587
+  // Check for unsupported clauses
+  if (S.clauses().size() > 0) {
+// Currently no clause is supported





Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2591
+  }
+  // Check if we have a statement with the ordered directive.
+  // Visit the statement hierarchy to find a compound statement




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

[testing] Could you add a test for `applySimd` into `OpenMPIRBuilderTests.cpp`, 
so we have a test that is independent of Clang?




Comment at: clang/test/OpenMP/irbuilder_simd.cpp:3
+// expected-no-diagnostics 
+// RUN: %clang_cc1 -fopenmp-enable-irbuilder -verify -fopenmp 
-fopenmp-version=45 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | 
FileCheck %s -check-prefix=CHECKTWOLOOPS 
+// expected-no-diagnostics

The invocation is identical to the RUN on line 1. `CHECKTWOLOOPS` is not needed 
and can also be made as `CHECK`.



Comment at: clang/test/OpenMP/irbuilder_simd.cpp:4
+// RUN: %clang_cc1 -fopenmp-enable-irbuilder -verify -fopenmp 
-fopenmp-version=45 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | 
FileCheck %s -check-prefix=CHECKTWOLOOPS 
+// expected-no-diagnostics
+

A single `expected-no-diagnostics` is sufficient



Comment at: clang/test/OpenMP/irbuilder_simd.cpp:67-69
+// CHECK: ![[META0:[0-9]+]] = !{i32 1, !"wchar_size", i32 4}
+// CHECK-NEXT: ![[META1:[0-9]+]]  = !{i32 7, !"openmp", i32 45}
+// CHECK-NEXT: ![[META2:[0-9]+]]  =

If you added these CHECK lines by hand, remove those that are not relevant



Comment at: clang/test/OpenMP/irbuilder_simd.cpp:71
+// CHECK-NEXT: ![[META3:[0-9]+]] = distinct !{}
+// CHECK-NEXT: ![[META4:[0-9]+]]  = distinct !{![[META4:[0-9]+]], 
![[META5:[0-9]+]], ![[META6:[0-9]+]]}
+// CHECK-NEXT: ![[META5:[0-9]+]]  = !{!"llvm.loop.parallel_accesses", 
![[META3:[0-9]+]]}

Do not add a regex specification for any but the first occurance. A regex 
specification will "overwrite" the previous match, i.e. the different 
occurrences of `META4` would not need to be the same.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2120
 
+/// Attach metadata llvm.access.group to the memref instructions of \p block
+static void addSimdMetadata(BasicBlock *Block,





Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2124
+  for (Instruction &I : *Block) {
+if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
+  I.setMetadata(LLVMContext::MD_access_group, AccessGroup);





Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2125
+if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
+  I.setMetadata(LLVMContext::MD_access_group, AccessGroup);
+}

The instruction may already have an access group assigned e.g. from a nested 
`#pragma clang loop vectorize(assume_safety)`. This would overwrite the access 
group. See `LoopInfoStack::InsertHelper` (`CGLoopInfo.cpp`) on how to assign 
multiple access groups, or add a TODO.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2156
+  DominatorTreeAnalysis DTA;
+  DominatorTree &&DT = DTA.run(*F, FAM);
+  LoopAnalysis LIA;

Compiler warning: `warning C4189: 'DT': local variable is initialized but not 
referenced`



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2162
+
+  llvm::SmallSet Reachable; 
+

OpenMPIRBuilder is part of the `llvm` namespace, `llvm::` is not necessary.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2166
+  // can be found.
+  for (BasicBlock *Block:L->getBlocks()) {
+if (Block == CanonicalLoop->getCond() || Block == 
CanonicalLoop->getHeader()) continue;

Could you add a todo note such as: 
```
// TODO: Generalize getting all blocks inside a CanonicalizeLoopInfo, 
preferably without running any passes.
```



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2173-2175
+  for (BasicBlock *BB : Reachable) {
+addSimdMetadata(BB, AccessGroup);
+  }

[style]



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2181
+  ConstantInt::getTrue(Type::getInt1Ty(Ctx)));
+  addLoopMetadata(
+  CanonicalLoop,

The loop might already have a `llvm.loop.parallel_accesses`, in which case 
those two lists could be combined.

I don't think its very relevant, but maybe add a TODO?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

ping?

@erichkeane Since you are pushing for upgrade the gcc/clang requirement as 
well, would you take care of that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114639/new/

https://reviews.llvm.org/D114639

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115378: OpenMP: Avoid using SmallVector::set_size()

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115378/new/

https://reviews.llvm.org/D115378

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2588-2594
+if (isa(C) || isa(C) ||
+isa(C) || isa(C) ||
+isa(C) || isa(C) ||
+isa(C) || isa(C) ||
+isa(C) || isa(C) ||
+isa(C))
+  return false;

I think it would be better to list the clauses that are supported. In addition 
to being shorter, it would also reject any clause we forgot about and new ones.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2123
+ArrayRef Properties) {
+  for (auto &I : *Block) {
+if (I.mayReadFromMemory() || I.mayWriteToMemory()) {

[style] According to the LLVM coding standard, auto is only used in specific 
cases.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2125
+if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
+  Instruction *instr = dyn_cast(&I);
+  LLVMContext &C = instr->getContext();

[style] Please use the current LLVM coding style.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2127
+  LLVMContext &C = instr->getContext();
+  MDNode *LoopID = MDNode::get(C, Properties);
+  instr->setMetadata("llvm.access.group", LoopID);

[serious] This does not create a LoopID. A LoopID is a bag of properties where 
the first element points to itself (which automatically makes then `distinct`). 
E.g.
```
!0 = distinct !{!0, !{!"llvm.loop.parallel_accesses", !1}}
```
where `!1` is an access group (eg `!1 = distinct !{}`).

[serious] The instruction's metadata should not be a LoopID, but a access group 
(eg `!1` for the above example). I think you should not pass an array of 
properties, but just the access group MDNode.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2152
+  Loop,
+  {MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.parallel_accesses")),
+   MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.vectorize.enable"))});

[serious] The `llvm.loop.parallel_accesses` property takes an argument: the 
access group.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2157-2158
+  // exit block.  May have to enhance this collection for nested loops.
+  BasicBlock *body = Loop->getBody();
+  BasicBlock *exit = Loop->getExit();
+

[Style] Please use the current LLVM coding standard.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2182
+worklist.push_back(succ);
+if (!DT.dominates(exit, succ) && skipBBs.count(succ) == 0)
+  reachable.insert(succ);

[serious] Using DominatorTree to check whether we passes Exit is redundant: For 
any block that is dominated by Exit (by definition of dominator) we will have 
to visited Exit first anyway. Therefore not adding Exit itself would be 
sufficient.

You actually don't want paths that go to exit, but the basic blocks that come 
from Body (the successor of Cond) to the Continue/Latch block. By the 
definition of CanonicalLoopInfo, any block reachable from Body can only go to 
Continue[1], so it would be sufficient to iterate everything reachable from 
Body, including Body itself, until we hit the Latch. Header and Cond themselves 
are not allowed to contain any memory accesses.

The safest way to determine the set of blocks inside the loop would be using 
LoopInfo, which internally uses DominatorTree, but for checking dominance with 
the header, not the exit.

[1] There are exceptions: paths can end in an `unreachable` instruction and I 
am currently working on defining what happens at cancellation[2]. There will 
also be a BasicBlock like Latch/Continue that signals the end of the loop. In 
both cases these still contain blocks that are not inside the loop in 
LoopInfo's sense. However, marking instructions in them with access groups 
should is not a problem since the access groups marker only has an effect if 
inside a loop that has the `llvm.loop.parallel_accesses` property set to that 
access group.

[2] I found interesting cases in which Clang re-uses the same basic blocks 
(cleanup block for destructors and exception handling) for multiple paths by 
using a PHINode and a switch to branch to the scope it came from. That is, if 
part of a loop it would always branch back to another block in the loop, but 
statically any other switch target would be reachable as well. Not sure yet 
whether this will be a problem that requires us to use LoopInfo.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D114413#3160044 , @peixin wrote:

> In D114413#3159095 , @Meinersbur 
> wrote:
>
>> In D114413#3154936 , @peixin wrote:
>>
>>> 
>
> `getPreheader()` was in `OMPIRBuilder.cpp` before you rebase in your last 
> update here. That's why I let you rebase since I failed to git apply your 
> last patch in main branch. It's not important now. Please forget about that.

D114368  (which this patch depends on) moves 
`getPreheder()` to the `.cpp` files (because it has become more than a simple 
getter)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2586-2587
 void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {
+
+  bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+  if (UseOMPIRBuilder) {

arnamoy10 wrote:
> Meinersbur wrote:
> > [nit] Unnecessary whitespace
> I took a look at `isSupportedByOpenMPIRBuilder()` and seems like it is more 
> suitable for a check against work-sharing loop construct, not for `simd`.  
> 
> And for `simd`, I am not sure what features I should be checking for.  One 
> check I can think of is checking whether any of the not-yet-supported clauses 
> is present in the simd directive or not.  Am I on the right track?  
Indeed, `isSupportedByOpenMPIRBuilder` is for the workshoring-loop, you could 
introduce another one for simd. Or modify it to accept `OMPExecutableDirective` 
as an argument. This might be useful for combined constructs such as `for simd`.

Best approach IMHO is to whitelist clauses. Reject any clause you did not 
test/add support yet. This way we can fallback to the non-OpenMPIRBuilder 
codegen and not break things that worked without `-fopenmp-enable-irbuilder` (a 
lot of code already does not work `-fopenmp-enable-irbuilder`, so don't put too 
much effort into getting the interaction between OpenMPIRBuilder and current 
codegen to work; but we need an overview what is already supported and which 
clauses are still require some work)



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2594
+// Emit the associated statement and get its loop representation.
+auto DL = SourceLocToDebugLoc(S.getBeginLoc());
+const Stmt *Inner = S.getRawStmt();

arnamoy10 wrote:
> Meinersbur wrote:
> > [style] According to the [[ 
> > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> >  | LLVM coding standard ]], `auto` is only used in specific cases.
> I am inspired by [[ 
> https://github.com/llvm/llvm-project/blob/003c9c7457d0be5deeca7eee84ab5f110bf6/clang/lib/CodeGen/CGStmtOpenMP.cpp#L2611
>  | existing code ]] though
There already is a lot of code violating the coding standard, either because 
the coding standard changed or it slipped through a review. A prominent example 
is that the methods of the IRBuilder are still PascalCase, although the current 
standard says they should use camelCase. Same applies to `SourceLocToDebugLoc` 
itself (which every time I see it makes me think `SourceLocToDebugLoc` is a 
type)

Not a reason to not use the newest coding standard for new code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D114413#3154936 , @peixin wrote:

> @Meinersbur Please rebase on main. The function "getPreheader()" was moved 
> into OMPIRBuilder.h.

I rebased, but I am not sure what you are referring to. `getPreheader()` always 
was in `OMPIRBuilder.h`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114143: [OpenMP][IRBuilder] Fix createSections

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

AllocaIP could be handled better, but as a fix LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114143/new/

https://reviews.llvm.org/D114143

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 17 2022" -T LLVM ..
 

jhenderson wrote:
> RKSimon wrote:
> > aaron.ballman wrote:
> > > jhenderson wrote:
> > > > I think the missing space should be fixed to :)
> > > +1 to the missing space.
> > This one is confusing - it isn't in my local diff, the raw diff, or if I 
> > reapply the raw diff - it looks to have just appeared when you quote it in 
> > phab comments?
> The fact that the space is missing? It's missing in the current repo too.
It works with and without the space, like `-GNinja` and `-G Ninja` both work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114639/new/

https://reviews.llvm.org/D114639

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

Thanks for helping to complete the OpenMPIRBuilder implementation!




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2586-2587
 void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {
+
+  bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+  if (UseOMPIRBuilder) {

[nit] Unnecessary whitespace



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2587
+
+  bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+  if (UseOMPIRBuilder) {

Did you consider adding a check whether OpenMPIRBuilder actually implements all 
features requested before using it? See `isSupportedByOpenMPIRBuilder`.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2594
+// Emit the associated statement and get its loop representation.
+auto DL = SourceLocToDebugLoc(S.getBeginLoc());
+const Stmt *Inner = S.getRawStmt();

[style] According to the [[ 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
 | LLVM coding standard ]], `auto` is only used in specific cases.



Comment at: clang/test/OpenMP/simd_codegen_irbuilder.cpp:16
+// CHECK-NEXT: call void @__captured_stmt.1(i32* %i, i32 %omp_loop.iv, 
%struct.anon.0* %agg.captured1)
+// CHECK-NEXT: %3 = load float*, float** %b.addr, align 8, 
!llvm.access.group !3
+// CHECK-NEXT: %4 = load i32, i32* %i, align 4, !llvm.access.group !3

Could you use regexes to match the virtual register names? The script 
`update_cc_checks.py` may help with this, see the other tests.

The naming scheme for files testing the OpenMPIRBiulder is usually 
`irbuilder_`



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:524
+  /// \param Loop The loop to simd-ise.
+  void createSIMDLoop(DebugLoc DL, CanonicalLoopInfo *Loop);
+

Capitalizing SIMD leads to long runs of capital letters. While not an LLVM 
coding style rule, it is still common to title-case acronyms in identifiers if 
the acronym is ~4 letters or more, such as `OMPSimdDirective` instead of 
`OMPSIMDDirective`.

For methods transformation `CanonicalLoopInfo`, we don't use `createXYZ` naming 
scheme which is reserved for inserting something new. Suggestions: `applySimd`, 
`vectorizeLoop`, or `simdizeLoop`.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2120
+/// Attach metadata access.group to the load and store instructions of \p block
+static void addSIMDMetadata(BasicBlock *block,
+ArrayRef Properties) {

The LLVM coding style still capitalizes parameters and variables.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2123
+  for (auto &I : *block) {
+if (isa(&I) || isa(&I)) {
+  Instruction *instr = dyn_cast(&I);

The property also applies to any memory access, e.g. `memset`. You could use 
`mayWriteToMemory()` and `mayReadFromMemory()`.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2126
+  LLVMContext &C = instr->getContext();
+  MDNode *N = MDNode::get(C, MDString::get(C, ""));
+  instr->setMetadata("llvm.access.group", N);

[serious] The access group must be unique only to the loop being accessed.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2157-2168
+  addSIMDMetadata(header,
+  {
+  MDNode::get(Ctx, MDString::get(Ctx, 
"llvm.access.group")),
+  });
+  addSIMDMetadata(cond,
+  {
+  MDNode::get(Ctx, MDString::get(Ctx, 
"llvm.access.group")),

[serious] header and cond don't contain any memory accesses. `body` may consist 
of multiple basic blocks, of which this only applies to the first one. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114379/new/

https://reviews.llvm.org/D114379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113210: [NewPM] Use the default AA pipeline by default

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This change caused the Polly build to fail: 
https://lab.llvm.org/buildbot/#/builders/10/builds/7501

  opt: 
/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.src/llvm/include/llvm/IR/PassManager.h:784:
 typename PassT::Result& llvm::AnalysisManager::getResult(IRUnitT&, ExtraArgTs ...) [with PassT = 
llvm::OuterAnalysisManagerProxy, 
llvm::Function>; IRUnitT = llvm::Function; ExtraArgTs = {}; typename 
PassT::Result = 
llvm::OuterAnalysisManagerProxy, 
llvm::Function>::Result]: Assertion `AnalysisPasses.count(PassT::ID()) && "This 
analysis pass was not registered prior to being queried"' failed.
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace.
  Stack dump:
  0.Program arguments: 
/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt 
-polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names 
-polly-import-jscop-dir=/home/worker/src/llvm-project/polly/test/ScopInliner 
-polly-codegen-verify -polly-detect-full-functions -polly-scop-inliner 
-polly-scops -analyze
  1.Running pass 'CallGraph Pass Manager' on module ''.
   #0 0x7fcc7bb29644 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
   #1 0x7fcc7bb26d5e SignalHandler(int) Signals.cpp:0:0
   #2 0x7fcc7b541210 (/lib/x86_64-linux-gnu/libc.so.6+0x46210)
   #3 0x7fcc7b54118b raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618b)
   #4 0x7fcc7b520859 abort (/lib/x86_64-linux-gnu/libc.so.6+0x25859)
   #5 0x7fcc7b520729 (/lib/x86_64-linux-gnu/libc.so.6+0x25729)
   #6 0x7fcc7b531f36 (/lib/x86_64-linux-gnu/libc.so.6+0x36f36)
   #7 0x7fcc7dbcb34f void 
llvm::AAManager::getModuleAAResultImpl(llvm::Function&, 
llvm::AnalysisManager&, llvm::AAResults&) PassBuilder.cpp:0:0
   #8 0x7fcc7c3c5363 llvm::AAManager::run(llvm::Function&, 
llvm::AnalysisManager&) 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/./lib/libLLVMAnalysis.so.14git+0x257363)
   #9 0x563487033358 llvm::detail::AnalysisPassModel::Invalidator>::run(llvm::Function&, 
llvm::AnalysisManager&) NewPMDriver.cpp:0:0
  #10 0x7fcc7bf95dd8 
llvm::AnalysisManager::getResultImpl(llvm::AnalysisKey*, 
llvm::Function&) 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/./lib/libLLVMCore.so.14git+0x450dd8)
  #11 0x7fcc7e84892a polly::ScopAnalysis::run(llvm::Function&, 
llvm::AnalysisManager&) 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/./lib/libPolly.so.14git+0x13c92a)
  #12 0x7fcc7e962aa4 llvm::detail::AnalysisPassModel::Invalidator>::run(llvm::Function&, 
llvm::AnalysisManager&) RegisterPasses.cpp:0:0
  #13 0x7fcc7bf95dd8 
llvm::AnalysisManager::getResultImpl(llvm::AnalysisKey*, 
llvm::Function&) 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/./lib/libLLVMCore.so.14git+0x450dd8)
  #14 0x7fcc7e9d7d7c (anonymous 
namespace)::ScopInliner::runOnSCC(llvm::CallGraphSCC&) ScopInliner.cpp:0:0
  #15 0x7fcc7c46dfef (anonymous 
namespace)::CGPassManager::runOnModule(llvm::Module&) CallGraphSCCPass.cpp:0:0
  #16 0x7fcc7bf4dee2 llvm::legacy::PassManagerImpl::run(llvm::Module&) 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/./lib/libLLVMCore.so.14git+0x408ee2)
  #17 0x563487054571 main 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt+0x48571)
  #18 0x7fcc7b5220b3 __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x270b3)
  #19 0x563487030c8e _start 
(/home/worker/buildbot-workers/polly-x86_64-gce1/rundir/llvm.obj/bin/opt+0x24c8e)

Polly is not using custom AliasAnalysis. Any idea how to fix this?

Btw, the pre-merge check failed because of this as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113210/new/

https://reviews.llvm.org/D113210

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D104601#3105044 , @manmanren wrote:

> It will fail the compilation on the preprocessed output with
> error: expected unqualified-id
> int test();#pragma clang assume_nonnull end

The handler for assume_nonull passes an invalid SourceLocation if embedded in 
in a _Pragma. Not sure whether this is already a bug, but we should insert 
semantically relevant newlines even if the line for the #pragma is unknown. 
Fixed in rG1606022fab2d90ed8ee6d15800ec1c2c293db20e 
. Thanks 
for the report.




Comment at: clang/lib/Frontend/PrintPreprocessedOutput.cpp:680
+if (RequireSpace || (!MinimizeWhitespace && Tok.hasLeadingSpace()) ||
+((EmittedTokensOnThisLine || EmittedTokensOnThisLine) &&
+ AvoidConcat(PrevPrevTok, PrevTok, Tok)))

RKSimon wrote:
> @Meinersbur Static analysis is warning that these are both the same - should 
> one be EmittedDirectiveOnThisLine ?
Fixed in rG8f099d17a1bee857ada3c5af032cfcb559252024. Thanks for the report and 
sorry for not seeing you comment until now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112181: [docs] Remove hard-coded version numbers from sphinx configs

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Changes in Polly LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112181/new/

https://reviews.llvm.org/D112181

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111488: [Clang][clang-nvlink-wrapper] Pass nvlink path to the wrapper

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Thanks, this fixed the errors introduced in D105191 
: 
http://meinersbur.de:8011/#/builders/1/builds/1594


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111488/new/

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111488: [Clang][clang-nvlink-wrapper] Pass nvlink path to the wrapper

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

Some nitpicks, but it fixes the build, so LGTM.




Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:617
+  // Find nvlink and pass it as "--nvlink-command=" argument of 
clang-nvlink-wrapper.
+  auto NvlinkBin = getToolChain().GetProgramPath("nvlink");
+  const char *NvlinkPath =

[style] [[ 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
 | LLVM's coding standard does not use almost-always-auto ]].

It's not immediately obvious here, does `GetProgramPath` look into the BinPath 
detected by CudaInstallationDetector? I applied the patch locally to 
http://meinersbur.de:8011/#/builders/1 and it actually does work.



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:169-170
 
-  for (size_t i = 1; i < Argv.size(); ++i) {
-std::string Arg = Argv[i];
+  for (size_t i = 0; i < NVArgs.size(); ++i) {
+std::string Arg = NVArgs[i];
 if (sys::path::extension(Arg) == ".a") {




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111488/new/

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111488: [Clang][clang-nvlink-wrapper] Pass nvlink path to the wrapper

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

Thanks for the path, but command line parsing should be done properly.




Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:50
+static cl::opt
+NvlinkUserPath("path", cl::desc("path of directory containing nvlink"),
+   cl::cat(ClangNvlinkWrapperCategory));

I suggest a more descriptive flag, `path` is too general, like `--nvlink` or 
`--nvlink-command` (bugpoint uses `--opt-command` for the path to `opt`)



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:124
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {

[nit] unrelated whitespace change



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:133
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {
 cl::PrintHelpMessage();

`cl::ParseCommandLineOptions(argc, argv,"Program description")` must have been 
called beforehand to make this work.



Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:154
+StringRef ArgRef(Arg);
+auto NvlPath = ArgRef.startswith_insensitive("--path=");
 if (sys::path::extension(Arg) == ".a") {

This way of parsing does not use the declared `NvlinkUserPath` option. Use 
`cl::ParseCommandLineOptions` do parse the arguments and 
```
cl::list InputFiles(cl::Positional, cl::desc("..."));
```
to catch all non-option arguments.

The way it is done here also does not catch the `--path ` (without 
equal sign) syntax.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111488/new/

https://reviews.llvm.org/D111488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111124: [Clang][OpenMP] Allow loop-transformations with template parameters.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2130117f92e5: [Clang][OpenMP] Allow loop-transformations 
with template parameters. (authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D24/new/

https://reviews.llvm.org/D24

Files:
  clang/include/clang/AST/StmtOpenMP.h
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/OpenMP/tile_ast_print.cpp
  clang/test/OpenMP/unroll_ast_print.cpp

Index: clang/test/OpenMP/unroll_ast_print.cpp
===
--- clang/test/OpenMP/unroll_ast_print.cpp
+++ clang/test/OpenMP/unroll_ast_print.cpp
@@ -124,4 +124,26 @@
   unroll_templated();
 }
 
+
+// PRINT-LABEL: template  void unroll_templated_factor(int start, int stop, int step) {
+// DUMP-LABEL:  FunctionTemplateDecl {{.*}} unroll_templated_factor
+template 
+void unroll_templated_factor(int start, int stop, int step) {
+  // PRINT: #pragma omp unroll partial(Factor)
+  // DUMP:  OMPUnrollDirective
+  // DUMP-NEXT: OMPPartialClause
+  // DUMP-NEXT:   DeclRefExpr {{.*}} 'Factor' 'int'
+  #pragma omp unroll partial(Factor)
+// PRINT-NEXT: for (int i = start; i < stop; i += step)
+// DUMP-NEXT:  ForStmt
+for (int i = start; i < stop; i += step)
+  // PRINT-NEXT: body(i);
+  // DUMP:  CallExpr
+  body(i);
+}
+void unroll_template_factor() {
+  unroll_templated_factor<4>(0, 42, 2);
+}
+
+
 #endif
Index: clang/test/OpenMP/tile_ast_print.cpp
===
--- clang/test/OpenMP/tile_ast_print.cpp
+++ clang/test/OpenMP/tile_ast_print.cpp
@@ -162,4 +162,25 @@
 }
 
 
+// PRINT-LABEL: template  void foo7(int start, int stop, int step) {
+// DUMP-LABEL: FunctionTemplateDecl {{.*}} foo7
+template 
+void foo7(int start, int stop, int step) {
+  // PRINT: #pragma omp tile sizes(Tile)
+  // DUMP:  OMPTileDirective
+  // DUMP-NEXT:   OMPSizesClause
+  // DUMP-NEXT: DeclRefExpr {{.*}} 'Tile' 'int'
+  #pragma omp tile sizes(Tile)
+// PRINT-NEXT:  for (int i = start; i < stop; i += step)
+// DUMP-NEXT: ForStmt
+for (int i = start; i < stop; i += step)
+  // PRINT-NEXT: body(i);
+  // DUMP:  CallExpr
+  body(i);
+}
+void tfoo7() {
+  foo7<5>(0, 42, 2);
+}
+
+
 #endif
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2226,6 +2226,7 @@
 void ASTStmtWriter::VisitOMPLoopTransformationDirective(
 OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+  Record.writeUInt32(D->getNumGeneratedLoops());
 }
 
 void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -2327,6 +2327,7 @@
 void ASTStmtReader::VisitOMPLoopTransformationDirective(
 OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+  D->setNumGeneratedLoops(Record.readUInt32());
 }
 
 void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -12919,10 +12919,12 @@
   Body, OriginalInits))
 return StmtError();
 
+  unsigned NumGeneratedLoops = PartialClause ? 1 : 0;
+
   // Delay unrolling to when template is completely instantiated.
   if (CurContext->isDependentContext())
 return OMPUnrollDirective::Create(Context, StartLoc, EndLoc, Clauses, AStmt,
-  nullptr, nullptr);
+  NumGeneratedLoops, nullptr, nullptr);
 
   OMPLoopBasedDirective::HelperExprs &LoopHelper = LoopHelpers.front();
 
@@ -12941,9 +12943,9 @@
   // The generated loop may only be passed to other loop-associated directive
   // when a partial clause is specified. Without the requirement it is
   // sufficient to generate loop unroll metadata at code-generation.
-  if (!PartialClause)
+  if (NumGeneratedLoops == 0)
 return OMPUnrollDirective::Create(Context, StartLoc, EndLoc, Clauses, AStmt,
-  nullptr, nullptr);
+  NumGeneratedLoops, nullptr, nullptr);
 
   // Otherwise, we need to provide a de-sugared/transformed AST that can be
   // associated with another loop directive.
@@ -13164,7 +13166,8 @@
   LoopHelper.Init->getBeginLoc(), LoopHelper.Inc->getEndLoc());
 
   return OMPUnrollDirective::Create(Context, StartLoc, EndLoc, Clauses, 

[PATCH] D111119: [Clang][OpenMP] Infix OMPLoopTransformationDirective abstract class. NFC.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf37e8b0b831e: [Clang][OpenMP] Infix 
OMPLoopTransformationDirective abstract class. NFC. (authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D19/new/

https://reviews.llvm.org/D19

Files:
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/StmtNodes.td
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2046,6 +2046,8 @@
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
   void VisitOMPParallelDirective(const OMPParallelDirective *D);
   void VisitOMPSimdDirective(const OMPSimdDirective *D);
+  void
+  VisitOMPLoopTransformationDirective(const OMPLoopTransformationDirective *D);
   void VisitOMPTileDirective(const OMPTileDirective *D);
   void VisitOMPUnrollDirective(const OMPUnrollDirective *D);
   void VisitOMPForDirective(const OMPForDirective *D);
@@ -2901,12 +2903,17 @@
   VisitOMPLoopDirective(D);
 }
 
-void EnqueueVisitor::VisitOMPTileDirective(const OMPTileDirective *D) {
+void EnqueueVisitor::VisitOMPLoopTransformationDirective(
+const OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
 }
 
+void EnqueueVisitor::VisitOMPTileDirective(const OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
+}
+
 void EnqueueVisitor::VisitOMPUnrollDirective(const OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
 }
 
 void EnqueueVisitor::VisitOMPForDirective(const OMPForDirective *D) {
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2223,13 +2223,18 @@
   Code = serialization::STMT_OMP_SIMD_DIRECTIVE;
 }
 
-void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
+void ASTStmtWriter::VisitOMPLoopTransformationDirective(
+OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+}
+
+void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
   Code = serialization::STMT_OMP_TILE_DIRECTIVE;
 }
 
 void ASTStmtWriter::VisitOMPUnrollDirective(OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
   Code = serialization::STMT_OMP_UNROLL_DIRECTIVE;
 }
 
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -2324,12 +2324,17 @@
   VisitOMPLoopDirective(D);
 }
 
-void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
+void ASTStmtReader::VisitOMPLoopTransformationDirective(
+OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
 }
 
+void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
+}
+
 void ASTStmtReader::VisitOMPUnrollDirective(OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
 }
 
 void ASTStmtReader::VisitOMPForDirective(OMPForDirective *D) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -3823,13 +3823,8 @@
 VisitSubCaptures(S);
   }
 
-  void VisitOMPTileDirective(OMPTileDirective *S) {
-// #pragma omp tile does not introduce data sharing.
-VisitStmt(S);
-  }
-
-  void VisitOMPUnrollDirective(OMPUnrollDirective *S) {
-// #pragma omp unroll does not introduce data sharing.
+  void VisitOMPLoopTransformationDirective(OMPLoopTransformationDirective *S) {
+// Loop transformation directives do not introduce data sharing
 VisitStmt(S);
   }
 
@@ -9050,15 +9045,8 @@
 }
 return false;
   },
-  [&SemaRef, &Captures](OMPLoopBasedDirective *Transform) {
-Stmt *DependentPreInits;
-if (auto *Dir = dyn_cast(Transform)) {
-  DependentPreInits = Dir->getPreInits();
-} else if (auto *Dir = dyn_cast(Transform)) {
-  DependentPreInits = Dir->getPreInits();
-} else {
-  llvm_unreachable("Unexpected loop transformation");
-}
+  [&SemaRef, &Captures](OMPLoopTransformationDirective *Transform) {
+Stmt *DependentPreInits = Transform->getPreInits();
 if (!DependentPreInits)
   retu

[PATCH] D111124: [Clang][OpenMP] Allow loop-transformations with template parameters.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: ABataev, jdenny.
Meinersbur added projects: OpenMP, clang.
Herald added subscribers: zzheng, guansong, yaxunl.
Meinersbur requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

Clang would reject

  #pragma omp for
  #pragma omp tile sizes(P)
  for (int i = 0; i < 128; ++i) {}

where P is a template parameter, but the loop itself is not template-dependent. 
Because P context-dependent, the TransformedStmt cannot be generated and 
therefore nullptr (until the template is instantiated). The OMPForDirective 
would still expect the a loop is the dependent context and trigger an error.

Fix by introducing a NumGeneratedLoops field to OMPLoopTransformation. This is 
used to distinguish the case where no TransformedStmt will be generated at all 
(e.g. `#pragma omp unroll full`) and template instantiation is needed. In the 
later case, delay resolving the iteration space like when the for-loop itself 
is template-dependent until the template instatiation.

A more radical solution would always delay the iteration space analysis until 
template instantiation, but would also break many test cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D24

Files:
  clang/include/clang/AST/StmtOpenMP.h
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/OpenMP/tile_ast_print.cpp
  clang/test/OpenMP/unroll_ast_print.cpp

Index: clang/test/OpenMP/unroll_ast_print.cpp
===
--- clang/test/OpenMP/unroll_ast_print.cpp
+++ clang/test/OpenMP/unroll_ast_print.cpp
@@ -124,4 +124,26 @@
   unroll_templated();
 }
 
+
+// PRINT-LABEL: template  void unroll_templated_factor(int start, int stop, int step) {
+// DUMP-LABEL:  FunctionTemplateDecl {{.*}} unroll_templated_factor
+template 
+void unroll_templated_factor(int start, int stop, int step) {
+  // PRINT: #pragma omp unroll partial(Factor)
+  // DUMP:  OMPUnrollDirective
+  // DUMP-NEXT: OMPPartialClause
+  // DUMP-NEXT:   DeclRefExpr {{.*}} 'Factor' 'int'
+  #pragma omp unroll partial(Factor)
+// PRINT-NEXT: for (int i = start; i < stop; i += step)
+// DUMP-NEXT:  ForStmt
+for (int i = start; i < stop; i += step)
+  // PRINT-NEXT: body(i);
+  // DUMP:  CallExpr
+  body(i);
+}
+void unroll_template_factor() {
+  unroll_templated_factor<4>(0, 42, 2);
+}
+
+
 #endif
Index: clang/test/OpenMP/tile_ast_print.cpp
===
--- clang/test/OpenMP/tile_ast_print.cpp
+++ clang/test/OpenMP/tile_ast_print.cpp
@@ -162,4 +162,25 @@
 }
 
 
+// PRINT-LABEL: template  void foo7(int start, int stop, int step) {
+// DUMP-LABEL: FunctionTemplateDecl {{.*}} foo7
+template 
+void foo7(int start, int stop, int step) {
+  // PRINT: #pragma omp tile sizes(Tile)
+  // DUMP:  OMPTileDirective
+  // DUMP-NEXT:   OMPSizesClause
+  // DUMP-NEXT: DeclRefExpr {{.*}} 'Tile' 'int'
+  #pragma omp tile sizes(Tile)
+// PRINT-NEXT:  for (int i = start; i < stop; i += step)
+// DUMP-NEXT: ForStmt
+for (int i = start; i < stop; i += step)
+  // PRINT-NEXT: body(i);
+  // DUMP:  CallExpr
+  body(i);
+}
+void tfoo7() {
+  foo7<5>(0, 42, 2);
+}
+
+
 #endif
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2227,6 +2227,7 @@
 void ASTStmtWriter::VisitOMPLoopTransformationDirective(
 OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+  Record.writeUInt32(D->getNumGeneratedLoops());
 }
 
 void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -2327,6 +2327,7 @@
 void ASTStmtReader::VisitOMPLoopTransformationDirective(
 OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+  D->setNumGeneratedLoops(Record.readUInt32());
 }
 
 void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -12919,10 +12919,12 @@
   Body, OriginalInits))
 return StmtError();
 
+  unsigned NumGeneratedLoops = PartialClause ? 1 : 0;
+
   // Delay unrolling to when template is completely instantiated.
   if (CurContext->isDependentContext())
 return OMPUnrollDirective::Create(Context, StartLoc, EndLoc, Clauses, AStmt,
-  nullptr, nullptr);
+   

[PATCH] D111119: [Clang][OpenMP] Infix OMPLoopTransformationDirective abstract class. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 377099.
Meinersbur added a comment.

Re-sync


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D19/new/

https://reviews.llvm.org/D19

Files:
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/StmtNodes.td
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2046,6 +2046,8 @@
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
   void VisitOMPParallelDirective(const OMPParallelDirective *D);
   void VisitOMPSimdDirective(const OMPSimdDirective *D);
+  void
+  VisitOMPLoopTransformationDirective(const OMPLoopTransformationDirective *D);
   void VisitOMPTileDirective(const OMPTileDirective *D);
   void VisitOMPUnrollDirective(const OMPUnrollDirective *D);
   void VisitOMPForDirective(const OMPForDirective *D);
@@ -2901,12 +2903,17 @@
   VisitOMPLoopDirective(D);
 }
 
-void EnqueueVisitor::VisitOMPTileDirective(const OMPTileDirective *D) {
+void EnqueueVisitor::VisitOMPLoopTransformationDirective(
+const OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
 }
 
+void EnqueueVisitor::VisitOMPTileDirective(const OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
+}
+
 void EnqueueVisitor::VisitOMPUnrollDirective(const OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
 }
 
 void EnqueueVisitor::VisitOMPForDirective(const OMPForDirective *D) {
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2224,13 +2224,18 @@
   Code = serialization::STMT_OMP_SIMD_DIRECTIVE;
 }
 
-void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
+void ASTStmtWriter::VisitOMPLoopTransformationDirective(
+OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+}
+
+void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
   Code = serialization::STMT_OMP_TILE_DIRECTIVE;
 }
 
 void ASTStmtWriter::VisitOMPUnrollDirective(OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
   Code = serialization::STMT_OMP_UNROLL_DIRECTIVE;
 }
 
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -2324,12 +2324,17 @@
   VisitOMPLoopDirective(D);
 }
 
-void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
+void ASTStmtReader::VisitOMPLoopTransformationDirective(
+OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
 }
 
+void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
+}
+
 void ASTStmtReader::VisitOMPUnrollDirective(OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
 }
 
 void ASTStmtReader::VisitOMPForDirective(OMPForDirective *D) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -3823,13 +3823,8 @@
 VisitSubCaptures(S);
   }
 
-  void VisitOMPTileDirective(OMPTileDirective *S) {
-// #pragma omp tile does not introduce data sharing.
-VisitStmt(S);
-  }
-
-  void VisitOMPUnrollDirective(OMPUnrollDirective *S) {
-// #pragma omp unroll does not introduce data sharing.
+  void VisitOMPLoopTransformationDirective(OMPLoopTransformationDirective *S) {
+// Loop transformation directives do not introduce data sharing
 VisitStmt(S);
   }
 
@@ -9050,15 +9045,8 @@
 }
 return false;
   },
-  [&SemaRef, &Captures](OMPLoopBasedDirective *Transform) {
-Stmt *DependentPreInits;
-if (auto *Dir = dyn_cast(Transform)) {
-  DependentPreInits = Dir->getPreInits();
-} else if (auto *Dir = dyn_cast(Transform)) {
-  DependentPreInits = Dir->getPreInits();
-} else {
-  llvm_unreachable("Unexpected loop transformation");
-}
+  [&SemaRef, &Captures](OMPLoopTransformationDirective *Transform) {
+Stmt *DependentPreInits = Transform->getPreInits();
 if (!DependentPreInits)
   return;
 for (Decl *C : cast(DependentPreInits)->getDeclGroup()) {
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp

[PATCH] D111119: [Clang][OpenMP] Infix OMPLoopTransformationDirective abstract class. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: ABataev, jdenny.
Meinersbur added projects: OpenMP, clang.
Herald added subscribers: dexonsmith, arphaman, guansong, yaxunl.
Meinersbur requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

Insert OMPLoopTransformationDirective between OMPLoopBasedDirective and the 
loop transformations OMPTileDirective and OMPUnrollDirective. This simplifies 
handling of loop transformations not requiring distinguishing between 
OMPTileDirective and OMPUnrollDirective anymore.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D19

Files:
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/StmtNodes.td
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2046,6 +2046,8 @@
   void VisitOMPLoopDirective(const OMPLoopDirective *D);
   void VisitOMPParallelDirective(const OMPParallelDirective *D);
   void VisitOMPSimdDirective(const OMPSimdDirective *D);
+  void
+  VisitOMPLoopTransformationDirective(const OMPLoopTransformationDirective *D);
   void VisitOMPTileDirective(const OMPTileDirective *D);
   void VisitOMPUnrollDirective(const OMPUnrollDirective *D);
   void VisitOMPForDirective(const OMPForDirective *D);
@@ -2901,12 +2903,17 @@
   VisitOMPLoopDirective(D);
 }
 
-void EnqueueVisitor::VisitOMPTileDirective(const OMPTileDirective *D) {
+void EnqueueVisitor::VisitOMPLoopTransformationDirective(
+const OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
 }
 
+void EnqueueVisitor::VisitOMPTileDirective(const OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
+}
+
 void EnqueueVisitor::VisitOMPUnrollDirective(const OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
 }
 
 void EnqueueVisitor::VisitOMPForDirective(const OMPForDirective *D) {
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2224,13 +2224,18 @@
   Code = serialization::STMT_OMP_SIMD_DIRECTIVE;
 }
 
-void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
+void ASTStmtWriter::VisitOMPLoopTransformationDirective(
+OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
+}
+
+void ASTStmtWriter::VisitOMPTileDirective(OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
   Code = serialization::STMT_OMP_TILE_DIRECTIVE;
 }
 
 void ASTStmtWriter::VisitOMPUnrollDirective(OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
   Code = serialization::STMT_OMP_UNROLL_DIRECTIVE;
 }
 
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -2324,12 +2324,17 @@
   VisitOMPLoopDirective(D);
 }
 
-void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
+void ASTStmtReader::VisitOMPLoopTransformationDirective(
+OMPLoopTransformationDirective *D) {
   VisitOMPLoopBasedDirective(D);
 }
 
+void ASTStmtReader::VisitOMPTileDirective(OMPTileDirective *D) {
+  VisitOMPLoopTransformationDirective(D);
+}
+
 void ASTStmtReader::VisitOMPUnrollDirective(OMPUnrollDirective *D) {
-  VisitOMPLoopBasedDirective(D);
+  VisitOMPLoopTransformationDirective(D);
 }
 
 void ASTStmtReader::VisitOMPForDirective(OMPForDirective *D) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -3823,13 +3823,8 @@
 VisitSubCaptures(S);
   }
 
-  void VisitOMPTileDirective(OMPTileDirective *S) {
-// #pragma omp tile does not introduce data sharing.
-VisitStmt(S);
-  }
-
-  void VisitOMPUnrollDirective(OMPUnrollDirective *S) {
-// #pragma omp unroll does not introduce data sharing.
+  void VisitOMPLoopTransformationDirective(OMPLoopTransformationDirective *S) {
+// Loop transformation directives do not introduce data sharing
 VisitStmt(S);
   }
 
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1829,9 +1829,7 @@
 return;
   }
   if (SimplifiedS == NextLoop) {
-if (auto *Dir = dyn_cast(SimplifiedS))
-  SimplifiedS = Dir->getTransformedStmt();
-if (auto *Dir = dyn_cast(SimplifiedS))
+  

[PATCH] D110037: [X86] Always check the size of SourceTy before getting the next type

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This fixed the builder: http://meinersbur.de:8011/#/builders/76/builds/803


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110037/new/

https://reviews.llvm.org/D110037

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110037: [X86] Always check the size of SourceTy before getting the next type

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110037/new/

https://reviews.llvm.org/D110037

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109607: [X86] Refactor GetSSETypeAtOffset to fix pr51813

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D109607#3009377 , @pengfei wrote:

> In D109607#3008486 , @Meinersbur 
> wrote:
>
>> However, my other one that is connected to lab.llvm.org has failed as well 
>> and should have sent an email: 
>> https://lab.llvm.org/buildbot/#/builders/102/builds/2722. Unfortunately it 
>> is slow and packing to many commits together, which I am trying to improve: 
>> D110048 
>
> I didn't receive it either. I once suspected my mailbox but haven't had any 
> fortune for now. :(

Might have because that somehow the master thinks the job before the one that 
is breaking is still building, ie. cannot identify whether the failure is new.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109607/new/

https://reviews.llvm.org/D109607

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110037: [X86] Always check the size of SourceTy before getting the next type

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D110037#3008292 , @pengfei wrote:

> What do you think?

The test-suite is an end-to-end/integration test while tests monorepository are 
unit/regression tests. They have different purposes.
(Personally, I would prefer to reduce the amount of unit/regression testing 
that can already be covered by the test-suite, but that would be a bigger story)




Comment at: clang/test/CodeGen/X86/va-arg-sse.c:2
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 %s -O2 -emit-llvm -o - -triple x86_64-unknown-unknown | 
FileCheck %s
 

lebedev.ri wrote:
> Please don't use `-O*` in clang irgen tests.
> This should *only* test what IR is produced by clang itself.
I agree here, testing `-O*` output will break easily with any unrelated change 
in LLVM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110037/new/

https://reviews.llvm.org/D110037

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109607: [X86] Refactor GetSSETypeAtOffset to fix pr51813

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D109607#3008290 , @pengfei wrote:

> By the way, does this bot send notification to authors when it fails? I 
> didn't receive this fail, so I'm not aware of it at the first time.

This is my personal staging buildbot server to park my worker until my zorg 
patches are greenlit. Hence, I disabled sending any email notification. 
However, my other one that is connected to lab.llvm.org has failed as well and 
should have sent an email: 
https://lab.llvm.org/buildbot/#/builders/102/builds/2722. Unfortunately it is 
slow and packing to many commits together, which I am trying to improve: 
D110048 

Independent of that, it seems no other builder running the test-suite on x86_64 
(there are armv7, s390x and ppc64le ones)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109607/new/

https://reviews.llvm.org/D109607

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109607: [X86] Refactor GetSSETypeAtOffset to fix pr51813

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This patch seem to have broken `GCC-C-execute-pr44575` from the 
llvm-test-suite. See http://meinersbur.de:8011/#/builders/76/builds/761 (this 
builder compiles with Polly, but it also crashes without Polly)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109607/new/

https://reviews.llvm.org/D109607

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/test/Driver/openmp-offload-gpu.c:178
 // CHK-BCLIB-USER: 
clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-builtin-bitcode{{.*}}libomptarget-nvptx-test.bc
-
+// CHK-BCLIB-USER-DIR: 
clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-builtin-bitcode{{.*}}libomptarget{{.}}libomptarget-nvptx-sm_35.bc
 // CHK-BCLIB-NOT: {{error:|warning:}}

`-###` escapes backslashes in Windows paths UNIX-style (which actually is 
unnecessary/wrong, the cmd.exe escape character is `^`, but windows also [[ 
https://stackoverflow.com/q/33027024 | ignores consecutive slashes ]]), so the 
actual output is
```
"-mlink-builtin-bitcode" 
"[...]\\clang\\test\\Driver/Inputs/libomptarget\\libomptarget-nvptx-sm_35.bc"
```

The regex `{{.}}` does not capture the double backslash. `{{/|}}` would 
work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109057/new/

https://reviews.llvm.org/D109057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

Accepting patch since no reaction from @fghanim


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107430/new/

https://reviews.llvm.org/D107430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107764: [OpenMP][OpenMPIRBuilder] Implement loop unrolling.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2055
+/// Attach loop metadata \p Properties to the loop described by \p Loop. If the
+/// loop already has metadata, the loop properties are appended.
+static void addLoopMetadata(CanonicalLoopInfo *Loop,

kiranchandramohan wrote:
> Nit: In the body, it seems we are prepending.
In the body, first we append `Existing->operands()`, then `Properties`. I.e. we 
append the Properties passes as an argument to after the existing properties to 
form a new list `NewLoopProperties`.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2168-2169
+  // actually unrolls the loop.
+  UP.Threshold *= 1.5;
+  UP.PartialThreshold *= 1.5;
+

kiranchandramohan wrote:
> Should this be settable for experimentation or additional control? If not can 
> you provide an explanation for these values?
Made them configurable using the new 
`-openmp-ir-builder-unroll-threshold-factor` switch.

I selected 1.5 as default to make it match approximately the unroll factor the 
LoopUnrollPass would derive in an -O3 pass pass pipeline. No large-scale 
experiments, just a loop that LoopUnrollPass would unroll by a factor of 4 (out 
of possible: 1 (no unrolling), 2, 4, 8) and make `#pragma omp unroll partial` 
also unroll by a factor of 4.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2275
+return nullptr;
+  }
+

jdoerfert wrote:
> This code matches the pattern we have in the 2 members above. Can we make it 
> a member as well and make it clear in the name of all 3 that we use metadata 
> and the unroller pass? No strong feelings, more of an idea.
I think the API should abstract over how the unrolling is performed, frontends 
should not need to be concerned about how unrolling is actually applied. The 
metadata version is preferred unless we cannot use it because we need the 
unrolled loop's CFG wrapped by CanonicalLoopInfo.

I changed the parameters to make it more idiomatic that the caller only 
receives a CanonicalLoopInfo if requested, and that's the only purpose of the 
4th parameter.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1666
 
+TEST_F(OpenMPIRBuilderTest, UnrollLoopFull) {
+  OpenMPIRBuilder OMPBuilder(*M);

kiranchandramohan wrote:
> Are we not calling ompbuilder finalize or verify because it is only adding 
> metadata?
Added
```
OMPBuilder.finalize();
EXPECT_FALSE(verifyModule(*M, &errs()));
```
to the unroll tests. Thanks for noticing.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1735
+
 TEST_F(OpenMPIRBuilderTest, StaticWorkShareLoop) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;

kiranchandramohan wrote:
> Should we add tests for workshare loops with unroll?
I think this is beyond the scope of these tests here. To test thoroughly, we'd 
have to test all combinations of loop transformations and loop-associated 
directives. The `CanonicalLoopInfo::assertOK()` should already check the 
invariants that ensure that the CLI can be used as input for other 
loop-associated directives. Additionally, the composition is tested with 
clang's tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107764/new/

https://reviews.llvm.org/D107764

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Note that the `OpenMPIRBuilderTest.OrderedDirective` test is still crashing.




Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2156-2157
+
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_EQ(EntryBBTI, nullptr);
+

peixin wrote:
> Meinersbur wrote:
> > Consider emitting a terminator, call `finalize()` and `verifyModule`.
> Why do you want me to emit the terminator? If it is because you think the 
> outlined captured function is not generated due to finalize call, there is no 
> need. Discussed above. Sorry about the misguide.
Without terminator, `verifyModule` will complain about it being missing.  
`verifyModule` should be called to ensure that the emitted IR is well-formed. 
Anyway, you seem to have added it in the last diff update.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107430/new/

https://reviews.llvm.org/D107430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

While manually editing `ordered_codegen.cpp` gives nicer results, re-running 
`update_cc_test_checks.py` would be less work. Your manual changes would be 
also be dropped the next time somebody runs update_cc_test_checks.py and 
commits.

The code seems derived from @fghanim single/master/etc implementation, would be 
nice if they could have a look.

The non-OMPBuilder code emits an outlined `__captured_stmt`, but not with 
OMPBuilder enabled. I assume this is due to the missing `finatlize` call.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5335
+  llvm::BasicBlock &FiniBB) {
+// TODO: The ordered directive with simd clause.
+

Also, out and `assert`/`llvm_unreachable` here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5323-5325
+Range = AssertSuccess(Actions.BuildBinOp(
+nullptr, {}, BO_Add, Range,
+Actions.ActOnIntegerConstant(SourceLocation(), 1).get()));

Haven't really understood why this is necessary, but this new version LGTM.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2154
+  Builder.restoreIP(
+  OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB, false));
+

Did you intend to pass IsThreads=true. Currently it is failing because no 
`__kmpc_ordered` is emitted.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2156-2157
+
+  Value *EntryBBTI = EntryBB->getTerminator();
+  EXPECT_EQ(EntryBBTI, nullptr);
+

Consider emitting a terminator, call `finalize()` and `verifyModule`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107430/new/

https://reviews.llvm.org/D107430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107540: [OMPIRBuilder] Clarify CanonicalLoopInfo. NFC.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb1de32d6ddd9: [OMPIRBuilder] Clarify CanonicalLoopInfo. NFC. 
(authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107540/new/

https://reviews.llvm.org/D107540

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -338,8 +338,8 @@
   llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
   findAllocaInsertPoint(builder, moduleTranslation);
   if (schedule == omp::ClauseScheduleKind::Static) {
-ompBuilder->createStaticWorkshareLoop(ompLoc, loopInfo, allocaIP,
-  !loop.nowait(), chunk);
+ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
+ !loop.nowait(), chunk);
   } else {
 llvm::omp::OMPScheduleType schedType;
 switch (schedule) {
@@ -360,8 +360,8 @@
   break;
 }
 
-ompBuilder->createDynamicWorkshareLoop(ompLoc, loopInfo, allocaIP,
-   schedType, !loop.nowait(), chunk);
+ompBuilder->applyDynamicWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
+  schedType, !loop.nowait(), chunk);
   }
 
   // Continue building IR after the loop. Note that the LoopInfo returned by
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -1586,6 +1586,7 @@
 CanonicalLoopInfo *Loop =
 OMPBuilder.createCanonicalLoop(Loc, LoopBodyGenCB, StartVal, StopVal,
StepVal, IsSigned, InclusiveStop);
+InsertPointTy AfterIP = Loop->getAfterIP();
 
 // Tile the loop.
 Value *TileSizeVal = ConstantInt::get(LCTy, TileSize);
@@ -1594,7 +1595,7 @@
 
 // Set the insertion pointer to after loop, where the next loop will be
 // emitted.
-Builder.restoreIP(Loop->getAfterIP());
+Builder.restoreIP(AfterIP);
 
 // Extract the trip count.
 CanonicalLoopInfo *FloorLoop = GenLoops[0];
@@ -1663,12 +1664,20 @@
   CanonicalLoopInfo *CLI = OMPBuilder.createCanonicalLoop(
   Loc, LoopBodyGen, StartVal, StopVal, StepVal,
   /*IsSigned=*/false, /*InclusiveStop=*/false);
+  BasicBlock *Preheader = CLI->getPreheader();
+  BasicBlock *Body = CLI->getBody();
+  Value *IV = CLI->getIndVar();
+  BasicBlock *ExitBlock = CLI->getExit();
 
   Builder.SetInsertPoint(BB, BB->getFirstInsertionPt());
   InsertPointTy AllocaIP = Builder.saveIP();
 
-  CLI = OMPBuilder.createStaticWorkshareLoop(Loc, CLI, AllocaIP,
- /*NeedsBarrier=*/true);
+  OMPBuilder.applyStaticWorkshareLoop(DL, CLI, AllocaIP, /*NeedsBarrier=*/true);
+
+  BasicBlock *Cond = Body->getSinglePredecessor();
+  Instruction *Cmp = &*Cond->begin();
+  Value *TripCount = Cmp->getOperand(1);
+
   auto AllocaIter = BB->begin();
   ASSERT_GE(std::distance(BB->begin(), BB->end()), 4);
   AllocaInst *PLastIter = dyn_cast(&*(AllocaIter++));
@@ -1680,10 +1689,8 @@
   EXPECT_NE(PUpperBound, nullptr);
   EXPECT_NE(PStride, nullptr);
 
-  auto PreheaderIter = CLI->getPreheader()->begin();
-  ASSERT_GE(
-  std::distance(CLI->getPreheader()->begin(), CLI->getPreheader()->end()),
-  7);
+  auto PreheaderIter = Preheader->begin();
+  ASSERT_GE(std::distance(Preheader->begin(), Preheader->end()), 7);
   StoreInst *LowerBoundStore = dyn_cast(&*(PreheaderIter++));
   StoreInst *UpperBoundStore = dyn_cast(&*(PreheaderIter++));
   StoreInst *StrideStore = dyn_cast(&*(PreheaderIter++));
@@ -1705,15 +1712,15 @@
 
   // Check that the loop IV is updated to account for the lower bound returned
   // by the OpenMP runtime call.
-  BinaryOperator *Add = dyn_cast(&CLI->getBody()->front());
-  EXPECT_EQ(Add->getOperand(0), CLI->getIndVar());
+  BinaryOperator *Add = dyn_cast(&Body->front());
+  EXPECT_EQ(Add->getOperand(0), IV);
   auto *LoadedLowerBound = dyn_cast(Add->getOperand(1));
   ASSERT_NE(LoadedLowerBound, nullptr);
   EXPECT_EQ(LoadedLowerBound->getPointerOperand(), PLowerBound);
 
   // Check that the trip count is updated to account for the lower and upper
   // bounds return by the OpenMP runtime call.
-  auto *AddOne = dyn_cast(CLI->getTripCount());
+  auto *AddOne = dyn_cast(TripCount);
   ASSERT_NE(AddOne, nullptr);
   ASSERT_TRUE(AddOne->isBinaryOp());
   auto *O

[PATCH] D107540: [OMPIRBuilder] Clarify CanonicalLoopInfo. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 366161.
Meinersbur marked 7 inline comments as done.
Meinersbur added a comment.

- Address @ftynse's review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107540/new/

https://reviews.llvm.org/D107540

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -338,8 +338,8 @@
   llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
   findAllocaInsertPoint(builder, moduleTranslation);
   if (schedule == omp::ClauseScheduleKind::Static) {
-ompBuilder->createStaticWorkshareLoop(ompLoc, loopInfo, allocaIP,
-  !loop.nowait(), chunk);
+ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
+ !loop.nowait(), chunk);
   } else {
 llvm::omp::OMPScheduleType schedType;
 switch (schedule) {
@@ -360,8 +360,8 @@
   break;
 }
 
-ompBuilder->createDynamicWorkshareLoop(ompLoc, loopInfo, allocaIP,
-   schedType, !loop.nowait(), chunk);
+ompBuilder->applyDynamicWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
+  schedType, !loop.nowait(), chunk);
   }
 
   // Continue building IR after the loop. Note that the LoopInfo returned by
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -1586,6 +1586,7 @@
 CanonicalLoopInfo *Loop =
 OMPBuilder.createCanonicalLoop(Loc, LoopBodyGenCB, StartVal, StopVal,
StepVal, IsSigned, InclusiveStop);
+InsertPointTy AfterIP = Loop->getAfterIP();
 
 // Tile the loop.
 Value *TileSizeVal = ConstantInt::get(LCTy, TileSize);
@@ -1594,7 +1595,7 @@
 
 // Set the insertion pointer to after loop, where the next loop will be
 // emitted.
-Builder.restoreIP(Loop->getAfterIP());
+Builder.restoreIP(AfterIP);
 
 // Extract the trip count.
 CanonicalLoopInfo *FloorLoop = GenLoops[0];
@@ -1663,12 +1664,20 @@
   CanonicalLoopInfo *CLI = OMPBuilder.createCanonicalLoop(
   Loc, LoopBodyGen, StartVal, StopVal, StepVal,
   /*IsSigned=*/false, /*InclusiveStop=*/false);
+  BasicBlock *Preheader = CLI->getPreheader();
+  BasicBlock *Body = CLI->getBody();
+  Value *IV = CLI->getIndVar();
+  BasicBlock *ExitBlock = CLI->getExit();
 
   Builder.SetInsertPoint(BB, BB->getFirstInsertionPt());
   InsertPointTy AllocaIP = Builder.saveIP();
 
-  CLI = OMPBuilder.createStaticWorkshareLoop(Loc, CLI, AllocaIP,
- /*NeedsBarrier=*/true);
+  OMPBuilder.applyStaticWorkshareLoop(DL, CLI, AllocaIP, /*NeedsBarrier=*/true);
+
+  BasicBlock *Cond = Body->getSinglePredecessor();
+  Instruction *Cmp = &*Cond->begin();
+  Value *TripCount = Cmp->getOperand(1);
+
   auto AllocaIter = BB->begin();
   ASSERT_GE(std::distance(BB->begin(), BB->end()), 4);
   AllocaInst *PLastIter = dyn_cast(&*(AllocaIter++));
@@ -1680,10 +1689,8 @@
   EXPECT_NE(PUpperBound, nullptr);
   EXPECT_NE(PStride, nullptr);
 
-  auto PreheaderIter = CLI->getPreheader()->begin();
-  ASSERT_GE(
-  std::distance(CLI->getPreheader()->begin(), CLI->getPreheader()->end()),
-  7);
+  auto PreheaderIter = Preheader->begin();
+  ASSERT_GE(std::distance(Preheader->begin(), Preheader->end()), 7);
   StoreInst *LowerBoundStore = dyn_cast(&*(PreheaderIter++));
   StoreInst *UpperBoundStore = dyn_cast(&*(PreheaderIter++));
   StoreInst *StrideStore = dyn_cast(&*(PreheaderIter++));
@@ -1705,15 +1712,15 @@
 
   // Check that the loop IV is updated to account for the lower bound returned
   // by the OpenMP runtime call.
-  BinaryOperator *Add = dyn_cast(&CLI->getBody()->front());
-  EXPECT_EQ(Add->getOperand(0), CLI->getIndVar());
+  BinaryOperator *Add = dyn_cast(&Body->front());
+  EXPECT_EQ(Add->getOperand(0), IV);
   auto *LoadedLowerBound = dyn_cast(Add->getOperand(1));
   ASSERT_NE(LoadedLowerBound, nullptr);
   EXPECT_EQ(LoadedLowerBound->getPointerOperand(), PLowerBound);
 
   // Check that the trip count is updated to account for the lower and upper
   // bounds return by the OpenMP runtime call.
-  auto *AddOne = dyn_cast(CLI->getTripCount());
+  auto *AddOne = dyn_cast(TripCount);
   ASSERT_NE(AddOne, nullptr);
   ASSERT_TRUE(AddOne->isBinaryOp());
   auto *One = dyn_cast(AddOne->getOpera

[PATCH] D107540: [OMPBuilder] Clarify CanonicalLoopInfo. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Herald added subscribers: wrengr, Chia-hungDuan, dcaballe, cota, teijeong, 
rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, 
liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, antiagainst, shauheen, 
rriddle, mehdi_amini, hiraditya.
Herald added a reviewer: ftynse.
Meinersbur requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, 
stephenneuendorffer, nicolasvasilache.
Herald added projects: clang, MLIR, LLVM.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107540

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Index: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
===
--- mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -275,10 +275,12 @@
   findAllocaInsertPoint(builder, moduleTranslation);
   llvm::OpenMPIRBuilder::InsertPointTy afterIP;
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
+  // TODO: OpenMPIRBuilder::applyWorkshareLoop should take care of the
+  // scheduling clause.
   if (schedule == omp::ClauseScheduleKind::Static) {
-loopInfo = ompBuilder->createStaticWorkshareLoop(ompLoc, loopInfo, allocaIP,
- !loop.nowait(), chunk);
-afterIP = loopInfo->getAfterIP();
+afterIP = ompBuilder->applyStaticWorkshareLoop(
+ompLoc.DL, loopInfo, allocaIP, !loop.nowait(), chunk);
   } else {
 llvm::omp::OMPScheduleType schedType;
 switch (schedule) {
@@ -299,8 +301,8 @@
   break;
 }
 
-afterIP = ompBuilder->createDynamicWorkshareLoop(
-ompLoc, loopInfo, allocaIP, schedType, !loop.nowait(), chunk);
+afterIP = ompBuilder->applyDynamicWorkshareLoop(
+ompLoc.DL, loopInfo, allocaIP, schedType, !loop.nowait(), chunk);
   }
 
   // Continue building IR after the loop.
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -1586,6 +1586,7 @@
 CanonicalLoopInfo *Loop =
 OMPBuilder.createCanonicalLoop(Loc, LoopBodyGenCB, StartVal, StopVal,
StepVal, IsSigned, InclusiveStop);
+InsertPointTy AfterIP = Loop->getAfterIP();
 
 // Tile the loop.
 Value *TileSizeVal = ConstantInt::get(LCTy, TileSize);
@@ -1594,7 +1595,7 @@
 
 // Set the insertion pointer to after loop, where the next loop will be
 // emitted.
-Builder.restoreIP(Loop->getAfterIP());
+Builder.restoreIP(AfterIP);
 
 // Extract the trip count.
 CanonicalLoopInfo *FloorLoop = GenLoops[0];
@@ -1663,12 +1664,20 @@
   CanonicalLoopInfo *CLI = OMPBuilder.createCanonicalLoop(
   Loc, LoopBodyGen, StartVal, StopVal, StepVal,
   /*IsSigned=*/false, /*InclusiveStop=*/false);
+  BasicBlock *Preheader = CLI->getPreheader();
+  BasicBlock *Body = CLI->getBody();
+  Value *IV = CLI->getIndVar();
+  BasicBlock *ExitBlock = CLI->getExit();
 
   Builder.SetInsertPoint(BB, BB->getFirstInsertionPt());
   InsertPointTy AllocaIP = Builder.saveIP();
 
-  CLI = OMPBuilder.createStaticWorkshareLoop(Loc, CLI, AllocaIP,
- /*NeedsBarrier=*/true);
+  OMPBuilder.applyStaticWorkshareLoop(DL, CLI, AllocaIP, /*NeedsBarrier=*/true);
+
+  BasicBlock *Cond = Body->getSinglePredecessor();
+  Instruction *Cmp = &*Cond->begin();
+  Value *TripCount = Cmp->getOperand(1);
+
   auto AllocaIter = BB->begin();
   ASSERT_GE(std::distance(BB->begin(), BB->end()), 4);
   AllocaInst *PLastIter = dyn_cast(&*(AllocaIter++));
@@ -1680,10 +1689,8 @@
   EXPECT_NE(PUpperBound, nullptr);
   EXPECT_NE(PStride, nullptr);
 
-  auto PreheaderIter = CLI->getPreheader()->begin();
-  ASSERT_GE(
-  std::distance(CLI->getPreheader()->begin(), CLI->getPreheader()->end()),
-  7);
+  auto PreheaderIter = Preheader->begin();
+  ASSERT_GE(std::distance(Preheader->begin(), Preheader->end()), 7);
   StoreInst *LowerBoundStore = dyn_cast(&*(PreheaderIter++));
   StoreInst *UpperBoundStore = dyn_cast(&*(PreheaderIter++));
   StoreInst *StrideStore = dyn_cast(&*(PreheaderIter++));
@@ -1705,15 +1712,15 @@
 
   // Check that the loop IV is updated to account for the lower bound returned
   // by the OpenMP runtime call.
-  BinaryOperator *Add = dyn_cast(&CLI->getBody()->front());
-  EXPECT_EQ(Add->getOperand(0), CLI->getIndVar());
+  BinaryOperator *Add = dyn_cast(&Body->front());
+  EXPECT_EQ(Add->getOperand(0), IV);
   auto *LoadedLowerBou

[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:708
   ///
-  /// \returns The insertion position *after* the master.
+  /// \returns The insertion position *after* the masked.
   InsertPointTy createMasked(const LocationDescription &Loc,

Could you commit these typo fixes separately? That is, just push it to main, no 
Phabricator review required.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:721
   ///
-  /// \returns The insertion position *after* the master.
+  /// \returns The insertion position *after* the critical.
   InsertPointTy createCritical(const LocationDescription &Loc,

Could you commit these typo fixes separately? That is, just push it to main, no 
Phabricator review required.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2125
+
+  AllocaInst *PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+

Could you give this alloca register a name?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107430/new/

https://reviews.llvm.org/D107430

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D104601#2919775 , @tstellar wrote:

> Do we have all the issues fixed in trunk yet or do we need to revert in the 
> release/13.x branch.

All known issues have been fixed on trunk. However, @alexfh said they are going 
to do more thorough testing and @aaron.ballman has not changed their suggestion 
to revert on release/13.x. Since you just reverted on clang-13.x, I'd say the 
decision has been made.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@mstorsjo I've taken all steps required for the resolution suggested 
@aaron.ballman. llvm.org/PR51300 suberseedes llvm.org/PR51261 (release-13.x 
branch is under @tstellard's control). D107183 
 has been pushed to main to fix the 
outstanding issue discovered by @romanovvlad (unfortunately without pre-commit 
review, but given that this might be blocking them, the patch is small and you 
would like the situation to be resolved by then, I think a post-commit review 
is acceptable).

@alexfh Happy to fix any additional issues you may find. The 
-fminimize-whitespace feature unfortunately required some restructuring of the 
the code since the decision on when to insert newline were done in many 
different places using different logics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107183: [Preprocessor] Ensure newline after #pragma introduced by -fms-extensions.

[Michael Kruse via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e2586779ca6: [Preprocessor] Ensure newline after #pragma 
introduced by -fms-extensions. (authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107183/new/

https://reviews.llvm.org/D107183

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/whitespace-ms-extensions.c


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -E -P %s -o - | FileCheck %s
+// RUN: %clang_cc1 -E -P -fms-extensions %s -o - | FileCheck %s 
--check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+text \
+__pragma(PRAGMA) \
+after
+
+before MACRO text
+
+
+// CHECK:  before text __pragma(PRAGMA) after text
+
+// MSEXT:  before text
+// MSEXT-NEXT: #pragma PRAGMA
+// MSEXT-NEXT: after text
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -E -P %s -o - | FileCheck %s
+// RUN: %clang_cc1 -E -P -fms-extensions %s -o - | FileCheck %s --check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+text \
+__pragma(PRAGMA) \
+after
+
+before MACRO text
+
+
+// CHECK:  before text __pragma(PRAGMA) after text
+
+// MSEXT:  before text
+// MSEXT-NEXT: #pragma PRAGMA
+// MSEXT-NEXT: after text
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107183: [Preprocessor] Ensure newline after #pragma introduced by -fms-extensions.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 363334.
Meinersbur added a comment.

- Use %clang_cc1 because %clang adds options according to it triple, i.e. 
-fms-extensions is added implicitly for Windows targets.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107183/new/

https://reviews.llvm.org/D107183

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/whitespace-ms-extensions.c


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -E -P %s -o - | FileCheck %s
+// RUN: %clang_cc1 -E -P -fms-extensions %s -o - | FileCheck %s 
--check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+text \
+__pragma(PRAGMA) \
+after
+
+before MACRO text
+
+
+// CHECK:  before text __pragma(PRAGMA) after text
+
+// MSEXT:  before text
+// MSEXT-NEXT: #pragma PRAGMA
+// MSEXT-NEXT: after text
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -E -P %s -o - | FileCheck %s
+// RUN: %clang_cc1 -E -P -fms-extensions %s -o - | FileCheck %s --check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+text \
+__pragma(PRAGMA) \
+after
+
+before MACRO text
+
+
+// CHECK:  before text __pragma(PRAGMA) after text
+
+// MSEXT:  before text
+// MSEXT-NEXT: #pragma PRAGMA
+// MSEXT-NEXT: after text
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

> Correct -- that would be unfortunate as I believe you were hoping for this to 
> be in Clang 13 for ccache support.

Yes, that would have been the ideal outcome.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107183: [Preprocessor] Ensure newline after #pragma introduced by -fms-extensions.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 363146.
Meinersbur added a comment.

Refine test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107183/new/

https://reviews.llvm.org/D107183

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/whitespace-ms-extensions.c


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,19 @@
+// RUN: %clang -E -P %s -o -| FileCheck %s
+// RUN: %clang -E -P -fms-extensions %s -o -| FileCheck %s --check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+text \
+__pragma(PRAGMA) \
+after
+
+before MACRO text
+
+
+// CHECK:  before text __pragma(PRAGMA) after text
+
+// MSEXT:  before text
+// MSEXT-NEXT: #pragma PRAGMA
+// MSEXT-NEXT: after text
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,19 @@
+// RUN: %clang -E -P %s -o -| FileCheck %s
+// RUN: %clang -E -P -fms-extensions %s -o -| FileCheck %s --check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+text \
+__pragma(PRAGMA) \
+after
+
+before MACRO text
+
+
+// CHECK:  before text __pragma(PRAGMA) after text
+
+// MSEXT:  before text
+// MSEXT-NEXT: #pragma PRAGMA
+// MSEXT-NEXT: after text
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Patch uploaded here: D107183 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107183: [Preprocessor] Ensure newline after #pragma introduced by -fms-extensions.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: aaron.ballman, dblaikie, mstorsjo, romanovvlad, 
alexfh.
Meinersbur added a project: clang.
Meinersbur requested review of this revision.

The -fms-extensions converts __pragma (and _Pragma) into a #pragma that has to 
occur at the beginning of a line and end with a newline. This patch ensures 
that the newline after the #pragma is added even though that 
`ok::isAtStartOfLine() indicated that we should not start a newline.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107183

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/whitespace-ms-extensions.c


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,18 @@
+// RUN: %clang -E -P %s -o -| FileCheck %s
+// RUN: %clang -E -P -fms-extensions %s -o -| FileCheck %s --check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+__pragma(PRAGMA) \
+text
+
+beforetext MACRO aftertext
+
+
+// CHECK:  beforetext __pragma(PRAGMA) text aftertext
+
+// MSEXT:  beforetext
+// MSEXT:  #pragma PRAGMA
+// MSEXT-NEXT: text aftertext
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))


Index: clang/test/Preprocessor/whitespace-ms-extensions.c
===
--- /dev/null
+++ clang/test/Preprocessor/whitespace-ms-extensions.c
@@ -0,0 +1,18 @@
+// RUN: %clang -E -P %s -o -| FileCheck %s
+// RUN: %clang -E -P -fms-extensions %s -o -| FileCheck %s --check-prefix=MSEXT
+
+// -fms-extensions changes __pragma into #pragma
+// Ensure that there is a newline after the #pragma line.
+
+#define MACRO\
+__pragma(PRAGMA) \
+text
+
+beforetext MACRO aftertext
+
+
+// CHECK:  beforetext __pragma(PRAGMA) text aftertext
+
+// MSEXT:  beforetext
+// MSEXT:  #pragma PRAGMA
+// MSEXT-NEXT: text aftertext
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -646,7 +646,9 @@
!Tok.is(tok::annot_module_begin) && !Tok.is(tok::annot_module_end)))
 return;
 
-  if (!RequireSameLine && MoveToLine(Tok, /*RequireStartOfLine=*/false)) {
+  // EmittedDirectiveOnThisLine takes priority over RequireSameLine.
+  if ((!RequireSameLine || EmittedDirectiveOnThisLine) &&
+  MoveToLine(Tok, /*RequireStartOfLine=*/EmittedDirectiveOnThisLine)) {
 if (MinimizeWhitespace) {
   // Avoid interpreting hash as a directive under -fpreprocessed.
   if (Tok.is(tok::hash))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D104601#2916897 , @aaron.ballman 
wrote:

> In D104601#2916887 , @Meinersbur 
> wrote:
>
>> I am working on a fix, but I wouldn't mind reverting.
>
> I would be comfortable reverting the feature for Clang 13 and fixing forward 
> on trunk (unless there's a need to revert from trunk temporarily, of course). 
> Giving the feature a full release to bake isn't a bad outcome.

While this means that ccache can only use this feature when `clang-14` is 
detected, I fully understand.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@romanovvlad This is due to -fms-extensions. It Expands `__Pragma` to `#pragma` 
instead of keeping it a `__Pragma`. This is a one-line fix.

@alexfh This was fixed by D106924 

I would suggest to not revert. Will upload a patch for `-fms-extensions` after 
some cleanup.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I am working on a fix, but I wouldn't mind reverting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106924: [Preprocessor] -E -P: Ensure newline after 8 skipped lines.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D106924#2911145 , @mstorsjo wrote:

> @Meinersbur After landing this, can you coordinate with @tstellar to get an 
> ack for backporting this to the 13.x release branch, which also suffers from 
> the regression?

Release blocker has been created: http://llvm.org/PR51261


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106924/new/

https://reviews.llvm.org/D106924

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106924: [Preprocessor] -E -P: Ensure newline after 8 skipped lines.

[Michael Kruse via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc6b0b16c0f55: [Preprocessor] -E -P: Ensure newline after 8 
skipped lines. (authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106924/new/

https://reviews.llvm.org/D106924

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/line-directive-output-mincol.c
  clang/test/Preprocessor/minimize-whitespace.c
  clang/test/Preprocessor/skip-empty-lines.c

Index: clang/test/Preprocessor/skip-empty-lines.c
===
--- /dev/null
+++ clang/test/Preprocessor/skip-empty-lines.c
@@ -0,0 +1,45 @@
+  int  a ;
+  int  b ;
+// A single empty line
+  int  c ;
+/*
+
+more than 8 empty lines
+(forces a line marker instead of newline padding)
+
+
+
+
+*/
+  int  d ;
+
+// RUN: %clang_cc1 -E %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=LINEMARKERS
+// RUN: %clang_cc1 -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=COLSONLY
+// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCOL
+// RUN: %clang_cc1 -E -P -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
+
+// Check behavior after varying number of lines without emitted tokens.
+
+// LINEMARKERS:   {{^}}# 1 "{{.*}}skip-empty-lines.c" 2
+// LINEMARKERS-NEXT: {{^}}  int a ;
+// LINEMARKERS-NEXT: {{^}}  int b ;
+// LINEMARKERS-EMPTY:
+// LINEMARKERS-NEXT: {{^}}  int c ;
+// LINEMARKERS-NEXT: {{^}}# 14 "{{.*}}skip-empty-lines.c"
+// LINEMARKERS-NEXT: {{^}}  int d ;
+
+// COLSONLY:  {{^}}  int a ;
+// COLSONLY-NEXT: {{^}}  int b ;
+// COLSONLY-NEXT: {{^}}  int c ;
+// COLSONLY-NEXT: {{^}}  int d ;
+
+// MINCOL:  {{^}}# 1 "{{.*}}skip-empty-lines.c" 2
+// MINCOL-NEXT: {{^}}int a;
+// MINCOL-NEXT: {{^}}int b;
+// MINCOL-EMPTY:
+// MINCOL-NEXT: {{^}}int c;
+// MINCOL-NEXT: {{^}}# 14 "{{.*}}skip-empty-lines.c"
+// MINCOL-NEXT: {{^}}int d;
+
+// MINWS: {{^}}int a;int b;int c;int d;
+
Index: clang/test/Preprocessor/minimize-whitespace.c
===
--- clang/test/Preprocessor/minimize-whitespace.c
+++ clang/test/Preprocessor/minimize-whitespace.c
@@ -2,6 +2,12 @@
 // RUN: %clang_cc1 -fminimize-whitespace -E -C %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCCOL
 // RUN: %clang_cc1 -fminimize-whitespace -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
 // RUN: %clang_cc1 -fminimize-whitespace -E -C -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCWS
+// The follow empty lines ensure that a #line directive is emitted instead of newline padding after the RUN comments.
+
+
+
+
+
 
 #define NOT_OMP  omp  something
 #define HASH #
@@ -16,11 +22,11 @@
 f  ;
 
 
-// MINCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCOL:  {{^}}# 15 "{{.*}}minimize-whitespace.c"{{$}}
 // MINCOL:  {{^}}int a;{{$}}
 // MINCOL-NEXT: {{^}}int b;{{$}}
 // MINCOL-NEXT: {{^}}#pragma omp barrier{{$}}
-// MINCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCOL-NEXT: # 17 "{{.*}}minimize-whitespace.c"
 // MINCOL-NEXT: {{^}}x{{$}}
 // MINCOL-NEXT: {{^}}#pragma omp nothing{{$}}
 // MINCOL-NEXT: {{^ }}#pragma omp something{{$}}
@@ -28,11 +34,11 @@
 // MINCOL-NEXT: {{^}}int f;{{$}}
 
 // FIXME: Comments after pragmas disappear, even without -fminimize-whitespace
-// MINCCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCCOL:  {{^}}# 15 "{{.*}}minimize-whitespace.c"{{$}}
 // MINCCOL:  {{^}}int a;/*  span-comment  */{{$}}
 // MINCCOL-NEXT: {{^}}int b;//  line-comment{{$}}
 // MINCCOL-NEXT: {{^}}#pragma omp barrier{{$}}
-// MINCCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCCOL-NEXT: # 17 "{{.*}}minimize-whitespace.c"
 // MINCCOL-NEXT: {{^}}x//  more line-comments{{$}}
 // MINCCOL-NEXT: {{^}}#pragma omp nothing{{$}}
 // MINCCOL-NEXT: {{^ }}#pragma omp something{{$}}
Index: clang/test/Preprocessor/line-directive-output-mincol.c
===
--- clang/test/Preprocessor/line-directive-output-mincol.c
+++ /dev/null
@@ -1,11 +0,0 @@
-// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s -strict-whitespace
-
-// CHECK:  # 6 "{{.*}}line-directive-output-mincol.c"
-// CHECK-NEXT: int x;
-// CHECK-NEXT: int y;
-int x;
-int y;
-// CHECK-NEXT: # 10 "{{.*}}line-directive-output-mincol.c"
-// CHECK-NEXT: int z;
-int z;
-
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -276,20 +276,27 @@
   // otherwise print a #line directive.
   if (CurLine == LineNo) {
 // Nothing to do if we are already on the correct line.
-  }

[PATCH] D106924: [Preprocessor] -E -P: Ensure newline after 8 skipped lines.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 362612.
Meinersbur added a comment.

- Use correct base commit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106924/new/

https://reviews.llvm.org/D106924

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/line-directive-output-mincol.c
  clang/test/Preprocessor/minimize-whitespace.c
  clang/test/Preprocessor/skip-empty-lines.c

Index: clang/test/Preprocessor/skip-empty-lines.c
===
--- /dev/null
+++ clang/test/Preprocessor/skip-empty-lines.c
@@ -0,0 +1,45 @@
+  int  a ;
+  int  b ;
+// A single empty line
+  int  c ;
+/*
+
+more than 8 empty lines
+(forces a line marker instead of newline padding)
+
+
+
+
+*/
+  int  d ;
+
+// RUN: %clang_cc1 -E %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=LINEMARKERS
+// RUN: %clang_cc1 -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=COLSONLY
+// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCOL
+// RUN: %clang_cc1 -E -P -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
+
+// Check behavior after varying number of lines without emitted tokens.
+
+// LINEMARKERS:   {{^}}# 1 "{{.*}}skip-empty-lines.c" 2
+// LINEMARKERS-NEXT: {{^}}  int a ;
+// LINEMARKERS-NEXT: {{^}}  int b ;
+// LINEMARKERS-EMPTY:
+// LINEMARKERS-NEXT: {{^}}  int c ;
+// LINEMARKERS-NEXT: {{^}}# 14 "{{.*}}skip-empty-lines.c"
+// LINEMARKERS-NEXT: {{^}}  int d ;
+
+// COLSONLY:  {{^}}  int a ;
+// COLSONLY-NEXT: {{^}}  int b ;
+// COLSONLY-NEXT: {{^}}  int c ;
+// COLSONLY-NEXT: {{^}}  int d ;
+
+// MINCOL:  {{^}}# 1 "{{.*}}skip-empty-lines.c" 2
+// MINCOL-NEXT: {{^}}int a;
+// MINCOL-NEXT: {{^}}int b;
+// MINCOL-EMPTY:
+// MINCOL-NEXT: {{^}}int c;
+// MINCOL-NEXT: {{^}}# 14 "{{.*}}skip-empty-lines.c"
+// MINCOL-NEXT: {{^}}int d;
+
+// MINWS: {{^}}int a;int b;int c;int d;
+
Index: clang/test/Preprocessor/minimize-whitespace.c
===
--- clang/test/Preprocessor/minimize-whitespace.c
+++ clang/test/Preprocessor/minimize-whitespace.c
@@ -2,6 +2,12 @@
 // RUN: %clang_cc1 -fminimize-whitespace -E -C %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCCOL
 // RUN: %clang_cc1 -fminimize-whitespace -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
 // RUN: %clang_cc1 -fminimize-whitespace -E -C -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCWS
+// The follow empty lines ensure that a #line directive is emitted instead of newline padding after the RUN comments.
+
+
+
+
+
 
 #define NOT_OMP  omp  something
 #define HASH #
@@ -16,11 +22,11 @@
 f  ;
 
 
-// MINCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCOL:  {{^}}# 15 "{{.*}}minimize-whitespace.c"{{$}}
 // MINCOL:  {{^}}int a;{{$}}
 // MINCOL-NEXT: {{^}}int b;{{$}}
 // MINCOL-NEXT: {{^}}#pragma omp barrier{{$}}
-// MINCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCOL-NEXT: # 17 "{{.*}}minimize-whitespace.c"
 // MINCOL-NEXT: {{^}}x{{$}}
 // MINCOL-NEXT: {{^}}#pragma omp nothing{{$}}
 // MINCOL-NEXT: {{^ }}#pragma omp something{{$}}
@@ -28,11 +34,11 @@
 // MINCOL-NEXT: {{^}}int f;{{$}}
 
 // FIXME: Comments after pragmas disappear, even without -fminimize-whitespace
-// MINCCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCCOL:  {{^}}# 15 "{{.*}}minimize-whitespace.c"{{$}}
 // MINCCOL:  {{^}}int a;/*  span-comment  */{{$}}
 // MINCCOL-NEXT: {{^}}int b;//  line-comment{{$}}
 // MINCCOL-NEXT: {{^}}#pragma omp barrier{{$}}
-// MINCCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCCOL-NEXT: # 17 "{{.*}}minimize-whitespace.c"
 // MINCCOL-NEXT: {{^}}x//  more line-comments{{$}}
 // MINCCOL-NEXT: {{^}}#pragma omp nothing{{$}}
 // MINCCOL-NEXT: {{^ }}#pragma omp something{{$}}
Index: clang/test/Preprocessor/line-directive-output-mincol.c
===
--- clang/test/Preprocessor/line-directive-output-mincol.c
+++ /dev/null
@@ -1,11 +0,0 @@
-// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s -strict-whitespace
-
-// CHECK:  # 6 "{{.*}}line-directive-output-mincol.c"
-// CHECK-NEXT: int x;
-// CHECK-NEXT: int y;
-int x;
-int y;
-// CHECK-NEXT: # 10 "{{.*}}line-directive-output-mincol.c"
-// CHECK-NEXT: int z;
-int z;
-
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -276,20 +276,27 @@
   // otherwise print a #line directive.
   if (CurLine == LineNo) {
 // Nothing to do if we are already on the correct line.
-  } else if (!StartedNewLine && (!MinimizeWhitespace || !DisableLineMarkers) &&
- LineNo - CurLine == 1) {
+  } else if (MinimizeWhitesp

[PATCH] D106924: [Preprocessor] -E -P: Ensure newline after 8 skipped lines.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 362611.
Meinersbur added a comment.

- Fix embrassing mistakes in skip-empty-lines test (misspelled "LINEMARKES", 
MINWS<->MINCOL, absolute path)
- Skip-empty-lines now also checks leading whitespace behavior
- Fix typos in comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106924/new/

https://reviews.llvm.org/D106924

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/minimize-whitespace.c
  clang/test/Preprocessor/skip-empty-lines.c

Index: clang/test/Preprocessor/skip-empty-lines.c
===
--- clang/test/Preprocessor/skip-empty-lines.c
+++ clang/test/Preprocessor/skip-empty-lines.c
@@ -1,44 +1,45 @@
-int  a ;
-int  b ;
+  int  a ;
+  int  b ;
 // A single empty line
-int  c ;
+  int  c ;
 /*
 
 more than 8 empty lines
-
+(forces a line marker instead of newline padding)
 
 
 
 
 */
-int  d ;
+  int  d ;
 
-// RUN: %clang_cc1 -E %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=LINEMARKES
+// RUN: %clang_cc1 -E %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=LINEMARKERS
 // RUN: %clang_cc1 -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=COLSONLY
-// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
-// RUN: %clang_cc1 -E -P -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCOL
+// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCOL
+// RUN: %clang_cc1 -E -P -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
 
-// Check behavior after variying number of lines without emitted tokens.
+// Check behavior after varying number of lines without emitted tokens.
 
-// LINEMARKES:   # 1 "{{.*}}skip-empty-lines.c" 2
-// LINEMARKERS-NEXT: int a ;
-// LINEMARKERS-NEXT: int b ;
+// LINEMARKERS:   {{^}}# 1 "{{.*}}skip-empty-lines.c" 2
+// LINEMARKERS-NEXT: {{^}}  int a ;
+// LINEMARKERS-NEXT: {{^}}  int b ;
 // LINEMARKERS-EMPTY:
-// LINEMARKERS-NEXT: int c ;
-// LINEMARKERS-NEXT: # 14 "/c/Users/meinersbur/src/llvm-project/clang/test/Preprocessor/skip-empty-lines.c"
-// LINEMARKERS-NEXT: int d ;
-
-// COLSONLY:  int a ;
-// COLSONLY-NEXT: int b ;
-// COLSONLY-NEXT: int c ;
-// COLSONLY-NEXT: int d ;
-
-// MINWS:  # 1 "{{.*}}skip-empty-lines.c" 2
-// MINWS-NEXT: int a;
-// MINWS-NEXT: int b;
-// MINWS-EMPTY:
-// MINWS-NEXT: int c;
-// MINWS-NEXT: # 14 "{{.*}}skip-empty-lines.c"
-// MINWS-NEXT: int d;
-
-// MINCOL: int a;int b;int c;int d;
+// LINEMARKERS-NEXT: {{^}}  int c ;
+// LINEMARKERS-NEXT: {{^}}# 14 "{{.*}}skip-empty-lines.c"
+// LINEMARKERS-NEXT: {{^}}  int d ;
+
+// COLSONLY:  {{^}}  int a ;
+// COLSONLY-NEXT: {{^}}  int b ;
+// COLSONLY-NEXT: {{^}}  int c ;
+// COLSONLY-NEXT: {{^}}  int d ;
+
+// MINCOL:  {{^}}# 1 "{{.*}}skip-empty-lines.c" 2
+// MINCOL-NEXT: {{^}}int a;
+// MINCOL-NEXT: {{^}}int b;
+// MINCOL-EMPTY:
+// MINCOL-NEXT: {{^}}int c;
+// MINCOL-NEXT: {{^}}# 14 "{{.*}}skip-empty-lines.c"
+// MINCOL-NEXT: {{^}}int d;
+
+// MINWS: {{^}}int a;int b;int c;int d;
+
Index: clang/test/Preprocessor/minimize-whitespace.c
===
--- clang/test/Preprocessor/minimize-whitespace.c
+++ clang/test/Preprocessor/minimize-whitespace.c
@@ -2,7 +2,7 @@
 // RUN: %clang_cc1 -fminimize-whitespace -E -C %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCCOL
 // RUN: %clang_cc1 -fminimize-whitespace -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
 // RUN: %clang_cc1 -fminimize-whitespace -E -C -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCWS
-// The follow empty lines ensure that a #line directive is emitted instead of newline padding after the RUN lines
+// The follow empty lines ensure that a #line directive is emitted instead of newline padding after the RUN comments.
 
 
 
Index: clang/lib/Frontend/PrintPreprocessedOutput.cpp
===
--- clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -294,8 +294,8 @@
 }
 StartedNewLine = true;
   } else if (!StartedNewLine) {
-// If we are not on the correct line and don't need to be line-correct, at
-// least ensure we start on a new line.
+// If we are not on the correct line and don't need to be line-correct,
+// at least ensure we start on a new line.
 OS << '\n';
 StartedNewLine = true;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Proposed fix here: D106924 

The reproducer had another whitespace difference before and after D104601 
: "  typedef __builtin_va_list 
__gnuc_va_list;" instead of " typedef __builtin_va_list __gnuc_va_list;" (two 
spaces before "typedef" instead of one). I assume this is not the issue, 
especially since the source file (`include\vadefs.h`) acutally has two spaces 
before "typedef".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106924: [Preprocessor] -E -P: Ensure newline after 8 skipped lines.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: aaron.ballman, mstorsjo, dblaikie.
Meinersbur added a project: clang.
Meinersbur requested review of this revision.

The implementation of -fminimize-whitespace (D104601 
) revised the logic when to emit newlines. 
There was no case to handle when more than 8 lines were skippped in `-P` 
(DisableLineMarkers) mode and instead fell through the case intended for 
`-fminimize-whitespace`, i.e. emit nothing. This patch will emit one newline in 
this case.

The newline logic is slightly reorganized. The `-fminimize-whitespace` case is 
handled explicitly and the emit at least one newline is the new fallback case. 
Without `DisableLineMarkers`, there is the choice between emitting a line 
marker or up to 8 empty lines. 
The up to 8 newlines likely are fewer characters that a line directive, but in 
`-P` mode this had the paradoxic effect that it would print up to 7 empty 
lines, but none at all if more than 8 lines had to be skipped. With 
`DisableLineMarkers`, we now don't consider printing empty lines (just start a 
new line) which matches gcc's behaviour.

The `line-directive-output-mincol.c` test is replaced with a more comprehensive 
test `skip-empty-lines.c` that also tests the more than 8 skipped lines 
behaviour with a various flag combinations.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106924

Files:
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/test/Preprocessor/line-directive-output-mincol.c
  clang/test/Preprocessor/minimize-whitespace.c
  clang/test/Preprocessor/skip-empty-lines.c

Index: clang/test/Preprocessor/skip-empty-lines.c
===
--- /dev/null
+++ clang/test/Preprocessor/skip-empty-lines.c
@@ -0,0 +1,44 @@
+int  a ;
+int  b ;
+// A single empty line
+int  c ;
+/*
+
+more than 8 empty lines
+
+
+
+
+
+*/
+int  d ;
+
+// RUN: %clang_cc1 -E %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=LINEMARKES
+// RUN: %clang_cc1 -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=COLSONLY
+// RUN: %clang_cc1 -E -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
+// RUN: %clang_cc1 -E -P -fminimize-whitespace %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCOL
+
+// Check behavior after variying number of lines without emitted tokens.
+
+// LINEMARKES:   # 1 "{{.*}}skip-empty-lines.c" 2
+// LINEMARKERS-NEXT: int a ;
+// LINEMARKERS-NEXT: int b ;
+// LINEMARKERS-EMPTY:
+// LINEMARKERS-NEXT: int c ;
+// LINEMARKERS-NEXT: # 14 "/c/Users/meinersbur/src/llvm-project/clang/test/Preprocessor/skip-empty-lines.c"
+// LINEMARKERS-NEXT: int d ;
+
+// COLSONLY:  int a ;
+// COLSONLY-NEXT: int b ;
+// COLSONLY-NEXT: int c ;
+// COLSONLY-NEXT: int d ;
+
+// MINWS:  # 1 "{{.*}}skip-empty-lines.c" 2
+// MINWS-NEXT: int a;
+// MINWS-NEXT: int b;
+// MINWS-EMPTY:
+// MINWS-NEXT: int c;
+// MINWS-NEXT: # 14 "{{.*}}skip-empty-lines.c"
+// MINWS-NEXT: int d;
+
+// MINCOL: int a;int b;int c;int d;
Index: clang/test/Preprocessor/minimize-whitespace.c
===
--- clang/test/Preprocessor/minimize-whitespace.c
+++ clang/test/Preprocessor/minimize-whitespace.c
@@ -2,6 +2,12 @@
 // RUN: %clang_cc1 -fminimize-whitespace -E -C %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCCOL
 // RUN: %clang_cc1 -fminimize-whitespace -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
 // RUN: %clang_cc1 -fminimize-whitespace -E -C -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCWS
+// The follow empty lines ensure that a #line directive is emitted instead of newline padding after the RUN lines
+
+
+
+
+
 
 #define NOT_OMP  omp  something
 #define HASH #
@@ -16,11 +22,11 @@
 f  ;
 
 
-// MINCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCOL:  {{^}}# 15 "{{.*}}minimize-whitespace.c"{{$}}
 // MINCOL:  {{^}}int a;{{$}}
 // MINCOL-NEXT: {{^}}int b;{{$}}
 // MINCOL-NEXT: {{^}}#pragma omp barrier{{$}}
-// MINCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCOL-NEXT: # 17 "{{.*}}minimize-whitespace.c"
 // MINCOL-NEXT: {{^}}x{{$}}
 // MINCOL-NEXT: {{^}}#pragma omp nothing{{$}}
 // MINCOL-NEXT: {{^ }}#pragma omp something{{$}}
@@ -28,11 +34,11 @@
 // MINCOL-NEXT: {{^}}int f;{{$}}
 
 // FIXME: Comments after pragmas disappear, even without -fminimize-whitespace
-// MINCCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCCOL:  {{^}}# 15 "{{.*}}minimize-whitespace.c"{{$}}
 // MINCCOL:  {{^}}int a;/*  span-comment  */{{$}}
 // MINCCOL-NEXT: {{^}}int b;//  line-comment{{$}}
 // MINCCOL-NEXT: {{^}}#pragma omp barrier{{$}}
-// MINCCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCCOL-NEXT: # 17 "{{.*}}minimize-whitespace.c"
 // MINCCOL-NEXT: {{^}}x//  more line-comments{{$}}
 // MINCCOL-NEXT: {{^}}#pragma omp nothing{{$}}
 // MINCCOL-NEXT: {{^ }

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Looking into it...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGae6b4238: [Preprocessor] Implement 
-fminimize-whitespace. (authored by Meinersbur).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/Types.h
  clang/include/clang/Frontend/PreprocessorOutputOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/Types.cpp
  clang/lib/Frontend/PrintPreprocessedOutput.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/test/Preprocessor/comment_save.c
  clang/test/Preprocessor/first-line-indent.c
  clang/test/Preprocessor/hash_line.c
  clang/test/Preprocessor/line-directive-output-mincol.c
  clang/test/Preprocessor/line-directive-output.c
  clang/test/Preprocessor/macro_space.c
  clang/test/Preprocessor/minimize-whitespace-messages.c
  clang/test/Preprocessor/minimize-whitespace.c
  clang/test/Preprocessor/print_line_include.c
  clang/test/Preprocessor/stringize_space.c

Index: clang/test/Preprocessor/stringize_space.c
===
--- clang/test/Preprocessor/stringize_space.c
+++ clang/test/Preprocessor/stringize_space.c
@@ -1,16 +1,18 @@
 // RUN: %clang_cc1 -E %s | FileCheck --strict-whitespace %s
+// RUN: %clang_cc1 -E -P -fminimize-whitespace %s | FileCheck --strict-whitespace %s --check-prefix=MINWS
 
 #define A(b) -#b  ,  - #b  ,  -# b  ,  - # b
 A()
 
 // CHECK: {{^}}-"" , - "" , -"" , - ""{{$}}
-
+// MINWS: {{^}}-"",-"",-"",-""
 
 #define t(x) #x
 t(a
 c)
 
 // CHECK: {{^}}"a c"{{$}}
+// MINWS-SAME: "a c"
 
 #define str(x) #x
 #define f(x) str(-x)
@@ -18,6 +20,7 @@
 1)
 
 // CHECK: {{^}}"-1"
+// MINWS-SAME: "-1"
 
 #define paste(a,b) str(a&1 | FileCheck %s --strict-whitespace --check-prefix=MINCOL
+// RUN: %clang_cc1 -fminimize-whitespace -E -C %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCCOL
+// RUN: %clang_cc1 -fminimize-whitespace -E -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINWS
+// RUN: %clang_cc1 -fminimize-whitespace -E -C -P %s 2>&1 | FileCheck %s --strict-whitespace --check-prefix=MINCWS
+
+#define NOT_OMP  omp  something
+#define HASH #
+
+  int  a; /*  span-comment  */
+  int  b  ;   //  line-comment
+  _Pragma  (  "omp  barrier"  ) x //  more line-comments
+  #pragma  omp  nothing  //  another comment
+HASH  pragma  NOT_OMP
+  int  e;// again a line
+  int  \
+f  ;
+
+
+// MINCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCOL:  {{^}}int a;{{$}}
+// MINCOL-NEXT: {{^}}int b;{{$}}
+// MINCOL-NEXT: {{^}}#pragma omp barrier{{$}}
+// MINCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCOL-NEXT: {{^}}x{{$}}
+// MINCOL-NEXT: {{^}}#pragma omp nothing{{$}}
+// MINCOL-NEXT: {{^ }}#pragma omp something{{$}}
+// MINCOL-NEXT: {{^}}int e;{{$}}
+// MINCOL-NEXT: {{^}}int f;{{$}}
+
+// FIXME: Comments after pragmas disappear, even without -fminimize-whitespace
+// MINCCOL:  {{^}}# 9 "{{.*}}minimize-whitespace.c"{{$}}
+// MINCCOL:  {{^}}int a;/*  span-comment  */{{$}}
+// MINCCOL-NEXT: {{^}}int b;//  line-comment{{$}}
+// MINCCOL-NEXT: {{^}}#pragma omp barrier{{$}}
+// MINCCOL-NEXT: # 11 "{{.*}}minimize-whitespace.c"
+// MINCCOL-NEXT: {{^}}x//  more line-comments{{$}}
+// MINCCOL-NEXT: {{^}}#pragma omp nothing{{$}}
+// MINCCOL-NEXT: {{^ }}#pragma omp something{{$}}
+// MINCCOL-NEXT: {{^}}int e;// again a line{{$}}
+// MINCCOL-NEXT: {{^}}int f;{{$}}
+
+// MINWS:  {{^}}int a;int b;{{$}}
+// MINWS-NEXT: {{^}}#pragma omp barrier{{$}}
+// MINWS-NEXT: {{^}}x{{$}}
+// MINWS-NEXT: {{^}}#pragma omp nothing{{$}}
+// MINWS-NEXT: {{^ }}#pragma omp something int e;int f;{{$}}
+
+// FIXME: Comments after pragmas disappear, even without -fminimize-whitespace
+// MINCWS:  {{^}}int a;/*  span-comment  */int b;//  line-comment{{$}}
+// MINCWS-NEXT: {{^}}#pragma omp barrier{{$}}
+// MINCWS-NEXT: {{^}}x//  more line-comments{{$}}
+// MINCWS-NEXT: {{^}}#pragma omp nothing{{$}}
+// MINCWS-NEXT: {{^ }}#pragma omp something int e;// again a line{{$}}
+// MINCWS-NEXT: {{^}}int f;
+
Index: clang/test/Preprocessor/minimize-whitespace-messages.c
===
--- /dev/null
+++ clang/test/Preprocessor/minimize-whitespace-messages.c
@@ -0,0 +1,8 @@
+// RUN: not %clang -c -fminimize-whitespace %s 2>&1 | FileCheck %s --check-prefix=ON
+// ON: error: invalid argument '-fminimize-whitespace' only allowed with '-E'
+
+// RUN: not %clang -c -fno-minimize-whitespace %s 2>&1 | FileCheck %s  --check-prefix=OFF
+// OFF: error: invalid argument '-fno-minimize-whitespace' only allowed with '-E'
+
+// RUN: not %clang -E -fminimize-whitespace -x assembler-with-cpp %s 2>&1 | FileCheck %s --check-prefix=ASM
+// 

[PATCH] D104601: [Preprocessor] Implement -fminimize-whitespace.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Because of how large the switch construct would have been, I created a new 
function `types::isDerivedFromC` together with the other functions. These 
commonly have default-cases so compilers would not warn when `Types.def` is 
amended, but `isDerivedFromC` can be found between other functions that would 
need to be updated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104601/new/

https://reviews.llvm.org/D104601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits