[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+line of code. Some common editors will automatically remove trailing whitespace
+when saving a file which causes unrelated changes to appear in diffs and
+commits.

JDevlieghere wrote:
> chandlerc wrote:
> > Meinersbur wrote:
> > > Just to note that this policy will prohibit the use of remove 
> > > trailing-whitespace feature in editors since it makes it impossible to 
> > > edit the file without also 'editing' any unrelated whitespace. At the 
> > > same time, since not being able to use the feature, I risk committing 
> > > trailing whitespace myself (that is, some additional steps are necessary: 
> > > I use 'git clang-format' which should remove traling whitespace only on 
> > > edited lines and 'git diff' shows trailing whitespace in red; these are 
> > > still additional steps that are easy to miss)
> > I also have editor settings that risk violating this, but I just reduce my 
> > patdh before submitting. This is a touch annoying with svn, but with got it 
> > is pretty simple to use `git add -p` and ignore the unnecessary removals if 
> > trailing whitespace 
> I had the same workflow as Chandler but that became rather tedious for the 
> LLDB repo where there's a lot of trailing whitespace. I ended up adding an 
> alias to my git config that only stages non-whitespace changes: `anw = !sh -c 
> 'git diff -U0 -w --no-color "$@" | git apply --cached --ignore-whitespace 
> --unidiff-zero -'`. It's far from optimal but it works pretty well. 
Thank you for sharing.


https://reviews.llvm.org/D50055



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

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: lib/AST/DeclBase.cpp:854-859
+  auto I = Attrs.begin(), E = Attrs.end();
+  for (; I != E; ++I) {
+if (!(*I)->isInherited())
+  break;
+  }
+  Attrs.insert(I, A);

aaron.ballman wrote:
> The unfortunate part about this is that inherited attributes are fairly 
> common, so this extra searching may happen more often than we'd like. I 
> wonder how bad it would be to keep track of the location within the list 
> where the inherited attributes start. Then again, the list of attributes 
> should be relatively short in most cases, so this search isn't likely to be 
> too expensive.
> 
> Having some performance measurements might help decide this, too.
Timed clang-compilation using perf (`perf stat bin/clang 
llvm/unittests/IR/InstructionsTest.cpp ...`), before this patch (r339574):
```
   7647.415800  task-clock (msec) #0.983 CPUs utilized
   289  context-switches  #0.038 K/sec
26  cpu-migrations#0.003 K/sec
86,494  page-faults   #0.011 M/sec
19,068,741,863  cycles#2.493 GHz
26,581,355,844  instructions  #1.39  insn per cycle
 6,242,394,037  branches  #  816.275 M/sec
   128,405,656  branch-misses #2.06% of all branches

   7.779848330 seconds time elapsed
```

With this patch:
```
   7679.173467  task-clock (msec) #0.987 CPUs utilized
   321  context-switches  #0.042 K/sec
23  cpu-migrations#0.003 K/sec
86,071  page-faults   #0.011 M/sec
19,150,248,538  cycles#2.494 GHz
26,667,465,697  instructions  #1.39  insn per cycle
 6,262,381,898  branches  #  815.502 M/sec
   128,527,669  branch-misses #2.05% of all branches

   7.779477815 seconds time elapsed
```
(Intel(R) Xeon(R) Platinum 8180M CPU @ 2.50GHz)

The vector insert operation is also be `O(n)`. If the number of non-inherited 
is expected to be smaller, we can instead search for the first inherited 
attribute starting at the end of the vector. If we want to avoid the `O(n)` 
entirely, we need one list for inherited and another for non-inherited 
attributes.


Repository:
  rC Clang

https://reviews.llvm.org/D50214



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

[PATCH] D50805: [Sema] Don't warn on returning the address of a label

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D50805#1201956, @Quuxplusone wrote:

> If you added a new option `-Wret-addr-label` as suggested above (for a total 
> patch of +2 lines), then is it accurate to say:
>
> - if `-Wret-addr-label` was enabled by default, we know of at least one 
> codebase that would pass `-Wno-ret-addr-label` to their build
> - if `-Wret-addr-label` was disabled by default, we don't know of any 
> codebases that would voluntarily enable it And if nobody would enable it 
> voluntarily... might as well eliminate it, right?


That nobody here can name a project that would enable it, does not mean that 
there is none, or that projects will decide in the future to use it, or that 
individual developers temporarily use. Besides, it'd be enabled by 
`-Weverything`.


Repository:
  rC Clang

https://reviews.llvm.org/D50805



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This is straightforward in that it clones the implementation of `#pragma 
unroll` for `unroll_and_jam`.

Hence, it also shares the same problem of clang's LoopHints, namely that the 
order of loop transformations (if there are multiple on the same loop: loop 
distribution, vectorization, etc) is defined by the order of the passes in the 
pass pipeline, which should be an implementation detail.

I am currently working on this topic. Could we maybe disable the `#pragma clang 
loop unroll_and_jam` spelling ftm to avoid compatibility issues? However, the 
problem already exists for the other loop hints, so I will have to ensure 
compatibility with those anyway.


https://reviews.llvm.org/D47267



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D47267#1109912, @dmgreen wrote:

> > Could we maybe disable the #pragma clang loop unroll_and_jam spelling ftm 
> > to avoid compatibility issues?
>
> Sure, I'm not against. It sounds like you have opinions on how this should 
> work. That's good. If there are multiple clang loop pragma's, what is the 
> expected behaviour there?
>
> In the llvm side of this, in the unroll and jam pass, I made it so that a 
> loop with "llvm.loop.unroll" metadata without any "llvm.loop.unroll_and_jam" 
> metadata will not do unroll and jam, it will leave the loop for the unroller. 
> On the expectation that the use really wants to unroll (and it applies to 
> llvm.loop.unroll.disable too, disabling unroll and jam as well as unroll). I 
> haven't done anything with other loop metadata though.


I'd expect that transformations "stack up". E.g.

  #pragma unroll_and_jam
  #pragma unroll(4)

which I'd expect to unroll by a factor of 4 first, then try to unroll-and-jam 
(which I am not sure https://reviews.llvm.org/D41953 can do due to then 
containing 4 loops). On the other hand

  #pragma unroll(4)
  #pragma unroll_and_jam

should unroll-and-jam, and then unroll the outer loop by a factor of 4.


https://reviews.llvm.org/D47267



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Yes, this is what I had in mind. Thank you.

I am preparing an RFC on what I am trying to do. This should clarify our goals 
and to what extend `#pragma clang loop` conflicts with it.


https://reviews.llvm.org/D47267



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

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, amusman, ABataev, tyler.nowicki.
Meinersbur added a dependency: D52116: Introduce llvm.loop.parallel_accesses 
and llvm.access.group metadata..

Instead of generating llvm.mem.parallel_loop_access metadata, generate 
llvm.access.group on instructions and llvm.loop.parallel_accesses on loops. 
Minimize the number of access groups by only creating one for loops that are 
parallel.

This is clang part of https://reviews.llvm.org/D52116.


Repository:
  rC Clang

https://reviews.llvm.org/D52117

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/OpenMP/for_codegen.cpp
  test/OpenMP/for_simd_codegen.cpp
  test/OpenMP/loops_explicit_clauses_codegen.cpp
  test/OpenMP/ordered_codegen.cpp
  test/OpenMP/parallel_for_simd_codegen.cpp
  test/OpenMP/schedule_codegen.cpp
  test/OpenMP/simd_codegen.cpp
  test/OpenMP/simd_metadata.c
  test/OpenMP/target_parallel_for_simd_codegen.cpp
  test/OpenMP/target_simd_codegen.cpp
  test/OpenMP/taskloop_simd_codegen.cpp

Index: test/OpenMP/taskloop_simd_codegen.cpp
===
--- test/OpenMP/taskloop_simd_codegen.cpp
+++ test/OpenMP/taskloop_simd_codegen.cpp
@@ -83,17 +83,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP1]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK2]](
@@ -113,17 +113,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP2]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK3]](
@@ -142,7 +142,7 @@
 // CHECK: [[LB_VAL:%.+]] = load i64, i64* [[LB]],
 // CHECK: store i64 [[LB_VAL]], i64* [[CNT:%.+]],
 // CHECK: br label
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
@@ -192,14 +192,14 @@
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: store i32 %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
 // CHECK: store i32 %{{.+}}, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHE

[PATCH] D52118: [Loopinfo] Remove one latch case in getLoopID. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, jdoerfert.

getLoopID has different control flow for two cases: If there is a single loop 
latch and for any other number of loop latches (0 and more than one). The 
latter case should return the same result if there is a single latch. We can 
save an iteration over the loop's basic blocks (which is what getLoopLatch 
does) by handling both cases with the same code.


Repository:
  rC Clang

https://reviews.llvm.org/D52118

Files:
  lib/Analysis/LoopInfo.cpp


Index: lib/Analysis/LoopInfo.cpp
===
--- lib/Analysis/LoopInfo.cpp
+++ lib/Analysis/LoopInfo.cpp
@@ -213,26 +213,21 @@
 
 MDNode *Loop::getLoopID() const {
   MDNode *LoopID = nullptr;
-  if (BasicBlock *Latch = getLoopLatch()) {
-LoopID = Latch->getTerminator()->getMetadata(LLVMContext::MD_loop);
-  } else {
-assert(!getLoopLatch() &&
-   "The loop should have no single latch at this point");
-// Go through the latch blocks and check the terminator for the metadata.
-SmallVector LatchesBlocks;
-getLoopLatches(LatchesBlocks);
-for (BasicBlock *BB : LatchesBlocks) {
-  TerminatorInst *TI = BB->getTerminator();
-  MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
-
-  if (!MD)
-return nullptr;
-
-  if (!LoopID)
-LoopID = MD;
-  else if (MD != LoopID)
-return nullptr;
-}
+
+  // Go through the latch blocks and check the terminator for the metadata.
+  SmallVector LatchesBlocks;
+  getLoopLatches(LatchesBlocks);
+  for (BasicBlock *BB : LatchesBlocks) {
+TerminatorInst *TI = BB->getTerminator();
+MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
+
+if (!MD)
+  return nullptr;
+
+if (!LoopID)
+  LoopID = MD;
+else if (MD != LoopID)
+  return nullptr;
   }
   if (!LoopID || LoopID->getNumOperands() == 0 ||
   LoopID->getOperand(0) != LoopID)


Index: lib/Analysis/LoopInfo.cpp
===
--- lib/Analysis/LoopInfo.cpp
+++ lib/Analysis/LoopInfo.cpp
@@ -213,26 +213,21 @@
 
 MDNode *Loop::getLoopID() const {
   MDNode *LoopID = nullptr;
-  if (BasicBlock *Latch = getLoopLatch()) {
-LoopID = Latch->getTerminator()->getMetadata(LLVMContext::MD_loop);
-  } else {
-assert(!getLoopLatch() &&
-   "The loop should have no single latch at this point");
-// Go through the latch blocks and check the terminator for the metadata.
-SmallVector LatchesBlocks;
-getLoopLatches(LatchesBlocks);
-for (BasicBlock *BB : LatchesBlocks) {
-  TerminatorInst *TI = BB->getTerminator();
-  MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
-
-  if (!MD)
-return nullptr;
-
-  if (!LoopID)
-LoopID = MD;
-  else if (MD != LoopID)
-return nullptr;
-}
+
+  // Go through the latch blocks and check the terminator for the metadata.
+  SmallVector LatchesBlocks;
+  getLoopLatches(LatchesBlocks);
+  for (BasicBlock *BB : LatchesBlocks) {
+TerminatorInst *TI = BB->getTerminator();
+MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
+
+if (!MD)
+  return nullptr;
+
+if (!LoopID)
+  LoopID = MD;
+else if (MD != LoopID)
+  return nullptr;
   }
   if (!LoopID || LoopID->getNumOperands() == 0 ||
   LoopID->getOperand(0) != LoopID)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48808: [CodeGen] Emit parallel_loop_access for each loop in the loop stack.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

ping


Repository:
  rC Clang

https://reviews.llvm.org/D48808



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[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: lib/Sema/SemaStmtAttr.cpp:183
{nullptr, nullptr}};
-
   for (const auto *I : Attrs) {

[nit] unrelated whitespace change?



Comment at: test/CodeGenCXX/pragma-unroll-and-jam.cpp:4
+void unroll_and_jam(int *List, int Length, int Value) {
+  // CHECK: define {{.*}} @_Z14unroll_and_jam
+#pragma unroll_and_jam

[suggestion] `CHECK-LABEL`? (applies to the function names below as well)


https://reviews.llvm.org/D47267



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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

Rebase after de-listifying in r336945


Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/ParsedAttr.h
  lib/AST/ItaniumMangle.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseObjc.cpp
  lib/Sema/SemaAttr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll-and-jam.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/nullability.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaObjC/nullability.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: test/SemaTemplate/attributes.cpp
===
--- test/SemaTemplate/attributes.cpp
+++ test/SemaTemplate/attributes.cpp
@@ -55,11 +55,11 @@
 }
 
 // CHECK: FunctionTemplateDecl {{.*}} HasAnnotations
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK: FunctionDecl {{.*}} HasAnnotations
 // CHECK:   TemplateArgument type 'int'
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -61,8 +61,8 @@
 
 void func_multiple_addr(void) {
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
   __private private_int_t var5; // expected-warning {{multiple identical address spaces specified for type}}
Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -36,14 +36,14 @@
 
 - (nonnull NSFoo **)invalidMethod1; // expected-error{{nullability keyword 'nonnull' cannot be applied to multi-level pointer type 'NSFoo **'}}
 // expected-note@-1{{use nullability type specifier '_Nonnull' to affect the innermost pointer type of 'NSFoo **'}}
-- (nonnull NSFoo * _Nullable)conflictingMethod1; // expected-error{{nullability specifier '_Nullable' conflicts with existing specifier '_Nonnull'}}
-- (nonnull NSFoo * _Nonnull)redundantMethod1; // expected-warning{{duplicate nullability specifier '_Nonnull'}}
+- (nonnull NSFoo * _Nullable)conflictingMethod1; // expected-error{{nullability specifier 'nonnull' conflicts with existing specifier '_Nullable'}}
+- (nonnull NSFoo * _Nonnull)redundantMethod1; // expected-warning{{duplicate nullability specifier 'nonnull'}}
 
 @property(nonnull,retain) NSFoo *property1;
 @property(nullable,assign) NSFoo ** invalidProperty1; // expected-error{{nullability keyword 'nullable' cannot be applied to multi-level pointer type 'NSFoo **'}}
 // expected-note@-1{{use nullability type specifier '_Nullable' to affect the innermost pointer type of 'NSFoo **'}}
-@property(null_unspecified,retain) NSFoo * _Nullable conflictingProperty1; // expected-error{{nullability specifier '_Nullable' conflicts with existing specifier '_Null_unspecified'}}
-@property(retain,nonnull) NSFoo * _Nonnull redundantProperty1; // expected-warning{{duplicate nullability specifier '_Nonnull'}}
+@property(null_unspecified,retain) NSFoo * _Nullable conflictingProperty1; // expected-error{{nullability specifier 'null_unspecified' conflicts with existing specifier '_Nullable'}}
+@property(retain,nonnull) NSFoo * _Nonnull redundantProperty1; // expected-warning{{duplicate nullability specifier 'nonnull'}}
 
 @property(nu

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur reopened this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jrtc27.

Reopen after revert (and to be able to update the diff)


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I am unsure how to proceed. Commit since already accepted? Wait for 
reconfirmation? Open new differential?


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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



Comment at: test/Sema/attr-ownership.c:22
 void f15(int, int)
-  __attribute__((ownership_returns(foo, 1)))  // expected-note {{declared with 
index 1 here}}
-  __attribute__((ownership_returns(foo, 2))); // expected-error 
{{'ownership_returns' attribute index does not match; here it is 2}}
+  __attribute__((ownership_returns(foo, 1)))  // expected-error 
{{'ownership_returns' attribute index does not match; here it is 1}}
+  __attribute__((ownership_returns(foo, 2))); // expected-note {{declared with 
index 2 here}}

erichkeane wrote:
> This seems wrong to me, the 2nd one should be the error condition, right?
This is the result of how attributes are combined. There are the two 
possibilities
```
void f15(int, int) __attribute__((ownership_returns(foo, 1)));
void f15(int, int) __attribute__((ownership_returns(foo, 2)));
```
and
```
void f15(int, int) __attribute__((ownership_returns(foo, 1)))
   __attribute__((ownership_returns(foo, 2)));
```

The error diagnosis seem to have been written with the first case in mind and 
emits in the order there. In the second case attributes merged in the other 
order (which is the naively correct order, see 
https://reviews.llvm.org/D48100#1142865), resulting diagnosis to be emitted in 
the reverse-textual order. There is no consistency in which SourceLocation is 
the first and which one is the second, and many diagnoses are already not 
printed in textual order.

I cannot change this without massively reworking how attributes are processed 
in clang.



Comment at: test/Sema/attr-print.c:25
 
 // TODO: the Type Printer has no way to specify the order to print attributes
 // in, and so it currently always prints them in reverse order. Fix this.

erichkeane wrote:
> This TODO doesn't apply, right?  Or is at least wrong...
Correct, I missed this todo.



Comment at: test/Sema/attr-visibility.c:18
 
-void test6() __attribute__((visibility("hidden"), // expected-note {{previous 
attribute is here}}
-visibility("default"))); // expected-error 
{{visibility does not match previous declaration}}
+void test6() __attribute__((visibility("default"), // expected-error 
{{visibility does not match previous declaration}}
+visibility("hidden"))); // expected-note 
{{previous attribute is here}}

erichkeane wrote:
> This order issue is likely to appear a couple of times I suspect.
See my previous reply and https://reviews.llvm.org/D48100#1142865



Comment at: test/SemaOpenCL/address-spaces.cl:64
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces 
specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces 
specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces 
specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces 
specified for type}}

erichkeane wrote:
> These changes have me concerned... The error message isn't specific, but we 
> have to change the order anyway?
An additional error is emitted with the reverse order (`non-kernel function 
variable cannot be declared in local address space`; a result of which 
attribute 'wins' in error resolution which is again related to which attribute 
is already in the AttributeList). I'd say it is the responsibility diagnosis 
code to emit the same set of messages independent of attribute order which I do 
not try to fix here.


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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

- Remove TODOs about the attribute order


Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/ParsedAttr.h
  lib/AST/ItaniumMangle.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseObjc.cpp
  lib/Sema/SemaAttr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll-and-jam.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/nullability.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaObjC/nullability.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: test/SemaTemplate/attributes.cpp
===
--- test/SemaTemplate/attributes.cpp
+++ test/SemaTemplate/attributes.cpp
@@ -55,11 +55,11 @@
 }
 
 // CHECK: FunctionTemplateDecl {{.*}} HasAnnotations
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK: FunctionDecl {{.*}} HasAnnotations
 // CHECK:   TemplateArgument type 'int'
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -61,8 +61,8 @@
 
 void func_multiple_addr(void) {
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
   __private private_int_t var5; // expected-warning {{multiple identical address spaces specified for type}}
Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -36,14 +36,14 @@
 
 - (nonnull NSFoo **)invalidMethod1; // expected-error{{nullability keyword 'nonnull' cannot be applied to multi-level pointer type 'NSFoo **'}}
 // expected-note@-1{{use nullability type specifier '_Nonnull' to affect the innermost pointer type of 'NSFoo **'}}
-- (nonnull NSFoo * _Nullable)conflictingMethod1; // expected-error{{nullability specifier '_Nullable' conflicts with existing specifier '_Nonnull'}}
-- (nonnull NSFoo * _Nonnull)redundantMethod1; // expected-warning{{duplicate nullability specifier '_Nonnull'}}
+- (nonnull NSFoo * _Nullable)conflictingMethod1; // expected-error{{nullability specifier 'nonnull' conflicts with existing specifier '_Nullable'}}
+- (nonnull NSFoo * _Nonnull)redundantMethod1; // expected-warning{{duplicate nullability specifier 'nonnull'}}
 
 @property(nonnull,retain) NSFoo *property1;
 @property(nullable,assign) NSFoo ** invalidProperty1; // expected-error{{nullability keyword 'nullable' cannot be applied to multi-level pointer type 'NSFoo **'}}
 // expected-note@-1{{use nullability type specifier '_Nullable' to affect the innermost pointer type of 'NSFoo **'}}
-@property(null_unspecified,retain) NSFoo * _Nullable conflictingProperty1; // expected-error{{nullability specifier '_Nullable' conflicts with existing specifier '_Null_unspecified'}}
-@property(retain,nonnull) NSFoo * _Nonnull redundantProperty1; // expected-warning{{duplicate nullability specifier '_Nonnull'}}
+@property(null_unspecified,retain) NSFoo * _Nullable conflictingProperty1; // expected-error{{nullability specifier 'null_unspecified' conflicts with existing specifier '_Nullable'}}
+@property(retain,nonnull) NSFoo * _Nonnull redundantProperty1; // expected-warning{{duplicate n

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338800: Append new attributes to the end of an 
AttributeList. (authored by Meinersbur, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D48100

Files:
  cfe/trunk/include/clang/Sema/ParsedAttr.h
  cfe/trunk/lib/AST/ItaniumMangle.cpp
  cfe/trunk/lib/Parse/ParseDeclCXX.cpp
  cfe/trunk/lib/Parse/ParseObjc.cpp
  cfe/trunk/lib/Sema/SemaAttr.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
  cfe/trunk/test/Index/annotate-comments-availability-attrs.cpp
  cfe/trunk/test/Index/complete-with-annotations.cpp
  cfe/trunk/test/Misc/ast-print-pragmas.cpp
  cfe/trunk/test/PCH/pragma-loop.cpp
  cfe/trunk/test/Parser/pragma-loop-safety.cpp
  cfe/trunk/test/Parser/pragma-loop.cpp
  cfe/trunk/test/Parser/pragma-unroll-and-jam.cpp
  cfe/trunk/test/Parser/pragma-unroll.cpp
  cfe/trunk/test/Sema/attr-availability-tvos.c
  cfe/trunk/test/Sema/attr-availability.c
  cfe/trunk/test/Sema/attr-coldhot.c
  cfe/trunk/test/Sema/attr-disable-tail-calls.c
  cfe/trunk/test/Sema/attr-long-call.c
  cfe/trunk/test/Sema/attr-micromips.c
  cfe/trunk/test/Sema/attr-notail.c
  cfe/trunk/test/Sema/attr-ownership.c
  cfe/trunk/test/Sema/attr-ownership.cpp
  cfe/trunk/test/Sema/attr-print.c
  cfe/trunk/test/Sema/attr-visibility.c
  cfe/trunk/test/Sema/internal_linkage.c
  cfe/trunk/test/Sema/mips-interrupt-attr.c
  cfe/trunk/test/Sema/nullability.c
  cfe/trunk/test/SemaCXX/attr-print.cpp
  cfe/trunk/test/SemaCXX/ms-uuid.cpp
  cfe/trunk/test/SemaObjC/nullability.m
  cfe/trunk/test/SemaOpenCL/address-spaces.cl
  cfe/trunk/test/SemaTemplate/attributes.cpp

Index: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
@@ -2832,36 +2832,25 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  SmallVector AEnableIfs;
-  // Since this is an equality check, we can ignore that enable_if attrs show up
-  // in reverse order.
-  for (const auto *EIA : A->specific_attrs())
-AEnableIfs.push_back(EIA);
-
-  SmallVector BEnableIfs;
-  for (const auto *EIA : B->specific_attrs())
-BEnableIfs.push_back(EIA);
-
-  // Two very common cases: either we have 0 enable_if attrs, or we have an
-  // unequal number of enable_if attrs.
-  if (AEnableIfs.empty() && BEnableIfs.empty())
-return true;
-
-  if (AEnableIfs.size() != BEnableIfs.size())
-return false;
-
+  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (unsigned I = 0, E = AEnableIfs.size(); I != E; ++I) {
+  auto AEnableIfAttrs = A->specific_attrs();
+  auto BEnableIfAttrs = B->specific_attrs();
+  auto AEnableIf = AEnableIfAttrs.begin();
+  auto BEnableIf = BEnableIfAttrs.begin();
+  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
+   ++BEnableIf, ++AEnableIf) {
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIfs[I]->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIfs[I]->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return false;
   }
 
-  return true;
+  // Return false if the number of enable_if attributes was different.
+  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == BEnableIfAttrs.end();
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: cfe/trunk/lib/Parse/ParseDeclCXX.cpp
===
--- cfe/trunk/lib/Parse/ParseDeclCXX.cpp
+++ cfe/trunk/lib/Parse/ParseDeclCXX.cpp
@@ -3875,7 +3875,7 @@
 
   if (!Attrs.empty() &&
   IsBuiltInOrStandardCXX11Attribute(AttrName, ScopeName)) {
-ParsedAttr &Attr = *Attrs.begin();
+ParsedAttr &Attr = Attrs.back();
 // If the attribute is a standard or built-in attribute and we are
 // parsing an argument list, we need to determine whether this attribute
 // was allowed to have an argument list (such as [[deprecated]]), and how
Index: cfe/trunk/lib/Parse/ParseObjc.cpp
===
--- cfe/trunk/lib/Parse/ParseObjc.cpp
+++ cfe/trunk/lib/Parse/ParseObjc.cpp
@@ -384,12 +384,12 @@
 
   if (D.getNumTypeObjects() > 0) {
 // Add the attribute to the declarator chunk nearest the declarator.
-D.getTypeObject(0).getAttrs().addAtStart(
+D.getTypeObject(0).getAttrs().addAtEnd(
 getNullabilityAttr(D.getAttributePool()));
   } else if (!addedToDeclSpec) {
 // Otherwise, just put it on th

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338800: Append new attributes to the end of an 
AttributeList. (authored by Meinersbur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48100?vs=158639&id=158880#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/ParsedAttr.h
  lib/AST/ItaniumMangle.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseObjc.cpp
  lib/Sema/SemaAttr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll-and-jam.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/nullability.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaObjC/nullability.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -721,10 +721,8 @@
   if (FD->hasAttr()) {
 FunctionTypeDepthState Saved = FunctionTypeDepth.push();
 Out << "Ua9enable_ifI";
-// FIXME: specific_attr_iterator iterates in reverse order. Fix that and use
-// it here.
-for (AttrVec::const_reverse_iterator I = FD->getAttrs().rbegin(),
- E = FD->getAttrs().rend();
+for (AttrVec::const_iterator I = FD->getAttrs().begin(),
+ E = FD->getAttrs().end();
  I != E; ++I) {
   EnableIfAttr *EIA = dyn_cast(*I);
   if (!EIA)
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -246,16 +246,16 @@
 
   getMutableDeclSpec().getAttributes().clearListOnly();
   for (ParsedAttr *AL : savedAttrs)
-getMutableDeclSpec().getAttributes().addAtStart(AL);
+getMutableDeclSpec().getAttributes().addAtEnd(AL);
 }
   };
 } // end anonymous namespace
 
 static void moveAttrFromListToList(ParsedAttr &attr,
ParsedAttributesView &fromList,
ParsedAttributesView &toList) {
   fromList.remove(&attr);
-  toList.addAtStart(&attr);
+  toList.addAtEnd(&attr);
 }
 
 /// The location of a type attribute.
@@ -4128,7 +4128,7 @@
   SourceRange(pointerLoc), nullptr, SourceLocation(), nullptr, 0,
   syntax);
 
-  attrs.addAtStart(nullabilityAttr);
+  attrs.addAtEnd(nullabilityAttr);
 
   if (inferNullabilityCS) {
 state.getDeclarator().getMutableDeclSpec().getObjCQualifiers()
@@ -5093,7 +5093,7 @@
   &S.Context.Idents.get("objc_ownership"), SourceLocation(),
   /*scope*/ nullptr, SourceLocation(),
   /*args*/ &Args, 1, ParsedAttr::AS_GNU);
-  chunk.getAttrs().addAtStart(attr);
+  chunk.getAttrs().addAtEnd(attr);
   // TODO: mark whether we did this inference?
 }
 
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -6215,24 +6215,6 @@
   return nullptr;
 }
 
-// specific_attr_iterator iterates over enable_if attributes in reverse, and
-// enable_if is order-sensitive. As a result, we need to reverse things
-// sometimes. Size of 4 elements is arbitrary.
-static SmallVector
-getOrderedEnableIfAttrs(const FunctionDecl *Function) {
-  SmallVector Result;
-  if (!Function->hasAttrs())
-return Result;
-
-  const auto &FuncAttrs = Function->getAttrs();
-  for (Attr *Attr : FuncAttrs)
-if (auto *EnableIf = dyn_cast(Attr))
-  Result.push_back(EnableIf);
-
-  std::reverse(Result.begin(), Result.end());
-  return Result;
-}
-
 static bool
 convertArgsForAvailabilityChecks(Sema &S, FunctionDecl *Function, Expr *ThisArg,
  ArrayRef Args, Sema::SFINAETrap &Trap,
@@ -6306,9 +6288,8 @@
 
 EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef Args,
   bool MissingImplicitThis) {
-  SmallVector EnableIfAttrs =
-  getOrderedEnableIfAttrs(Function);
-  if (EnableIfAttrs.empty())
+  auto EnableIfAttrs = Function->specific_attrs();
+  if (EnableIfAttrs.begin() == EnableIfAttrs.end())
 return nullptr;
 
   SFINAETrap Trap(*this);
@@ -6318,7 +6299,7 @@
   if (!convertArgsFor

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, aaron.ballman, nicholas, dlj, echristo, 
erichkeane.

Currently, attributes from previous declarations ('inherited attributes') are 
added to the end of a declaration's list of attributes. Before r338800, the 
attribute list was in reverse. r338800 changed the order of non-inherited 
(parsed from the current declaration) attributes, but inherited attributes are 
still appended to the end of the list.

This patch appends inherited attributes after other inherited attributes, but 
before any non-inherited attribute. This is to make the order of attributes in 
the AST correspond to the order in the source code.


Repository:
  rC Clang

https://reviews.llvm.org/D50214

Files:
  include/clang/AST/DeclBase.h
  lib/AST/DeclBase.cpp
  test/Misc/ast-dump-attr.cpp
  test/Sema/attr-availability-ios.c
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability-watchos.c

Index: test/Sema/attr-availability-watchos.c
===
--- test/Sema/attr-availability-watchos.c
+++ test/Sema/attr-availability-watchos.c
@@ -32,8 +32,8 @@
 void f5_attr_reversed_watchos(int) __attribute__((availability(ios, deprecated=3.0))) __attribute__((availability(watchos,introduced=2.0)));
 void f5b_watchos(int) __attribute__((availability(watchos,introduced=2.0))) __attribute__((availability(watchos,deprecated=3.0))); // expected-note {{'f5b_watchos' has been explicitly marked deprecated here}}
 void f5c_watchos(int) __attribute__((availability(ios,introduced=2.0))) __attribute__((availability(ios,deprecated=3.0))); // expected-note {{'f5c_watchos' has been explicitly marked deprecated here}}
-void f6_watchos(int) __attribute__((availability(watchos,deprecated=3.0)));
-void f6_watchos(int) __attribute__((availability(watchOS,introduced=2.0))); // expected-note {{'f6_watchos' has been explicitly marked deprecated here}}
+void f6_watchos(int) __attribute__((availability(watchos,deprecated=3.0))); // expected-note {{'f6_watchos' has been explicitly marked deprecated here}}
+void f6_watchos(int) __attribute__((availability(watchOS,introduced=2.0)));
 
 void test_watchos() {
   f0_watchos(0); // expected-warning{{'f0_watchos' is deprecated: first deprecated in watchOS 2.1}}
Index: test/Sema/attr-availability-tvos.c
===
--- test/Sema/attr-availability-tvos.c
+++ test/Sema/attr-availability-tvos.c
@@ -7,8 +7,8 @@
 void f4(int) __attribute__((availability(macosx,introduced=10.1,deprecated=10.3,obsoleted=10.5), availability(tvos,introduced=2.0,deprecated=2.1,obsoleted=3.0))); // expected-note{{explicitly marked unavailable}}
 
 void f5(int) __attribute__((availability(tvos,introduced=2.0))) __attribute__((availability(tvos,deprecated=3.0))); // expected-note {{'f5' has been explicitly marked deprecated here}}
-void f6(int) __attribute__((availability(tvos,deprecated=3.0)));
-void f6(int) __attribute__((availability(tvos,introduced=2.0))); // expected-note {{'f6' has been explicitly marked deprecated here}}
+void f6(int) __attribute__((availability(tvos,deprecated=3.0))); // expected-note {{'f6' has been explicitly marked deprecated here}}
+void f6(int) __attribute__((availability(tvos,introduced=2.0)));
 
 void test() {
   f0(0); // expected-warning{{'f0' is deprecated: first deprecated in tvOS 2.1}}
@@ -41,8 +41,8 @@
 void f5_attr_reversed_tvos(int) __attribute__((availability(ios, deprecated=3.0))) __attribute__((availability(tvos,introduced=2.0)));
 void f5b_tvos(int) __attribute__((availability(tvos,introduced=2.0))) __attribute__((availability(tvos,deprecated=3.0))); // expected-note {{'f5b_tvos' has been explicitly marked deprecated here}}
 void f5c_tvos(int) __attribute__((availability(ios,introduced=2.0))) __attribute__((availability(ios,deprecated=3.0))); // expected-note {{'f5c_tvos' has been explicitly marked deprecated here}}
-void f6_tvos(int) __attribute__((availability(tvos,deprecated=3.0)));
-void f6_tvos(int) __attribute__((availability(tvOS,introduced=2.0))); // expected-note {{'f6_tvos' has been explicitly marked deprecated here}}
+void f6_tvos(int) __attribute__((availability(tvos,deprecated=3.0))); // expected-note {{'f6_tvos' has been explicitly marked deprecated here}}
+void f6_tvos(int) __attribute__((availability(tvOS,introduced=2.0)));
 
 void test_tvos() {
   f0_tvos(0); // expected-warning{{'f0_tvos' is deprecated: first deprecated in tvOS 2.1}}
Index: test/Sema/attr-availability-ios.c
===
--- test/Sema/attr-availability-ios.c
+++ test/Sema/attr-availability-ios.c
@@ -7,8 +7,8 @@
 void f4(int) __attribute__((availability(macosx,introduced=10.1,deprecated=10.3,obsoleted=10.5), availability(ios,introduced=2.0,deprecated=2.1,obsoleted=3.0))); // expected-note{{explicitly marked unavailable}}
 
 void f5(int) __attribute__((availability(ios,introduced=2.0))) __attribut

[PATCH] D50216: Pass IsInherited when merging attribute lists [RFC]

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Herald added subscribers: cfe-commits, eraman.

Attempt to solve the the diagnostic marker order ('note: previous declaration 
here' pointing to a source location before the main error marker) from 
https://reviews.llvm.org/D48100.

The approach is to pass a `IsInherited` parameters. If false, the order if the 
diagnostic marker is reversed.

The actual reversal is only implemented for two diagnoses: 
`diag::err_mismatched_visibility` and 
`diag::err_ownership_returns_index_mismatch`. Test cases not adapted.


Repository:
  rC Clang

https://reviews.llvm.org/D50216

Files:
  include/clang/Sema/Sema.h
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaAttr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaObjCProperty.cpp
  lib/Sema/SemaTemplate.cpp

Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -1609,7 +1609,7 @@
   if (TUK == TUK_Definition)
 NewClass->startDefinition();
 
-  ProcessDeclAttributeList(S, NewClass, Attr);
+  ProcessDeclAttributeList(S, NewClass, Attr, true, false);
 
   if (PrevClassTemplate)
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
@@ -7711,7 +7711,7 @@
 }
   }
 
-  ProcessDeclAttributeList(S, Specialization, Attr);
+  ProcessDeclAttributeList(S, Specialization, Attr, true, false);
 
   // Add alignment attributes if necessary; these attributes are checked when
   // the ASTContext lays out the structure.
@@ -8763,7 +8763,7 @@
   Specialization->setBraceRange(SourceRange());
 
   bool PreviouslyDLLExported = Specialization->hasAttr();
-  ProcessDeclAttributeList(S, Specialization, Attr);
+  ProcessDeclAttributeList(S, Specialization, Attr, true, false);
 
   // Add the explicit instantiation into its lexical context. However,
   // since explicit instantiations are never found by name lookup, we
@@ -9167,7 +9167,8 @@
   Prev->setTemplateSpecializationKind(TSK, D.getIdentifierLoc());
   if (PrevTemplate) {
 // Merge attributes.
-ProcessDeclAttributeList(S, Prev, D.getDeclSpec().getAttributes());
+ProcessDeclAttributeList(S, Prev, D.getDeclSpec().getAttributes(), true,
+ false);
   }
   if (TSK == TSK_ExplicitInstantiationDefinition)
 InstantiateVariableDefinition(D.getIdentifierLoc(), Prev);
@@ -9329,7 +9330,8 @@
   return (Decl*) nullptr;
   }
 
-  ProcessDeclAttributeList(S, Specialization, D.getDeclSpec().getAttributes());
+  ProcessDeclAttributeList(S, Specialization, D.getDeclSpec().getAttributes(),
+   true, false);
 
   // In MSVC mode, dllimported explicit instantiation definitions are treated as
   // instantiation declarations.
Index: lib/Sema/SemaObjCProperty.cpp
===
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -644,7 +644,7 @@
 PDecl->setInvalidDecl();
   }
 
-  ProcessDeclAttributes(S, PDecl, FD.D);
+  ProcessDeclAttributes(S, PDecl, FD.D, false);
 
   // Regardless of setter/getter attribute, we save the default getter/setter
   // selector names in anticipation of declaration of setter/getter methods.
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -919,7 +919,7 @@
 Method->addAttr(A);
 
   // Attributes on the lambda apply to the method.
-  ProcessDeclAttributes(CurScope, Method, ParamInfo);
+  ProcessDeclAttributes(CurScope, Method, ParamInfo, false);
 
   // CUDA lambdas get implicit attributes based on the scope in which they're
   // declared.
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -13381,7 +13381,7 @@
   }
 
   // Finally we can process decl attributes.
-  ProcessDeclAttributes(CurScope, CurBlock->TheDecl, ParamInfo);
+  ProcessDeclAttributes(CurScope, CurBlock->TheDecl, ParamInfo, false);
 
   // Put the parameter variables in scope.
   for (auto AI : CurBlock->TheDecl->parameters()) {
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -1059,7 +1059,7 @@
 }
   }
 
-  ProcessDeclAttributeList(TUScope, IDecl, AttrList);
+  ProcessDeclAttributeList(TUScope, IDecl, AttrList, true, false);
   AddPragmaAttributes(TUScope, IDecl);
   PushOnScopeChains(IDecl, TUScope);
 
@@ -1246,7 +1246,7 @@
 PDecl->startDefinition();
   }
 
-  ProcessDeclAttributeList(TUScope, PDecl, AttrList);
+  ProcessDeclAttributeList(TUScope, PDecl, AttrList, true, false);
   AddPragmaAttribute

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I have two approaches to tackle the wrong marker order: 
https://reviews.llvm.org/D50215 and https://reviews.llvm.org/D50216. IMHO both 
are too invasive to be justified for the small issue.


Repository:
  rL LLVM

https://reviews.llvm.org/D48100



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

[PATCH] D48808: [CodeGen] Emit parallel_loop_access for each loop in the loop stack.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338810: [CodeGen] Emit parallel_loop_access for each loop in 
the loop stack. (authored by Meinersbur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48808?vs=155288&id=158912#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48808

Files:
  lib/CodeGen/CGLoopInfo.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp


Index: lib/CodeGen/CGLoopInfo.cpp
===
--- lib/CodeGen/CGLoopInfo.cpp
+++ lib/CodeGen/CGLoopInfo.cpp
@@ -344,6 +344,17 @@
 return;
   }
 
-  if (L.getAttributes().IsParallel && I->mayReadOrWriteMemory())
-I->setMetadata("llvm.mem.parallel_loop_access", L.getLoopID());
+  if (I->mayReadOrWriteMemory()) {
+SmallVector ParallelLoopIDs;
+for (const LoopInfo &AL : Active)
+  if (AL.getAttributes().IsParallel)
+ParallelLoopIDs.push_back(AL.getLoopID());
+
+MDNode *ParallelMD = nullptr;
+if (ParallelLoopIDs.size() == 1)
+  ParallelMD = cast(ParallelLoopIDs[0]);
+else if (ParallelLoopIDs.size() >= 2)
+  ParallelMD = MDNode::get(I->getContext(), ParallelLoopIDs);
+I->setMetadata("llvm.mem.parallel_loop_access", ParallelMD);
+  }
 }
Index: test/CodeGenCXX/pragma-loop-safety-nested.cpp
===
--- test/CodeGenCXX/pragma-loop-safety-nested.cpp
+++ test/CodeGenCXX/pragma-loop-safety-nested.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | 
FileCheck %s
+
+// Verify that the inner access is tagged with a parallel_loop_access
+// for the inner and outer loop using a list.
+void vectorize_nested_test(int *List, int Length) {
+#pragma clang loop vectorize(assume_safety) interleave(disable) unroll(disable)
+  for (int i = 0; i < Length; ++i) {
+#pragma clang loop vectorize(assume_safety) interleave(disable) unroll(disable)
+for (int j = 0; j < Length; ++j)
+  List[i * Length + j] = (i + j) * 2;
+  }
+}
+
+// CHECK: %[[MUL:.+]] = mul
+// CHECK: store i32 %[[MUL]], i32* %{{.+}}, !llvm.mem.parallel_loop_access 
![[PARALLEL_LIST:[0-9]+]]
+// CHECK: br label %{{.+}}, !llvm.loop ![[INNER_LOOPID:[0-9]+]]
+// CHECK: br label %{{.+}}, !llvm.loop ![[OUTER_LOOPID:[0-9]+]]
+
+// CHECK: ![[OUTER_LOOPID]] = distinct !{![[OUTER_LOOPID]],
+// CHECK: ![[PARALLEL_LIST]] = !{![[OUTER_LOOPID]], ![[INNER_LOOPID]]}
+// CHECK: ![[INNER_LOOPID]] = distinct !{![[INNER_LOOPID]],
Index: test/CodeGenCXX/pragma-loop-safety-outer.cpp
===
--- test/CodeGenCXX/pragma-loop-safety-outer.cpp
+++ test/CodeGenCXX/pragma-loop-safety-outer.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | 
FileCheck %s
+
+// Verify that the inner access is tagged with a parallel_loop_access
+// for the outer loop.
+void vectorize_outer_test(int *List, int Length) {
+#pragma clang loop vectorize(assume_safety) interleave(disable) unroll(disable)
+  for (int i = 0; i < Length; i += 2) {
+#pragma clang loop unroll(full)
+for (int j = 0; j < 2; j += 1)
+  List[i + j] = (i + j) * 2;
+  }
+}
+
+// CHECK: %[[MUL:.+]] = mul
+// CHECK: store i32 %[[MUL]], i32* %{{.+}}, !llvm.mem.parallel_loop_access 
![[OUTER_LOOPID:[0-9]+]]
+// CHECK: br label %{{.+}}, !llvm.loop ![[INNER_LOOPID:[0-9]+]]
+// CHECK: br label %{{.+}}, !llvm.loop ![[OUTER_LOOPID]]
+
+// CHECK: ![[OUTER_LOOPID]] = distinct !{![[OUTER_LOOPID]],
+// CHECK: ![[INNER_LOOPID]] = distinct !{![[INNER_LOOPID]],


Index: lib/CodeGen/CGLoopInfo.cpp
===
--- lib/CodeGen/CGLoopInfo.cpp
+++ lib/CodeGen/CGLoopInfo.cpp
@@ -344,6 +344,17 @@
 return;
   }
 
-  if (L.getAttributes().IsParallel && I->mayReadOrWriteMemory())
-I->setMetadata("llvm.mem.parallel_loop_access", L.getLoopID());
+  if (I->mayReadOrWriteMemory()) {
+SmallVector ParallelLoopIDs;
+for (const LoopInfo &AL : Active)
+  if (AL.getAttributes().IsParallel)
+ParallelLoopIDs.push_back(AL.getLoopID());
+
+MDNode *ParallelMD = nullptr;
+if (ParallelLoopIDs.size() == 1)
+  ParallelMD = cast(ParallelLoopIDs[0]);
+else if (ParallelLoopIDs.size() >= 2)
+  ParallelMD = MDNode::get(I->getContext(), ParallelLoopIDs);
+I->setMetadata("llvm.mem.parallel_loop_access", ParallelMD);
+  }
 }
Index: test/CodeGenCXX/pragma-loop-safety-nested.cpp
===
--- test/CodeGenCXX/pragma-loop-safety-nested.cpp
+++ test/CodeGenCXX/pragma-loop-safety-nested.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// Verify that the inner access is tagged with a parallel_loop_access
+// for the inner and outer loop using a list.
+void vectorize_ne

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

For this patch the goal is to have the attributes in the AST in an order that 
is less surprising to consumers (including out-of-tree). If we change it now, 
new/revised code/diagnostics will be written to match this order.

I agree that ideally, most attributes (exceptions are e.g. `enable_if`) should 
be more like a set than a list, but I don't think we can get rid of the 
'ordered-ness' in general.

I though about ordering by SourceLocation as well and could be implemented like 
this:
Add all SLoc's sequences for two SourceLocation to lists. Start comparing from 
the bottom (=> the translation units) until there is a difference.

However, I dislike the idea that the result could potentially depend on 
SourceLocation that previously was only used when emitting diagnostics.




Comment at: test/Misc/ast-dump-attr.cpp:228-230
+// CHECK-NEXT: DeprecatedAttr{{.*}} Inherited
+// CHECK-NEXT: WarnUnusedResultAttr{{.*}} Inherited
+// CHECK-NEXT: AnnotateAttr{{.*}} Inherited

Note that before rC338800, these were mixed-up (not even the reverse order)



Comment at: test/Sema/attr-availability-ios.c:10
 void f5(int) __attribute__((availability(ios,introduced=2.0))) 
__attribute__((availability(ios,deprecated=3.0))); // expected-note {{'f5' has 
been explicitly marked deprecated here}}
-void f6(int) __attribute__((availability(ios,deprecated=3.0)));
-void f6(int) __attribute__((availability(iOS,introduced=2.0))); // 
expected-note {{'f6' has been explicitly marked deprecated here}}
+void f6(int) __attribute__((availability(ios,deprecated=3.0))); // 
expected-note {{'f6' has been explicitly marked deprecated here}}
+void f6(int) __attribute__((availability(iOS,introduced=2.0)));

Note that the deprecated marker moved to the line that actually declares the 
function as deprecated.


Repository:
  rC Clang

https://reviews.llvm.org/D50214



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

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: docs/CodingStandards.rst:514-516
+line of code. Some common editors will automatically remove trailing whitespace
+when saving a file which causes unrelated changes to appear in diffs and
+commits.

Just to note that this policy will prohibit the use of remove 
trailing-whitespace feature in editors since it makes it impossible to edit the 
file without also 'editing' any unrelated whitespace. At the same time, since 
not being able to use the feature, I risk committing trailing whitespace myself 
(that is, some additional steps are necessary: I use 'git clang-format' which 
should remove traling whitespace only on edited lines and 'git diff' shows 
trailing whitespace in red; these are still additional steps that are easy to 
miss)


https://reviews.llvm.org/D50055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://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 added a comment.

Definition of `__attibute__((const))` from 
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

> Many functions do not examine any values except their arguments, and have no 
> effects except the return value. Basically this is just slightly more strict 
> class than the pure attribute below, since function is not allowed to read 
> global memory.
>  Note that a function that has pointer arguments and examines the data 
> pointed to must not be declared const. Likewise, a function that calls a 
> non-const function usually must not be const. It does not make sense for a 
> const function to return void.

Definition of `speculatable` from 
http://llvm.org/docs/LangRef.html#function-attributes

> This function attribute indicates that the function does not have any effects 
> besides calculating its result and does not have undefined behavior.


https://reviews.llvm.org/D33774



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

The RFC: https://lists.llvm.org/pipermail/cfe-dev/2018-May/058141.html


https://reviews.llvm.org/D47267



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D47267#1122425, @dmgreen wrote:

> I noticed in the paper that you used the name "unrollandjam", minus 
> underscores. Should I change this use that spelling here? I have no strong 
> opinion of one over the other (was just using what I had found from the Intel 
> docs).


IMHO you can keep `unroll_and_jam` (which is already supported by Intel 
syntax). When I imagined the name, I had xlc's `unrollandfuse` in mind, but 
found that "and jam" is better known than "and fuse".

We can have a discussion about how to name them in general. `nounroll_and_jam` 
seems a strange mix of write words together and separate words by underscores.


https://reviews.llvm.org/D47267



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

[PATCH] D47267: [UnrollAndJam] Add unroll_and_jam pragma handling

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D47267#1123013, @dmgreen wrote:

> I see your point about the mix of underscores. "nounroll_and_jam" also comes 
> from the Intel docs, and theres already "nounroll" vs "unroll". The "no" 
> becomes a qualifier on "unroll_and_jam". "no_unroll_and_jam" feels less 
> consistent to me.


`nounroll_and_jam` looks like it should be parsed as "(no unroll) and jam" (do 
not unroll, but fuse) instead of "no (unroll-and-jam)" because `nounroll` is 
one word and as you mentioned, already used as a keyword somewhere else. Other 
variants use the underscore to append an option, e.g. `vectorize_width`.

> But if you have a strong opinion, I'm happy enough to change it.

I don't. Feel free to chose the name you think fits best. We might support 
multiple spellings if necessary.

If we want to add more transformations, it would be nice to have an explicit 
naming scheme. E.g. for "register tiling", "stream_unroll" (supported by xlc), 
"index set splitting", "statement reordering", "strip mine", "overlap tiling", 
"diamond tiling", "thread-parallelization", "task-parallelization", "loop 
unswitching", etc.


https://reviews.llvm.org/D47267



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, TylerNowicki, ABataev, thakis, rjmccall, 
george.burgess.iv, nicholas, nlewycky.
Herald added subscribers: atanasyan, zzheng.

... instead of prepending it at the beginning (the original behavior since 
implemented in r122535 2010-12-23). This builds up an AttributeList in the the 
order in which the attributes appear in the source.

The reverse order caused nodes for attributes in the AST (e.g. `LoopHint`) to 
be in the reverse order, and therefore printed in the wrong order in 
`-ast-dump`. Some TODO comments mention this. The order was explicitly reversed 
for `enable_if` attribute overload resolution and name mangling, which is not 
necessary anymore with this patch.

The change unfortunately has some secondary effect, especially on diagnostic 
output. In the simplest cases, the CHECK lines or expected diagnostic were 
changed to the the new output. If the kind of error/warning changed, the 
attribute' order was changed instead.

This unfortunately causes some 'previous occurrence here' hints to be textually 
after the main marker. This typically happens when attributes are merged, but 
are incompatible to each other. Interchanging the role of the the main and note 
SourceLocation will also cause the case where two different declaration's 
attributes (in contrast to multiple attributes of the same declaration) are 
merged to be reverse. There is no easy fix because sometimes previous 
attributes are merged into a new declaration's attribute list, sometimes new 
attributes are added to a previous declaration's attribute list. Since 
'previous occurrence here' pointing to locations after the main marker is not 
rare, I left the markers as-is; it is only relevant when the attributes are 
declared in the same declaration anyway.


Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/AttributeList.h
  lib/AST/ItaniumMangle.cpp
  lib/Sema/SemaOverload.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-swiftcall.c
  test/Sema/attr-target-mv.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/ms_abi-sysv_abi.c
  test/Sema/nullability.c
  test/Sema/stdcall-fastcall.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/attr-swiftcall.cpp
  test/SemaCXX/decl-microsoft-call-conv.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: test/SemaTemplate/attributes.cpp
===
--- test/SemaTemplate/attributes.cpp
+++ test/SemaTemplate/attributes.cpp
@@ -55,11 +55,11 @@
 }
 
 // CHECK: FunctionTemplateDecl {{.*}} HasAnnotations
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK: FunctionDecl {{.*}} HasAnnotations
 // CHECK:   TemplateArgument type 'int'
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -58,8 +58,8 @@
 
 void func_multiple_addr(void) {
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
 }
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -62,14 +62,14 @@
 [uuid("22A0---C000-0049")] class C4 {};
 
 // Both cl and clang-cl error out on this:
-// expected-note@+1 {{previous uuid specified here}}
-class __dec

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:6194
 static SmallVector
 getOrderedEnableIfAttrs(const FunctionDecl *Function) {
   SmallVector Result;

nicholas wrote:
> This function shouldn't be necessary any more. All it's doing now is making 
> an unnecessary copy.
I tried to keep the diff small and obvious. I'll refactor it in the next diff 
update.



Comment at: lib/Sema/SemaOverload.cpp:8943
 
-  // FIXME: The next several lines are just
-  // specific_attr_iterator but going in declaration order,
-  // instead of reverse order which is how they're stored in the AST.
   auto Cand1Attrs = getOrderedEnableIfAttrs(Cand1);
   auto Cand2Attrs = getOrderedEnableIfAttrs(Cand2);

nicholas wrote:
> This would become "auto Cand1Attrs = Cand1->specific_attrs();" 
> but I think you can simplify that even further if you want. To do that you'd 
> need to sink the "return Comparison::Worse;" inside the loop that follows it. 
> If you don't do that you'll have to switch the calls to Cand1Attrs.size() and 
> Cand2Attrs.size() into calls to std::distance, since llvm::iterator_range 
> doesn't have a size() method.
Thanks for the hint.


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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

- Remove obsolete comment
- Refactor getOrderedEnableIfAttrs


Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/AttributeList.h
  lib/AST/ItaniumMangle.cpp
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-swiftcall.c
  test/Sema/attr-target-mv.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/ms_abi-sysv_abi.c
  test/Sema/nullability.c
  test/Sema/stdcall-fastcall.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/attr-swiftcall.cpp
  test/SemaCXX/decl-microsoft-call-conv.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: test/SemaTemplate/attributes.cpp
===
--- test/SemaTemplate/attributes.cpp
+++ test/SemaTemplate/attributes.cpp
@@ -55,11 +55,11 @@
 }
 
 // CHECK: FunctionTemplateDecl {{.*}} HasAnnotations
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK: FunctionDecl {{.*}} HasAnnotations
 // CHECK:   TemplateArgument type 'int'
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -58,8 +58,8 @@
 
 void func_multiple_addr(void) {
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
 }
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -62,14 +62,14 @@
 [uuid("22A0---C000-0049")] class C4 {};
 
 // Both cl and clang-cl error out on this:
-// expected-note@+1 {{previous uuid specified here}}
-class __declspec(uuid("00A0---C000-0049"))
 // expected-error@+1 {{uuid does not match previous declaration}}
+class __declspec(uuid("00A0---C000-0049"))
+// expected-note@+1 {{previous uuid specified here}}
   __declspec(uuid("11A0---C000-0049")) C5;
 
-// expected-note@+1 {{previous uuid specified here}}
-[uuid("00A0---C000-0049"),
 // expected-error@+1 {{uuid does not match previous declaration}}
+[uuid("00A0---C000-0049"),
+// expected-note@+1 {{previous uuid specified here}}
  uuid("11A0---C000-0049")] class C6;
 
 // cl doesn't diagnose having one uuid each as []-style attributes and as
Index: test/SemaCXX/decl-microsoft-call-conv.cpp
===
--- test/SemaCXX/decl-microsoft-call-conv.cpp
+++ test/SemaCXX/decl-microsoft-call-conv.cpp
@@ -161,7 +161,7 @@
 // expected-error@+3 {{vectorcall and cdecl attributes are not compatible}}
 // expected-error@+2 {{stdcall and cdecl attributes are not compatible}}
 // expected-error@+1 {{fastcall and cdecl attributes are not compatible}}
-void __cdecl __cdecl __stdcall __cdecl __fastcall __vectorcall multi_cc(int x);
+void __vectorcall __fastcall __cdecl __stdcall __cdecl __cdecl multi_cc(int x);
 
 template  void __stdcall StdcallTemplate(T) {}
 template <> void StdcallTemplate(int) {}
Index: test/SemaCXX/attr-swiftcall.cpp
===
--- test/SemaCXX/attr-swiftcall.cpp
+++ test/SemaCXX/attr-swiftcall.cpp
@@ -7,7 +7,7 @@
 
 int notAFunction SWIFTCALL; // expected-warni

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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



Comment at: lib/AST/ItaniumMangle.cpp:710-711
 Out << "Ua9enable_ifI";
 // FIXME: specific_attr_iterator iterates in reverse order. Fix that and 
use
 // it here.
+for (AttrVec::const_iterator I = FD->getAttrs().begin(),

Will remove this FIXME in the next update.


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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

- Remove FIXME
- Add comment about O(n^2) execution time when adding attributes
- Do not store enable_if attributes in temporary list


Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/AttributeList.h
  lib/AST/ItaniumMangle.cpp
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-swiftcall.c
  test/Sema/attr-target-mv.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/ms_abi-sysv_abi.c
  test/Sema/nullability.c
  test/Sema/stdcall-fastcall.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/attr-swiftcall.cpp
  test/SemaCXX/decl-microsoft-call-conv.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: test/SemaTemplate/attributes.cpp
===
--- test/SemaTemplate/attributes.cpp
+++ test/SemaTemplate/attributes.cpp
@@ -55,11 +55,11 @@
 }
 
 // CHECK: FunctionTemplateDecl {{.*}} HasAnnotations
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK: FunctionDecl {{.*}} HasAnnotations
 // CHECK:   TemplateArgument type 'int'
-// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
+// CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 template [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations(); }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -58,8 +58,8 @@
 
 void func_multiple_addr(void) {
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
 }
Index: test/SemaCXX/ms-uuid.cpp
===
--- test/SemaCXX/ms-uuid.cpp
+++ test/SemaCXX/ms-uuid.cpp
@@ -62,14 +62,14 @@
 [uuid("22A0---C000-0049")] class C4 {};
 
 // Both cl and clang-cl error out on this:
-// expected-note@+1 {{previous uuid specified here}}
-class __declspec(uuid("00A0---C000-0049"))
 // expected-error@+1 {{uuid does not match previous declaration}}
+class __declspec(uuid("00A0---C000-0049"))
+// expected-note@+1 {{previous uuid specified here}}
   __declspec(uuid("11A0---C000-0049")) C5;
 
-// expected-note@+1 {{previous uuid specified here}}
-[uuid("00A0---C000-0049"),
 // expected-error@+1 {{uuid does not match previous declaration}}
+[uuid("00A0---C000-0049"),
+// expected-note@+1 {{previous uuid specified here}}
  uuid("11A0---C000-0049")] class C6;
 
 // cl doesn't diagnose having one uuid each as []-style attributes and as
Index: test/SemaCXX/decl-microsoft-call-conv.cpp
===
--- test/SemaCXX/decl-microsoft-call-conv.cpp
+++ test/SemaCXX/decl-microsoft-call-conv.cpp
@@ -161,7 +161,7 @@
 // expected-error@+3 {{vectorcall and cdecl attributes are not compatible}}
 // expected-error@+2 {{stdcall and cdecl attributes are not compatible}}
 // expected-error@+1 {{fastcall and cdecl attributes are not compatible}}
-void __cdecl __cdecl __stdcall __cdecl __fastcall __vectorcall multi_cc(int x);
+void __vectorcall __fastcall __cdecl __stdcall __cdecl __cdecl multi_cc(int x);
 
 template  void __stdcall StdcallTemplate(T) {}
 template <> void StdcallTemplate(int) {}
Index: test/SemaCXX/attr-swiftcall.cpp

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D48100#1132208, @aaron.ballman wrote:

> Can you also simplify `hasSameOverloadableAttrs()` in ASTReaderDecl.cpp 
> similar to what you did in SemaOverload.cpp (the copy seems spurious)?


Changed `hasSameOverloadableAttrs`. Apart to the comment about that the 
reversal of attributes being unimportant, it does not seem related to the new 
attribute order, though.
I made the iteration over `AEnableIfAttrs` and `BEnableIfAttrs` symmetric (and 
IMHO easier to understand), which unfortunately is not possible with a foreach 
(there are also a zip-iterators, but I am not sure how to check for different 
lengths without `std::distance` iterating over it separately).
I can change `compareEnableIfAttrs` as well on request.


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC335084: Append new attributes to the end of an 
AttributeList. (authored by Meinersbur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48100?vs=151378&id=151998#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48100

Files:
  include/clang/Sema/AttributeList.h
  lib/AST/ItaniumMangle.cpp
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Index/annotate-comments-availability-attrs.cpp
  test/Index/complete-with-annotations.cpp
  test/Misc/ast-print-pragmas.cpp
  test/PCH/pragma-loop.cpp
  test/Parser/pragma-loop-safety.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-unroll.cpp
  test/Sema/attr-availability-tvos.c
  test/Sema/attr-availability.c
  test/Sema/attr-coldhot.c
  test/Sema/attr-disable-tail-calls.c
  test/Sema/attr-long-call.c
  test/Sema/attr-micromips.c
  test/Sema/attr-notail.c
  test/Sema/attr-ownership.c
  test/Sema/attr-ownership.cpp
  test/Sema/attr-print.c
  test/Sema/attr-swiftcall.c
  test/Sema/attr-target-mv.c
  test/Sema/attr-visibility.c
  test/Sema/internal_linkage.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/ms_abi-sysv_abi.c
  test/Sema/nullability.c
  test/Sema/stdcall-fastcall.c
  test/SemaCXX/attr-print.cpp
  test/SemaCXX/attr-swiftcall.cpp
  test/SemaCXX/decl-microsoft-call-conv.cpp
  test/SemaCXX/ms-uuid.cpp
  test/SemaOpenCL/address-spaces.cl
  test/SemaTemplate/attributes.cpp

Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -707,10 +707,8 @@
   if (FD->hasAttr()) {
 FunctionTypeDepthState Saved = FunctionTypeDepth.push();
 Out << "Ua9enable_ifI";
-// FIXME: specific_attr_iterator iterates in reverse order. Fix that and use
-// it here.
-for (AttrVec::const_reverse_iterator I = FD->getAttrs().rbegin(),
- E = FD->getAttrs().rend();
+for (AttrVec::const_iterator I = FD->getAttrs().begin(),
+ E = FD->getAttrs().end();
  I != E; ++I) {
   EnableIfAttr *EIA = dyn_cast(*I);
   if (!EIA)
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -6189,24 +6189,6 @@
   return nullptr;
 }
 
-// specific_attr_iterator iterates over enable_if attributes in reverse, and
-// enable_if is order-sensitive. As a result, we need to reverse things
-// sometimes. Size of 4 elements is arbitrary.
-static SmallVector
-getOrderedEnableIfAttrs(const FunctionDecl *Function) {
-  SmallVector Result;
-  if (!Function->hasAttrs())
-return Result;
-
-  const auto &FuncAttrs = Function->getAttrs();
-  for (Attr *Attr : FuncAttrs)
-if (auto *EnableIf = dyn_cast(Attr))
-  Result.push_back(EnableIf);
-
-  std::reverse(Result.begin(), Result.end());
-  return Result;
-}
-
 static bool
 convertArgsForAvailabilityChecks(Sema &S, FunctionDecl *Function, Expr *ThisArg,
  ArrayRef Args, Sema::SFINAETrap &Trap,
@@ -6280,9 +6262,9 @@
 
 EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef Args,
   bool MissingImplicitThis) {
-  SmallVector EnableIfAttrs =
-  getOrderedEnableIfAttrs(Function);
-  if (EnableIfAttrs.empty())
+  auto EnableIfAttrs = Function->specific_attrs();
+
+  if (EnableIfAttrs.begin() == EnableIfAttrs.end())
 return nullptr;
 
   SFINAETrap Trap(*this);
@@ -6292,7 +6274,7 @@
   if (!convertArgsForAvailabilityChecks(
   *this, Function, /*ThisArg=*/nullptr, Args, Trap,
   /*MissingImplicitThis=*/true, DiscardedThis, ConvertedArgs))
-return EnableIfAttrs[0];
+return *EnableIfAttrs.begin();
 
   for (auto *EIA : EnableIfAttrs) {
 APValue Result;
@@ -8943,24 +8925,21 @@
 return Cand1Attr ? Comparison::Better : Comparison::Worse;
   }
 
-  // FIXME: The next several lines are just
-  // specific_attr_iterator but going in declaration order,
-  // instead of reverse order which is how they're stored in the AST.
-  auto Cand1Attrs = getOrderedEnableIfAttrs(Cand1);
-  auto Cand2Attrs = getOrderedEnableIfAttrs(Cand2);
-
-  // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-  // has fewer enable_if attributes than Cand2.
-  if (Cand1Attrs.size() < Cand2Attrs.size())
-return Comparison::Worse;
+  auto Cand1Attrs = Cand1->specific_attrs();
+  auto Cand2Attrs = Cand2->specific_attrs();
 
   auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (auto &Cand2A : Cand2Attrs) {
+  for (auto Cand2A : Cand2Attrs) {
 Cand1ID.clear();
 Cand2ID.clear();
 
-auto &Cand1A = *Cand1I++;
+// It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
+// has fewer enable_if attributes than Cand2.
+

[PATCH] D56928: Support attribute used in member funcs of class templates

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Thanks for looking into my almost 6 year old bug!




Comment at: 
test/CodeGenCXX/attr-used-member-function-implicit-instantiation.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -O0 -o - %s \
+// RUN:  | FileCheck %s

Could you mention PR17480 in this test file as well?



Comment at: 
test/CodeGenCXX/attr-used-member-function-implicit-instantiation.cpp:8
+  template  struct S {
+int USED f() {
+  return 0;

Any reason to use the preprocessor here instead of  `__attribute__` directly?



Comment at: 
test/CodeGenCXX/attr-used-member-function-implicit-instantiation.cpp:16
+// as a result of the S class template implicit instantiation
+// CHECK-DAG: define linkonce_odr i32 
@_ZN31InstantiateUsedMemberDefinition1SIiE1fEv
+S inst;

`CHECK:` instead of `CHECK-DAG` should be sufficient. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D56928



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

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

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

- Allow multiple access groups per instructions, i.e. an instruction can be in 
multiple access groups. This allows a simple 'union' operation that occurs when 
inlining into another function. A memory access is considered parallel when at 
least one access group is listed in llvm.loop.parallel_accesses. This is 
prioritized over the 'intersect' case for combining instructions which would be 
dual. We only do best-effort here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52117

Files:
  docs/LangRef.rst
  include/llvm/Analysis/LoopInfo.h
  include/llvm/Analysis/LoopInfoImpl.h
  include/llvm/Analysis/VectorUtils.h
  include/llvm/IR/LLVMContext.h
  include/llvm/Transforms/Utils/LoopUtils.h
  lib/Analysis/LoopInfo.cpp
  lib/Analysis/VectorUtils.cpp
  lib/IR/LLVMContext.cpp
  lib/Transforms/InstCombine/InstCombineCalls.cpp
  lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
  lib/Transforms/InstCombine/InstCombinePHI.cpp
  lib/Transforms/Scalar/GVNHoist.cpp
  lib/Transforms/Scalar/LoopVersioningLICM.cpp
  lib/Transforms/Scalar/MemCpyOptimizer.cpp
  lib/Transforms/Scalar/SROA.cpp
  lib/Transforms/Scalar/Scalarizer.cpp
  lib/Transforms/Utils/InlineFunction.cpp
  lib/Transforms/Utils/Local.cpp
  lib/Transforms/Utils/LoopUtils.cpp
  lib/Transforms/Utils/SimplifyCFG.cpp
  test/Analysis/LoopInfo/annotated-parallel-complex.ll
  test/Analysis/LoopInfo/annotated-parallel-simple.ll
  test/ThinLTO/X86/lazyload_metadata.ll
  test/Transforms/Inline/parallel-loop-md-callee.ll
  test/Transforms/Inline/parallel-loop-md-merge.ll
  test/Transforms/Inline/parallel-loop-md.ll
  test/Transforms/InstCombine/intersect-accessgroup.ll
  test/Transforms/InstCombine/loadstore-metadata.ll
  test/Transforms/InstCombine/mem-par-metadata-memcpy.ll
  test/Transforms/LoopVectorize/X86/force-ifcvt.ll
  test/Transforms/LoopVectorize/X86/parallel-loops-after-reg2mem.ll
  test/Transforms/LoopVectorize/X86/parallel-loops.ll
  test/Transforms/LoopVectorize/X86/pr34438.ll
  test/Transforms/LoopVectorize/X86/vect.omp.force.ll
  test/Transforms/LoopVectorize/X86/vect.omp.force.small-tc.ll
  test/Transforms/LoopVectorize/X86/vector_max_bandwidth.ll
  test/Transforms/SROA/mem-par-metadata-sroa.ll
  test/Transforms/Scalarizer/basic.ll
  test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll

Index: test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
===
--- test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
+++ test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
@@ -8,39 +8,39 @@
   br label %for.body
 
 ; CHECK-LABEL: @Test
-; CHECK: load i32, i32* {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
-; CHECK: load i32, i32* {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
-; CHECK: store i32 {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
+; CHECK: load i32, i32* {{.*}}, align 4, !llvm.access.group !0
+; CHECK: load i32, i32* {{.*}}, align 4, !llvm.access.group !0
+; CHECK: store i32 {{.*}}, align 4, !llvm.access.group !0
 ; CHECK-NOT: load
 ; CHECK-NOT: store
 
 for.body: ; preds = %cond.end, %entry
   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %cond.end ]
   %arrayidx = getelementptr inbounds i32, i32* %p, i64 %indvars.iv
-  %0 = load i32, i32* %arrayidx, align 4, !llvm.mem.parallel_loop_access !0
+  %0 = load i32, i32* %arrayidx, align 4, !llvm.access.group !0
   %cmp1 = icmp eq i32 %0, 0
   br i1 %cmp1, label %cond.true, label %cond.false
 
 cond.false:   ; preds = %for.body
   %arrayidx3 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  %v = load i32, i32* %arrayidx3, align 4, !llvm.mem.parallel_loop_access !0
+  %v = load i32, i32* %arrayidx3, align 4, !llvm.access.group !0
   %arrayidx7 = getelementptr inbounds i32, i32* %d, i64 %indvars.iv
-  %1 = load i32, i32* %arrayidx7, align 4, !llvm.mem.parallel_loop_access !0
+  %1 = load i32, i32* %arrayidx7, align 4, !llvm.access.group !0
   %add = add nsw i32 %1, %v
   br label %cond.end
 
 cond.true:   ; preds = %for.body
   %arrayidx4 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  %w = load i32, i32* %arrayidx4, align 4, !llvm.mem.parallel_loop_access !0
+  %w = load i32, i32* %arrayidx4, align 4, !llvm.access.group !0
   %arrayidx8 = getelementptr inbounds i32, i32* %d, i64 %indvars.iv
-  %2 = load i32, i32* %arrayidx8, align 4, !llvm.mem.parallel_loop_access !0
+  %2 = load i32, i32* %arrayidx8, align 4, !llvm.access.group !0
   %add2 = add nsw i32 %2, %w
   br label %cond.end
 
 cond.end: ; preds = %for.body, %cond.false
   %cond = phi i32 [ %add, %cond.false ], [ %add2, %cond.true ]
   %arrayidx9 = getelementptr inbounds i32, i32* %res

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

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

- Fix wrong patch upload
- Simplify access group emission ... .. possible due to the added possibility 
for instructions to belong to multiple access groups in D52116 
. However, the number of access groups is not 
minimized anymore.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52117

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/OpenMP/for_codegen.cpp
  test/OpenMP/for_simd_codegen.cpp
  test/OpenMP/loops_explicit_clauses_codegen.cpp
  test/OpenMP/ordered_codegen.cpp
  test/OpenMP/parallel_for_simd_codegen.cpp
  test/OpenMP/schedule_codegen.cpp
  test/OpenMP/simd_codegen.cpp
  test/OpenMP/simd_metadata.c
  test/OpenMP/target_parallel_for_simd_codegen.cpp
  test/OpenMP/target_simd_codegen.cpp
  test/OpenMP/taskloop_simd_codegen.cpp

Index: test/OpenMP/taskloop_simd_codegen.cpp
===
--- test/OpenMP/taskloop_simd_codegen.cpp
+++ test/OpenMP/taskloop_simd_codegen.cpp
@@ -83,17 +83,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP1]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK2]](
@@ -113,17 +113,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP2]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK3]](
@@ -142,7 +142,7 @@
 // CHECK: [[LB_VAL:%.+]] = load i64, i64* [[LB]],
 // CHECK: store i64 [[LB_VAL]], i64* [[CNT:%.+]],
 // CHECK: br label
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
@@ -192,14 +192,14 @@
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: store i32 %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
 // CHECK: store i32 %{{.+}}, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHE

[PATCH] D55468: Use zip_longest for list comparisons.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: dblaikie, hfinkel.

Use zip_longest in two locations that compare iterator ranges. zip_longest 
allows the iteration using a range-based for-loop and to be symmetric over both 
ranges instead of prioritizing one over the other. In that latter case code 
have to handle the case that the first is longer than the second, the second is 
longer than the first, and both are of the same length, which must partially be 
checked after the loop.

With zip_longest, this becomes an element comparison within the loop like the 
comparison of the elements themselves. The symmetry makes it clearer that 
neither the first and second iterators are handled differently. The iterators 
are not event used directly anymore, just the ranges.


Repository:
  rC Clang

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,27 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, A->getASTContext(), 
true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, B->getASTContext(), 
true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8972,25 +8972,25 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
-
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;
+if (std::get<1>(Pair) == None)
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,27 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (aut

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 177444.
Meinersbur marked 4 inline comments as done.
Meinersbur added a comment.

- Address @dblaikie's review


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,27 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair) || std::get<1>(Pair))
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, A->getASTContext(), 
true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, B->getASTContext(), 
true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8977,25 +8977,25 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
-
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!std::get<0>(Pair))
   return Comparison::Worse;
+if (!std::get<1>(Pair))
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,27 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair) || std::get<1>(Pair))
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressi

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:8979
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;

dblaikie wrote:
> I'd probably write this as "if (!std::get<0>(Pair))" - but I understand that 
> opinions differ on such things easily enough.
Here I tried do be explicit that it's an `llvm::Optional` and not testing for 
null pointers (or whatever the payload of the llvm::Optional is, which might 
itself have an overload bool conversion operator). It seemed worthwhile 
especially because `llvm::Optional` itself does not appear itself around these 
lines.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

dblaikie wrote:
> This might be more legible as "if (std::get<0>(Pair) || std::get<1>(Pair))", 
> I think? (optionally using "hasValue", if preferred - but comparing the 
> hasValues to each other, rather than testing if either is false, seems a bit 
> opaque to me at least)
The idea was 'if the items are unequal, the list is unequal', where when either 
one is undefined, but not the the other, is considered unequal. The test for 
the elements themselves have the same structure (Cand1ID != Cand2ID).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

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

- Fix xor
- Store tuple elements in variables


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8977,25 +8977,28 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
 
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!Cand1A)
   return Comparison::Worse;
+if (!Cand2A)
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*Cand2A)->getCond()->P

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

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



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

dblaikie wrote:
> Meinersbur wrote:
> > dblaikie wrote:
> > > This might be more legible as "if (std::get<0>(Pair) || 
> > > std::get<1>(Pair))", I think? (optionally using "hasValue", if preferred 
> > > - but comparing the hasValues to each other, rather than testing if 
> > > either is false, seems a bit opaque to me at least)
> > The idea was 'if the items are unequal, the list is unequal', where when 
> > either one is undefined, but not the the other, is considered unequal. The 
> > test for the elements themselves have the same structure (Cand1ID != 
> > Cand2ID).
> Sorry, looks like I made a mistake in the suggested change - should be a ! 
> before each of the gets (I wonder if the change as you have it now/as I 
> originally suggested, is causing any test failures - one really hopes it 
> does... because as written it looks like it'd cause this loop to always 
> return false on the first iteration):
> 
>   if (!std::get<0>(Pair) || !std::get<1>(Pair))
> 
> & thanks for the explanation about the approach you were using - I see where 
> you're coming from. I'd personally still lean this ^ way, I think.
> 
> (maybe if we get super ridiculous one day, and have a monadic (not sure if 
> that's the right word) sort of conditional accessor for Optional (where you 
> pass in a lambda over T, returning U, and the result is Optional) we could 
> write this in terms of that & then the != would fit right in... though would 
> be a bit verbose/quirky to be sure)
I knew exactly what you suggested -- I considered before going with the `!=` 
version -- it seems It also only saw what I wanted to see. I still just 
copy&pasted from your comment to save some keystrokes. Maybe the `!=` is less 
error-prone, as just demonstrated? Test cases did not fail.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Meinersbur marked an inline comment as done.
Closed by commit rC348762: Use zip_longest for iterator range comparisons. NFC. 
(authored by Meinersbur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55468?vs=177447&id=177506#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8977,25 +8977,28 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
 
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!Cand1A)
   return Comparison::Worse;
+if (!Cand2A)
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1I

[PATCH] D58638: [OpenMP 5.0] Parsing/sema support for from clause with mapper modifier

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354817: [OpenMP 5.0] Parsing/sema support for from clause 
with mapper modifier. (authored by Meinersbur, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58638?vs=188215&id=188241#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58638

Files:
  cfe/trunk/include/clang/AST/OpenMPClause.h
  cfe/trunk/include/clang/Basic/OpenMPKinds.def
  cfe/trunk/include/clang/Basic/OpenMPKinds.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/OpenMPClause.cpp
  cfe/trunk/lib/Basic/OpenMPKinds.cpp
  cfe/trunk/lib/Parse/ParseOpenMP.cpp
  cfe/trunk/lib/Sema/SemaOpenMP.cpp
  cfe/trunk/lib/Sema/TreeTransform.h
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/OpenMP/declare_mapper_ast_print.c
  cfe/trunk/test/OpenMP/declare_mapper_ast_print.cpp
  cfe/trunk/test/OpenMP/declare_mapper_codegen.cpp
  cfe/trunk/test/OpenMP/declare_mapper_messages.c
  cfe/trunk/test/OpenMP/declare_mapper_messages.cpp

Index: cfe/trunk/lib/Basic/OpenMPKinds.cpp
===
--- cfe/trunk/lib/Basic/OpenMPKinds.cpp
+++ cfe/trunk/lib/Basic/OpenMPKinds.cpp
@@ -122,6 +122,12 @@
   .Case(#Name, static_cast(OMPC_TO_MODIFIER_##Name))
 #include "clang/Basic/OpenMPKinds.def"
 .Default(OMPC_TO_MODIFIER_unknown);
+  case OMPC_from:
+return llvm::StringSwitch(Str)
+#define OPENMP_FROM_MODIFIER_KIND(Name) \
+  .Case(#Name, static_cast(OMPC_FROM_MODIFIER_##Name))
+#include "clang/Basic/OpenMPKinds.def"
+.Default(OMPC_FROM_MODIFIER_unknown);
   case OMPC_dist_schedule:
 return llvm::StringSwitch(Str)
 #define OPENMP_DIST_SCHEDULE_KIND(Name) .Case(#Name, OMPC_DIST_SCHEDULE_##Name)
@@ -180,7 +186,6 @@
   case OMPC_num_tasks:
   case OMPC_hint:
   case OMPC_uniform:
-  case OMPC_from:
   case OMPC_use_device_ptr:
   case OMPC_is_device_ptr:
   case OMPC_unified_address:
@@ -277,6 +282,18 @@
   break;
 }
 llvm_unreachable("Invalid OpenMP 'to' clause type");
+  case OMPC_from:
+switch (Type) {
+case OMPC_FROM_MODIFIER_unknown:
+  return "unknown";
+#define OPENMP_FROM_MODIFIER_KIND(Name)\
+  case OMPC_FROM_MODIFIER_##Name:  \
+return #Name;
+#include "clang/Basic/OpenMPKinds.def"
+default:
+  break;
+}
+llvm_unreachable("Invalid OpenMP 'from' clause type");
   case OMPC_dist_schedule:
 switch (Type) {
 case OMPC_DIST_SCHEDULE_unknown:
@@ -350,7 +367,6 @@
   case OMPC_num_tasks:
   case OMPC_hint:
   case OMPC_uniform:
-  case OMPC_from:
   case OMPC_use_device_ptr:
   case OMPC_is_device_ptr:
   case OMPC_unified_address:
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -12448,6 +12448,10 @@
 
 void OMPClauseReader::VisitOMPFromClause(OMPFromClause *C) {
   C->setLParenLoc(Record.readSourceLocation());
+  C->setMapperQualifierLoc(Record.readNestedNameSpecifierLoc());
+  DeclarationNameInfo DNI;
+  Record.readDeclarationNameInfo(DNI);
+  C->setMapperIdInfo(DNI);
   auto NumVars = C->varlist_size();
   auto UniqueDecls = C->getUniqueDeclarationsNum();
   auto TotalLists = C->getTotalComponentListNum();
@@ -12459,6 +12463,12 @@
 Vars.push_back(Record.readSubExpr());
   C->setVarRefs(Vars);
 
+  SmallVector UDMappers;
+  UDMappers.reserve(NumVars);
+  for (unsigned I = 0; I < NumVars; ++I)
+UDMappers.push_back(Record.readSubExpr());
+  C->setUDMapperRefs(UDMappers);
+
   SmallVector Decls;
   Decls.reserve(UniqueDecls);
   for (unsigned i = 0; i < UniqueDecls; ++i)
Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -6885,8 +6885,12 @@
   Record.push_back(C->getTotalComponentListNum());
   Record.push_back(C->getTotalComponentsNum());
   Record.AddSourceLocation(C->getLParenLoc());
+  Record.AddNestedNameSpecifierLoc(C->getMapperQualifierLoc());
+  Record.AddDeclarationNameInfo(C->getMapperIdInfo());
   for (auto *E : C->varlists())
 Record.AddStmt(E);
+  for (auto *E : C->mapperlists())
+Record.AddStmt(E);
   for (auto *D : C->all_decls())
 Record.AddDeclRef(D);
   for (auto N : C->all_num_lists())
Index: cfe/trunk/lib/Parse/ParseOpenMP.cpp
===
--- cfe/trunk/lib/Parse/ParseOpenMP.cpp
+++ cfe/trunk/lib/Parse/ParseOpenMP.cpp
@@ -2161,13 +2161,20 @@
 
 if (Tok.is(tok::colon))
   Data.ColonLoc = ConsumeToken()

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

ping

The Polly-powered additional transformations 
 now also generate this kind of 
metadata.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57978



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

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D57978



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

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 192194.
Meinersbur marked 5 inline comments as done.
Meinersbur added a comment.

- Rebase
- Add two test cases for all attributes combined (as inner and outer loop for 
of an unroll-and-jam)
- Of two nested unroll-and-jams, apply the inner first
- Typos and comment clarification
- Add missing llvm.loop.isvectorized when attributes are split between 
BeforeJam and AfterJam


Repository:
  rC Clang

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

https://reviews.llvm.org/D57978

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-followup_inner.cpp
  test/CodeGenCXX/pragma-followup_outer.cpp
  test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/CodeGenCXX/pragma-loop.cpp
  test/CodeGenCXX/pragma-unroll-and-jam.cpp
  test/OpenMP/simd_metadata.c

Index: test/OpenMP/simd_metadata.c
===
--- test/OpenMP/simd_metadata.c
+++ test/OpenMP/simd_metadata.c
@@ -147,16 +147,16 @@
 // CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8:![0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: [[LOOP_WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[ACCESS_GROUP_7]] = distinct !{}
-// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_9:[0-9]+]]}
+// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], ![[PARALLEL_ACCESSES_9:[0-9]+]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_9]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_7]]}
 //
 // Metadata for h2:
 // CHECK: ![[ACCESS_GROUP_10]] = distinct !{}
-// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_12:[0-9]+]]}
+// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], ![[PARALLEL_ACCESSES_12:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_12]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_10]]}
 //
 // Metadata for h3:
 // CHECK: ![[ACCESS_GROUP_13]] = distinct !{}
-// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_15:[0-9]+]]}
+// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], ![[PARALLEL_ACCESSES_15:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_15]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_13]]}
 //
Index: test/CodeGenCXX/pragma-unroll-and-jam.cpp
===
--- test/CodeGenCXX/pragma-unroll-and-jam.cpp
+++ test/CodeGenCXX/pragma-unroll-and-jam.cpp
@@ -51,5 +51,5 @@
 // CHECK: ![[UNJ_4]] = !{!"llvm.loop.unroll_and_jam.count", i32 4}
 // CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[UNJ_DISABLE:.*]]}
 // CHECK: ![[UNJ_DISABLE]] = !{!"llvm.loop.unroll_and_jam.disable"}
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[UNROLL_4:.*]], ![[UNJ_DISABLE:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[UNJ_DISABLE:.*]], ![[UNROLL_4:.*]]}
 // CHECK: ![[UNROLL_4]] = !{!"llvm.loop.unroll.count", i32 4}
Index: test/CodeGenCXX/pragma-loop.cpp
===
--- test/CodeGenCXX/pragma-loop.cpp
+++ test/CodeGenCXX/pragma-loop.cpp
@@ -158,37 +158,60 @@
   for_template_constant_expression_test(List, Length);
 }
 
-// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[WIDTH_4:.*]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[UNROLL_FULL:.*]], ![[DISTRIBUTE_ENABLE:.*]]}
-// CHECK: ![[WIDTH_4]] = !{!"llvm.loop.vectorize.width", i32 4}
-// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
-// CHECK: ![[DISTRIBUTE_ENABLE]] = !{!"llvm.loop.distribute.enable", i1 true}
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]]}
-// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
+
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[UNROLL_8:.*]], ![[INTENABLE_1:.*]]}
+// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
+// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
+// CHECK: ![[INTENAB

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D57978#1440894 , @dmgreen wrote:

> This could maybe do with a few extra tests. Am I correct in saying something 
> like this:
>
>   #pragma unroll_and_jam(4)
>   for(int j = 0; j < n; j++) {
> #pragma unroll(4)
> for(int k = 0; k < n; k++) {
>   x[j*n+k] = 10;
> }
>   }
>
>
> would end up with a llvm.loop.unroll_and_jam.followup_inner with a 
> llvm.loop.unroll_count?


correct. I added a new test `pragma-followup_inner.cpp` where this can be seen.




Comment at: lib/CodeGen/CGLoopInfo.cpp:500
+// Unroll-and-jam of an inner loop and unroll-and-jam of the same loop as
+// the outer loop does not make much sense, but we have to pick an order.
+AfterJam.UnrollAndJamCount = Attrs.UnrollAndJamCount;

dmgreen wrote:
> I was having trouble parsing this sentance. Does it mean both the inner loop 
> and the outer loop both have unroll-and-jam? UnrollAndJam processes loops 
> from inner to outer, so if this is working as I think, maybe it should be put 
> into BeforeJam.
> Does it mean both the inner loop and the outer loop both have unroll-and-jam? 

Correct. I was thinking of such a case:
```
#pragma unroll_and_jam(2)
for (int i = 0; i < m; i+=1)
  #pragma unroll_and_jam(3)
  for (int j = 0; j < n; j+=1)
```

Assuming the inner unroll-and-jam would do something, would it happen before or 
after the outer transformation. As you mentioned, there is an argument to put 
it into `BeforeJam`. Changed.



Comment at: lib/CodeGen/CGLoopInfo.cpp:502
+AfterJam.UnrollAndJamCount = Attrs.UnrollAndJamCount;
+AfterJam.UnrollAndJamEnable = AfterJam.UnrollAndJamEnable;
+

dmgreen wrote:
>  = Attrs.UnrollAndJamEnable ?
thanks


Repository:
  rC Clang

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

https://reviews.llvm.org/D57978



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

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357415: [CodeGen] Generate follow-up metadata for loops with 
more than one… (authored by Meinersbur, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57978?vs=192194&id=193128#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57978

Files:
  cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
  cfe/trunk/lib/CodeGen/CGLoopInfo.h
  cfe/trunk/test/CodeGenCXX/pragma-followup_inner.cpp
  cfe/trunk/test/CodeGenCXX/pragma-followup_outer.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety-nested.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety-outer.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
  cfe/trunk/test/CodeGenCXX/pragma-unroll-and-jam.cpp
  cfe/trunk/test/OpenMP/simd_metadata.c

Index: cfe/trunk/test/OpenMP/simd_metadata.c
===
--- cfe/trunk/test/OpenMP/simd_metadata.c
+++ cfe/trunk/test/OpenMP/simd_metadata.c
@@ -147,16 +147,16 @@
 // CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8:![0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: [[LOOP_WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[ACCESS_GROUP_7]] = distinct !{}
-// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_9:[0-9]+]]}
+// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], ![[PARALLEL_ACCESSES_9:[0-9]+]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_9]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_7]]}
 //
 // Metadata for h2:
 // CHECK: ![[ACCESS_GROUP_10]] = distinct !{}
-// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_12:[0-9]+]]}
+// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], ![[PARALLEL_ACCESSES_12:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_12]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_10]]}
 //
 // Metadata for h3:
 // CHECK: ![[ACCESS_GROUP_13]] = distinct !{}
-// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_15:[0-9]+]]}
+// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], ![[PARALLEL_ACCESSES_15:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_15]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_13]]}
 //
Index: cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
===
--- cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
+++ cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
@@ -158,37 +158,60 @@
   for_template_constant_expression_test(List, Length);
 }
 
-// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[WIDTH_4:.*]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[UNROLL_FULL:.*]], ![[DISTRIBUTE_ENABLE:.*]]}
-// CHECK: ![[WIDTH_4]] = !{!"llvm.loop.vectorize.width", i32 4}
-// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
-// CHECK: ![[DISTRIBUTE_ENABLE]] = !{!"llvm.loop.distribute.enable", i1 true}
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]]}
-// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
+
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[UNROLL_8:.*]], ![[INTENABLE_1:.*]]}
+// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
+// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
+// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
+// CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
+// CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
+
 // CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: ![[INTERLEAV

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@dmgreen Thank you for the review!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57978



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

[PATCH] D60749: [Test] Remove obsolete test.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, aaron.ballman, tyler.nowicki, TylerNowicki.
Meinersbur added a project: clang.

The FIXME of this test case has been addressed in r335084/r338800. Its 
execution still does not succeed because of multiple syntax errors.

First, the "clang" namespace is missing on each of the 4 pragmas. Second, the 
pragma for defining the vector width is "vectorize_width(4)" instead of 
"vectorize(4)". Third, the pragma for defining the interleave factor is 
"interleave_count(8)" instead of "interleave(8)".

The file was already using the wrong syntax when added in r210925 2014-06-13. 
The file ast-print-pragmas.cpp already checks for the correct pragma order, 
making this test redundant even if fixed.


Repository:
  rC Clang

https://reviews.llvm.org/D60749

Files:
  test/AST/ast-print-pragmas-xfail.cpp


Index: test/AST/ast-print-pragmas-xfail.cpp
===
--- test/AST/ast-print-pragmas-xfail.cpp
+++ /dev/null
@@ -1,21 +0,0 @@
-// RUN: %clang_cc1 %s -ast-print -o - | FileCheck %s
-
-// FIXME: Test fails because attribute order is reversed by ParsedAttributes.
-// XFAIL: *
-
-void run1(int *List, int Length) {
-  int i = 0;
-// CHECK: #pragma loop vectorize(4)
-// CHECK-NEXT: #pragma loop interleave(8)
-// CHECK-NEXT: #pragma loop vectorize(enable)
-// CHECK-NEXT: #pragma loop interleave(enable)
-#pragma loop vectorize(4)
-#pragma loop interleave(8)
-#pragma loop vectorize(enable)
-#pragma loop interleave(enable)
-// CHECK-NEXT: while (i < Length)
-  while (i < Length) {
-List[i] = i;
-i++;
-  }
-}


Index: test/AST/ast-print-pragmas-xfail.cpp
===
--- test/AST/ast-print-pragmas-xfail.cpp
+++ /dev/null
@@ -1,21 +0,0 @@
-// RUN: %clang_cc1 %s -ast-print -o - | FileCheck %s
-
-// FIXME: Test fails because attribute order is reversed by ParsedAttributes.
-// XFAIL: *
-
-void run1(int *List, int Length) {
-  int i = 0;
-// CHECK: #pragma loop vectorize(4)
-// CHECK-NEXT: #pragma loop interleave(8)
-// CHECK-NEXT: #pragma loop vectorize(enable)
-// CHECK-NEXT: #pragma loop interleave(enable)
-#pragma loop vectorize(4)
-#pragma loop interleave(8)
-#pragma loop vectorize(enable)
-#pragma loop interleave(enable)
-// CHECK-NEXT: while (i < Length)
-  while (i < Length) {
-List[i] = i;
-i++;
-  }
-}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60749: [Test] Remove obsolete test.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358507: [Test] Remove obsolete test. (authored by 
Meinersbur, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D60749

Files:
  test/AST/ast-print-pragmas-xfail.cpp


Index: test/AST/ast-print-pragmas-xfail.cpp
===
--- test/AST/ast-print-pragmas-xfail.cpp
+++ test/AST/ast-print-pragmas-xfail.cpp
@@ -1,21 +0,0 @@
-// RUN: %clang_cc1 %s -ast-print -o - | FileCheck %s
-
-// FIXME: Test fails because attribute order is reversed by ParsedAttributes.
-// XFAIL: *
-
-void run1(int *List, int Length) {
-  int i = 0;
-// CHECK: #pragma loop vectorize(4)
-// CHECK-NEXT: #pragma loop interleave(8)
-// CHECK-NEXT: #pragma loop vectorize(enable)
-// CHECK-NEXT: #pragma loop interleave(enable)
-#pragma loop vectorize(4)
-#pragma loop interleave(8)
-#pragma loop vectorize(enable)
-#pragma loop interleave(enable)
-// CHECK-NEXT: while (i < Length)
-  while (i < Length) {
-List[i] = i;
-i++;
-  }
-}


Index: test/AST/ast-print-pragmas-xfail.cpp
===
--- test/AST/ast-print-pragmas-xfail.cpp
+++ test/AST/ast-print-pragmas-xfail.cpp
@@ -1,21 +0,0 @@
-// RUN: %clang_cc1 %s -ast-print -o - | FileCheck %s
-
-// FIXME: Test fails because attribute order is reversed by ParsedAttributes.
-// XFAIL: *
-
-void run1(int *List, int Length) {
-  int i = 0;
-// CHECK: #pragma loop vectorize(4)
-// CHECK-NEXT: #pragma loop interleave(8)
-// CHECK-NEXT: #pragma loop vectorize(enable)
-// CHECK-NEXT: #pragma loop interleave(enable)
-#pragma loop vectorize(4)
-#pragma loop interleave(8)
-#pragma loop vectorize(enable)
-#pragma loop interleave(enable)
-// CHECK-NEXT: while (i < Length)
-  while (i < Length) {
-List[i] = i;
-i++;
-  }
-}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52118: [Loopinfo] Remove one latch case in getLoopID. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D52118#1235897, @jdoerfert wrote:

> > save an iteration over the loop's basic blocks (which is what getLoopLatch 
> > does)
>
> I'm not sure this is true. getLoopLatch() in LoopInfoImpl.h 
>  only traverses the children of the header in the inverse graph.
>  That should, I think, be similar to predecessors(Header) in case
>  of the IR CFG.


You're right. I changed the summary/commit message accordingly.


Repository:
  rC Clang

https://reviews.llvm.org/D52118



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

[PATCH] D52118: [Loopinfo] Remove one latch case in getLoopID. NFC.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342406: [Loopinfo] Remove one latch-case in getLoopID. NFC. 
(authored by Meinersbur, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52118?vs=165567&id=165798#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52118

Files:
  llvm/trunk/lib/Analysis/LoopInfo.cpp


Index: llvm/trunk/lib/Analysis/LoopInfo.cpp
===
--- llvm/trunk/lib/Analysis/LoopInfo.cpp
+++ llvm/trunk/lib/Analysis/LoopInfo.cpp
@@ -213,26 +213,21 @@
 
 MDNode *Loop::getLoopID() const {
   MDNode *LoopID = nullptr;
-  if (BasicBlock *Latch = getLoopLatch()) {
-LoopID = Latch->getTerminator()->getMetadata(LLVMContext::MD_loop);
-  } else {
-assert(!getLoopLatch() &&
-   "The loop should have no single latch at this point");
-// Go through the latch blocks and check the terminator for the metadata.
-SmallVector LatchesBlocks;
-getLoopLatches(LatchesBlocks);
-for (BasicBlock *BB : LatchesBlocks) {
-  TerminatorInst *TI = BB->getTerminator();
-  MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
-
-  if (!MD)
-return nullptr;
-
-  if (!LoopID)
-LoopID = MD;
-  else if (MD != LoopID)
-return nullptr;
-}
+
+  // Go through the latch blocks and check the terminator for the metadata.
+  SmallVector LatchesBlocks;
+  getLoopLatches(LatchesBlocks);
+  for (BasicBlock *BB : LatchesBlocks) {
+TerminatorInst *TI = BB->getTerminator();
+MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
+
+if (!MD)
+  return nullptr;
+
+if (!LoopID)
+  LoopID = MD;
+else if (MD != LoopID)
+  return nullptr;
   }
   if (!LoopID || LoopID->getNumOperands() == 0 ||
   LoopID->getOperand(0) != LoopID)


Index: llvm/trunk/lib/Analysis/LoopInfo.cpp
===
--- llvm/trunk/lib/Analysis/LoopInfo.cpp
+++ llvm/trunk/lib/Analysis/LoopInfo.cpp
@@ -213,26 +213,21 @@
 
 MDNode *Loop::getLoopID() const {
   MDNode *LoopID = nullptr;
-  if (BasicBlock *Latch = getLoopLatch()) {
-LoopID = Latch->getTerminator()->getMetadata(LLVMContext::MD_loop);
-  } else {
-assert(!getLoopLatch() &&
-   "The loop should have no single latch at this point");
-// Go through the latch blocks and check the terminator for the metadata.
-SmallVector LatchesBlocks;
-getLoopLatches(LatchesBlocks);
-for (BasicBlock *BB : LatchesBlocks) {
-  TerminatorInst *TI = BB->getTerminator();
-  MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
-
-  if (!MD)
-return nullptr;
-
-  if (!LoopID)
-LoopID = MD;
-  else if (MD != LoopID)
-return nullptr;
-}
+
+  // Go through the latch blocks and check the terminator for the metadata.
+  SmallVector LatchesBlocks;
+  getLoopLatches(LatchesBlocks);
+  for (BasicBlock *BB : LatchesBlocks) {
+TerminatorInst *TI = BB->getTerminator();
+MDNode *MD = TI->getMetadata(LLVMContext::MD_loop);
+
+if (!MD)
+  return nullptr;
+
+if (!LoopID)
+  LoopID = MD;
+else if (MD != LoopID)
+  return nullptr;
   }
   if (!LoopID || LoopID->getNumOperands() == 0 ||
   LoopID->getOperand(0) != LoopID)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

ping


Repository:
  rC Clang

https://reviews.llvm.org/D50214



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

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur closed this revision.
Meinersbur added a comment.

Commited as https://reviews.llvm.org/rL342861.


Repository:
  rC Clang

https://reviews.llvm.org/D50214



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

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 167099.
Meinersbur added a comment.
Herald added subscribers: llvm-commits, dexonsmith, steven_wu, eraman, 
mehdi_amini.

- Rebase
- Use call access group if instruction's access group is not set


Repository:
  rL LLVM

https://reviews.llvm.org/D52117

Files:
  docs/LangRef.rst
  include/llvm/IR/LLVMContext.h
  include/llvm/Transforms/Utils/LoopUtils.h
  lib/Analysis/LoopInfo.cpp
  lib/IR/LLVMContext.cpp
  lib/Transforms/InstCombine/InstCombineCalls.cpp
  lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
  lib/Transforms/Scalar/LoopVersioningLICM.cpp
  lib/Transforms/Scalar/SROA.cpp
  lib/Transforms/Scalar/Scalarizer.cpp
  lib/Transforms/Utils/InlineFunction.cpp
  lib/Transforms/Utils/Local.cpp
  lib/Transforms/Utils/LoopUtils.cpp
  lib/Transforms/Utils/SimplifyCFG.cpp
  test/ThinLTO/X86/lazyload_metadata.ll
  test/Transforms/Inline/parallel-loop-md-callee.ll
  test/Transforms/Inline/parallel-loop-md.ll
  test/Transforms/InstCombine/loadstore-metadata.ll
  test/Transforms/InstCombine/mem-par-metadata-memcpy.ll
  test/Transforms/LoopVectorize/X86/force-ifcvt.ll
  test/Transforms/LoopVectorize/X86/illegal-parallel-loop-uniform-write.ll
  test/Transforms/LoopVectorize/X86/parallel-loops-after-reg2mem.ll
  test/Transforms/LoopVectorize/X86/parallel-loops.ll
  test/Transforms/LoopVectorize/X86/pr34438.ll
  test/Transforms/LoopVectorize/X86/vect.omp.force.ll
  test/Transforms/LoopVectorize/X86/vect.omp.force.small-tc.ll
  test/Transforms/LoopVectorize/X86/vector_max_bandwidth.ll
  test/Transforms/SROA/mem-par-metadata-sroa.ll
  test/Transforms/Scalarizer/basic.ll
  test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll

Index: test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
===
--- test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
+++ test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
@@ -8,39 +8,39 @@
   br label %for.body
 
 ; CHECK-LABEL: @Test
-; CHECK: load i32, i32* {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
-; CHECK: load i32, i32* {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
-; CHECK: store i32 {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
+; CHECK: load i32, i32* {{.*}}, align 4, !llvm.access.group !0
+; CHECK: load i32, i32* {{.*}}, align 4, !llvm.access.group !0
+; CHECK: store i32 {{.*}}, align 4, !llvm.access.group !0
 ; CHECK-NOT: load
 ; CHECK-NOT: store
 
 for.body: ; preds = %cond.end, %entry
   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %cond.end ]
   %arrayidx = getelementptr inbounds i32, i32* %p, i64 %indvars.iv
-  %0 = load i32, i32* %arrayidx, align 4, !llvm.mem.parallel_loop_access !0
+  %0 = load i32, i32* %arrayidx, align 4, !llvm.access.group !0
   %cmp1 = icmp eq i32 %0, 0
   br i1 %cmp1, label %cond.true, label %cond.false
 
 cond.false:   ; preds = %for.body
   %arrayidx3 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  %v = load i32, i32* %arrayidx3, align 4, !llvm.mem.parallel_loop_access !0
+  %v = load i32, i32* %arrayidx3, align 4, !llvm.access.group !0
   %arrayidx7 = getelementptr inbounds i32, i32* %d, i64 %indvars.iv
-  %1 = load i32, i32* %arrayidx7, align 4, !llvm.mem.parallel_loop_access !0
+  %1 = load i32, i32* %arrayidx7, align 4, !llvm.access.group !0
   %add = add nsw i32 %1, %v
   br label %cond.end
 
 cond.true:   ; preds = %for.body
   %arrayidx4 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  %w = load i32, i32* %arrayidx4, align 4, !llvm.mem.parallel_loop_access !0
+  %w = load i32, i32* %arrayidx4, align 4, !llvm.access.group !0
   %arrayidx8 = getelementptr inbounds i32, i32* %d, i64 %indvars.iv
-  %2 = load i32, i32* %arrayidx8, align 4, !llvm.mem.parallel_loop_access !0
+  %2 = load i32, i32* %arrayidx8, align 4, !llvm.access.group !0
   %add2 = add nsw i32 %2, %w
   br label %cond.end
 
 cond.end: ; preds = %for.body, %cond.false
   %cond = phi i32 [ %add, %cond.false ], [ %add2, %cond.true ]
   %arrayidx9 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  store i32 %cond, i32* %arrayidx9, align 4, !llvm.mem.parallel_loop_access !0
+  store i32 %cond, i32* %arrayidx9, align 4, !llvm.access.group !0
   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
   %exitcond = icmp eq i64 %indvars.iv.next, 16
   br i1 %exitcond, label %for.end, label %for.body, !llvm.loop !0
@@ -51,5 +51,6 @@
 
 attributes #0 = { norecurse nounwind uwtable }
 
-!0 = distinct !{!0, !1}
+!0 = distinct !{!0, !1, !{!"llvm.loop.parallel_accesses", !10}}
 !1 = !{!"llvm.loop.vectorize.enable", i1 true}
+!10 = distinct !{}
Index: test/Transforms/Scalarizer/basic.ll
===
--- test/Transforms/Scalarizer/basic.ll
+++ test/Transforms/Scalarizer/basic.ll
@@ -205,28 +205,28 @@
   ret void
 }
 
-; 

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

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

- Upload diff for clang portion (instead of https://reviews.llvm.org/D52116)


Repository:
  rC Clang

https://reviews.llvm.org/D52117

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/OpenMP/for_codegen.cpp
  test/OpenMP/for_simd_codegen.cpp
  test/OpenMP/loops_explicit_clauses_codegen.cpp
  test/OpenMP/ordered_codegen.cpp
  test/OpenMP/parallel_for_simd_codegen.cpp
  test/OpenMP/schedule_codegen.cpp
  test/OpenMP/simd_codegen.cpp
  test/OpenMP/simd_metadata.c
  test/OpenMP/target_parallel_for_simd_codegen.cpp
  test/OpenMP/target_simd_codegen.cpp
  test/OpenMP/taskloop_simd_codegen.cpp

Index: test/OpenMP/taskloop_simd_codegen.cpp
===
--- test/OpenMP/taskloop_simd_codegen.cpp
+++ test/OpenMP/taskloop_simd_codegen.cpp
@@ -83,17 +83,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP1]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK2]](
@@ -113,17 +113,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP2]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK3]](
@@ -142,7 +142,7 @@
 // CHECK: [[LB_VAL:%.+]] = load i64, i64* [[LB]],
 // CHECK: store i64 [[LB_VAL]], i64* [[CNT:%.+]],
 // CHECK: br label
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
@@ -192,14 +192,14 @@
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: store i32 %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
 // CHECK: store i32 %{{.+}}, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
Index: test/OpenMP/target_simd_codegen.cpp
===
--- test/OpenMP/target_simd_codegen.cpp
+++ test/OpenMP/target_simd_codegen.cpp
@@ -342,7 +342,7 @@
 // CHECK-64:[[AA_CADDR:%.+]] = bitcast i[[SZ]]* [[AA_ADDR]] to i32*
 // CHECK-64:   

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 168251.
Meinersbur marked 2 inline comments as done.
Meinersbur added a comment.

- Address @pekka.jaaskelainen's review.


Repository:
  rC Clang

https://reviews.llvm.org/D52117

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/OpenMP/for_codegen.cpp
  test/OpenMP/for_simd_codegen.cpp
  test/OpenMP/loops_explicit_clauses_codegen.cpp
  test/OpenMP/ordered_codegen.cpp
  test/OpenMP/parallel_for_simd_codegen.cpp
  test/OpenMP/schedule_codegen.cpp
  test/OpenMP/simd_codegen.cpp
  test/OpenMP/simd_metadata.c
  test/OpenMP/target_parallel_for_simd_codegen.cpp
  test/OpenMP/target_simd_codegen.cpp
  test/OpenMP/taskloop_simd_codegen.cpp

Index: test/OpenMP/taskloop_simd_codegen.cpp
===
--- test/OpenMP/taskloop_simd_codegen.cpp
+++ test/OpenMP/taskloop_simd_codegen.cpp
@@ -83,17 +83,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP1]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK2]](
@@ -113,17 +113,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP2]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK3]](
@@ -142,7 +142,7 @@
 // CHECK: [[LB_VAL:%.+]] = load i64, i64* [[LB]],
 // CHECK: store i64 [[LB_VAL]], i64* [[CNT:%.+]],
 // CHECK: br label
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
@@ -192,14 +192,14 @@
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: store i32 %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
 // CHECK: store i32 %{{.+}}, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
Index: test/OpenMP/target_simd_codegen.cpp
===
--- test/OpenMP/target_simd_codegen.cpp
+++ test/OpenMP/target_simd_codegen.cpp
@@ -342,7 +342,7 @@
 // CHECK-64:[[AA

[PATCH] D56326: [OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@lildmh Thanks for the patch. I can commit this patch for you. The tests run 
fine on my machine. However, for committing on other's behalf, we were asked to 
make the contributor aware that the contribution will be published under a new 
license 
.
 Are you OK with the new license?


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

https://reviews.llvm.org/D56326



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

[PATCH] D56326: [OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352906: [OpenMP 5.0] Parsing/sema support for "omp 
declare mapper" directive. (authored by Meinersbur, committed by ).
Herald added a project: clang.

Repository:
  rC Clang

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

https://reviews.llvm.org/D56326

Files:
  include/clang/AST/DeclBase.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/DeclOpenMP.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DeclNodes.td
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/OpenMPKinds.def
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/ASTDumper.cpp
  lib/AST/CXXInheritance.cpp
  lib/AST/DeclBase.cpp
  lib/AST/DeclOpenMP.cpp
  lib/AST/DeclPrinter.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/Basic/OpenMPKinds.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTCommon.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/OpenMP/declare_mapper_ast_print.c
  test/OpenMP/declare_mapper_ast_print.cpp
  test/OpenMP/declare_mapper_messages.c
  test/OpenMP/declare_mapper_messages.cpp
  tools/libclang/CIndex.cpp

Index: include/clang/AST/DeclOpenMP.h
===
--- include/clang/AST/DeclOpenMP.h
+++ include/clang/AST/DeclOpenMP.h
@@ -206,6 +206,108 @@
   }
 };
 
+/// This represents '#pragma omp declare mapper ...' directive. Map clauses are
+/// allowed to use with this directive. The following example declares a user
+/// defined mapper for the type 'struct vec'. This example instructs the fields
+/// 'len' and 'data' should be mapped when mapping instances of 'struct vec'.
+///
+/// \code
+/// #pragma omp declare mapper(mid: struct vec v) map(v.len, v.data[0:N])
+/// \endcode
+class OMPDeclareMapperDecl final : public ValueDecl, public DeclContext {
+  friend class ASTDeclReader;
+
+  /// Clauses assoicated with this mapper declaration
+  MutableArrayRef Clauses;
+
+  /// Mapper variable, which is 'v' in the example above
+  Expr *MapperVarRef = nullptr;
+
+  /// Name of the mapper variable
+  DeclarationName VarName;
+
+  LazyDeclPtr PrevDeclInScope;
+
+  virtual void anchor();
+
+  OMPDeclareMapperDecl(Kind DK, DeclContext *DC, SourceLocation L,
+   DeclarationName Name, QualType Ty,
+   DeclarationName VarName,
+   OMPDeclareMapperDecl *PrevDeclInScope)
+  : ValueDecl(DK, DC, L, Name, Ty), DeclContext(DK), VarName(VarName),
+PrevDeclInScope(PrevDeclInScope) {}
+
+  void setPrevDeclInScope(OMPDeclareMapperDecl *Prev) {
+PrevDeclInScope = Prev;
+  }
+
+  /// Sets an array of clauses to this mapper declaration
+  void setClauses(ArrayRef CL);
+
+public:
+  /// Creates declare mapper node.
+  static OMPDeclareMapperDecl *Create(ASTContext &C, DeclContext *DC,
+  SourceLocation L, DeclarationName Name,
+  QualType T, DeclarationName VarName,
+  OMPDeclareMapperDecl *PrevDeclInScope);
+  /// Creates deserialized declare mapper node.
+  static OMPDeclareMapperDecl *CreateDeserialized(ASTContext &C, unsigned ID,
+  unsigned N);
+
+  /// Creates an array of clauses to this mapper declaration and intializes
+  /// them.
+  void CreateClauses(ASTContext &C, ArrayRef CL);
+
+  using clauselist_iterator = MutableArrayRef::iterator;
+  using clauselist_const_iterator = ArrayRef::iterator;
+  using clauselist_range = llvm::iterator_range;
+  using clauselist_const_range =
+  llvm::iterator_range;
+
+  unsigned clauselist_size() const { return Clauses.size(); }
+  bool clauselist_empty() const { return Clauses.empty(); }
+
+  clauselist_range clauselists() {
+return clauselist_range(clauselist_begin(), clauselist_end());
+  }
+  clauselist_const_range clauselists() const {
+return clauselist_const_range(clauselist_begin(), clauselist_end());
+  }
+  clauselist_iterator clauselist_begin() { return Clauses.begin(); }
+  clauselist_iterator clauselist_end() { return Clauses.end(); }
+  clauselist_const_iterator clauselist_begin() const { return Clauses.begin(); }
+  clauselist_const_iterator clauselist_end() const { return Clauses.end(); }
+
+  /// Get the variable declared in the mapper
+  Expr *getMapperVarRef() { return MapperVarRef; }
+  const Expr *getMapperVarRef() const { return MapperVarRef; }
+ 

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, aaron.ballman, hsaito, dmgreen, anemet, 
rjmccall, Anastasia, pekka.jaaskelainen, meheff, tyler.nowicki.
Herald added a subscriber: zzheng.
Herald added a project: clang.

Before this patch, CGLoop would dump all transformations for a loop into a 
single LoopID without encoding any order in which to apply them. rL348944 
 added the possibility to encode a 
transformation order using followup-attributes.

When a loop has more than one transformation, use the follow-up attribute 
define the order in which they are applied. The emitted order is the defacto 
order as defined by the current LLVM pass pipeline, which is:

1. LoopFullUnrollPass
2. LoopDistributePass
3. LoopVectorizePass
4. LoopUnrollAndJamPass
5. LoopUnrollPass
6. MachinePipeliner

This patch should therefore not change the assembly output, assuming that all 
explicit transformations can be applied, and no implicit transformations 
in-between. In the former case, WarnMissedTransformationsPass should emit a 
warning (except for MachinePipeliner which is not implemented yet). The latter 
could be avoided by adding 'llvm.loop.disable_nonforced' attributes.

Because LoopUnrollAndJamPass processes a loop nest, generation of the MDNode is 
delayed to after the inner loop metadata have been processed. A temporary 
LoopID is therefore used to annotate instructions and RAUW'ed by the actual 
LoopID later.


Repository:
  rC Clang

https://reviews.llvm.org/D57978

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/CodeGenCXX/pragma-loop.cpp
  test/CodeGenCXX/pragma-unroll-and-jam.cpp
  test/OpenMP/simd_metadata.c

Index: test/OpenMP/simd_metadata.c
===
--- test/OpenMP/simd_metadata.c
+++ test/OpenMP/simd_metadata.c
@@ -147,16 +147,16 @@
 // CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8:![0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: [[LOOP_WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[ACCESS_GROUP_7]] = distinct !{}
-// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_9:[0-9]+]]}
+// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], ![[PARALLEL_ACCESSES_9:[0-9]+]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_9]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_7]]}
 //
 // Metadata for h2:
 // CHECK: ![[ACCESS_GROUP_10]] = distinct !{}
-// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_12:[0-9]+]]}
+// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], ![[PARALLEL_ACCESSES_12:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_12]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_10]]}
 //
 // Metadata for h3:
 // CHECK: ![[ACCESS_GROUP_13]] = distinct !{}
-// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_15:[0-9]+]]}
+// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], ![[PARALLEL_ACCESSES_15:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_15]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_13]]}
 //
Index: test/CodeGenCXX/pragma-unroll-and-jam.cpp
===
--- test/CodeGenCXX/pragma-unroll-and-jam.cpp
+++ test/CodeGenCXX/pragma-unroll-and-jam.cpp
@@ -51,5 +51,5 @@
 // CHECK: ![[UNJ_4]] = !{!"llvm.loop.unroll_and_jam.count", i32 4}
 // CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[UNJ_DISABLE:.*]]}
 // CHECK: ![[UNJ_DISABLE]] = !{!"llvm.loop.unroll_and_jam.disable"}
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[UNROLL_4:.*]], ![[UNJ_DISABLE:.*]]}
+// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[UNJ_DISABLE:.*]], ![[UNROLL_4:.*]]}
 // CHECK: ![[UNROLL_4]] = !{!"llvm.loop.unroll.count", i32 4}
Index: test/CodeGenCXX/pragma-loop.cpp
===
--- test/CodeGenCXX/pragma-loop.cpp
+++ test/CodeGenCXX/pragma-loop.cpp
@@ -158,37 +158,60 @@
   for_template_constant_expression_test(List, Length);
 }
 
-// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[WIDTH_4:.*]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[UNROLL_FULL:.*]], ![[DISTRIBUTE_ENABLE:.*]]}
-// CHECK: ![[WIDTH_4]] = !{!"llvm.loop.vectorize.width", i32 4}
-// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
-// CHECK: ![[DISTRIBUTE_ENABLE]] = !{!"llvm.loo

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGLoopInfo.cpp:107
+  FollowupLoopProperties.push_back(
+  MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll.disable")));
+

Anastasia wrote:
> Will this end up emitted in the final module?
This MDNode is emitted in the module generated by clang -- as a follow-up 
attribute -- in case there is another transformation after partial unrolling 
(currently just pipelining).


Repository:
  rC Clang

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

https://reviews.llvm.org/D57978



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

[PATCH] D58523: [OpenMP 5.0] Parsing/sema support for to clause with mapper modifier

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354698: [OpenMP 5.0] Parsing/sema support for to clause with 
mapper modifier. (authored by Meinersbur, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58523?vs=187970&id=187988#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58523

Files:
  cfe/trunk/include/clang/AST/OpenMPClause.h
  cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
  cfe/trunk/include/clang/Basic/OpenMPKinds.def
  cfe/trunk/include/clang/Basic/OpenMPKinds.h
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/OpenMPClause.cpp
  cfe/trunk/lib/Basic/OpenMPKinds.cpp
  cfe/trunk/lib/Parse/ParseOpenMP.cpp
  cfe/trunk/lib/Sema/SemaOpenMP.cpp
  cfe/trunk/lib/Sema/TreeTransform.h
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/test/OpenMP/declare_mapper_ast_print.c
  cfe/trunk/test/OpenMP/declare_mapper_ast_print.cpp
  cfe/trunk/test/OpenMP/declare_mapper_codegen.cpp
  cfe/trunk/test/OpenMP/declare_mapper_messages.c
  cfe/trunk/test/OpenMP/declare_mapper_messages.cpp

Index: cfe/trunk/lib/AST/OpenMPClause.cpp
===
--- cfe/trunk/lib/AST/OpenMPClause.cpp
+++ cfe/trunk/lib/AST/OpenMPClause.cpp
@@ -845,11 +845,11 @@
   return new (Mem) OMPMapClause(Sizes);
 }
 
-OMPToClause *OMPToClause::Create(const ASTContext &C,
- const OMPVarListLocTy &Locs,
- ArrayRef Vars,
- ArrayRef Declarations,
- MappableExprComponentListsRef ComponentLists) {
+OMPToClause *OMPToClause::Create(
+const ASTContext &C, const OMPVarListLocTy &Locs, ArrayRef Vars,
+ArrayRef Declarations,
+MappableExprComponentListsRef ComponentLists, ArrayRef UDMapperRefs,
+NestedNameSpecifierLoc UDMQualifierLoc, DeclarationNameInfo MapperId) {
   OMPMappableExprListSizeTy Sizes;
   Sizes.NumVars = Vars.size();
   Sizes.NumUniqueDeclarations = getUniqueDeclarationsTotalNumber(Declarations);
@@ -857,8 +857,8 @@
   Sizes.NumComponents = getComponentsTotalNumber(ComponentLists);
 
   // We need to allocate:
-  // NumVars x Expr* - we have an original list expression for each clause list
-  // entry.
+  // 2 x NumVars x Expr* - we have an original list expression and an associated
+  // user-defined mapper for each clause list entry.
   // NumUniqueDeclarations x ValueDecl* - unique base declarations associated
   // with each component list.
   // (NumUniqueDeclarations + NumComponentLists) x unsigned - we specify the
@@ -869,13 +869,14 @@
   void *Mem = C.Allocate(
   totalSizeToAlloc(
-  Sizes.NumVars, Sizes.NumUniqueDeclarations,
+  2 * Sizes.NumVars, Sizes.NumUniqueDeclarations,
   Sizes.NumUniqueDeclarations + Sizes.NumComponentLists,
   Sizes.NumComponents));
 
-  OMPToClause *Clause = new (Mem) OMPToClause(Locs, Sizes);
+  auto *Clause = new (Mem) OMPToClause(UDMQualifierLoc, MapperId, Locs, Sizes);
 
   Clause->setVarRefs(Vars);
+  Clause->setUDMapperRefs(UDMapperRefs);
   Clause->setClauseInfo(Declarations, ComponentLists);
   return Clause;
 }
@@ -885,7 +886,7 @@
   void *Mem = C.Allocate(
   totalSizeToAlloc(
-  Sizes.NumVars, Sizes.NumUniqueDeclarations,
+  2 * Sizes.NumVars, Sizes.NumUniqueDeclarations,
   Sizes.NumUniqueDeclarations + Sizes.NumComponentLists,
   Sizes.NumComponents));
   return new (Mem) OMPToClause(Sizes);
@@ -1444,7 +1445,19 @@
 void OMPClausePrinter::VisitOMPToClause(OMPToClause *Node) {
   if (!Node->varlist_empty()) {
 OS << "to";
-VisitOMPClauseList(Node, '(');
+DeclarationNameInfo MapperId = Node->getMapperIdInfo();
+if (MapperId.getName() && !MapperId.getName().isEmpty()) {
+  OS << '(';
+  OS << "mapper(";
+  NestedNameSpecifier *MapperNNS =
+  Node->getMapperQualifierLoc().getNestedNameSpecifier();
+  if (MapperNNS)
+MapperNNS->print(OS, Policy);
+  OS << MapperId << "):";
+  VisitOMPClauseList(Node, ' ');
+} else {
+  VisitOMPClauseList(Node, '(');
+}
 OS << ")";
   }
 }
Index: cfe/trunk/lib/Sema/SemaOpenMP.cpp
===
--- cfe/trunk/lib/Sema/SemaOpenMP.cpp
+++ cfe/trunk/lib/Sema/SemaOpenMP.cpp
@@ -9802,7 +9802,8 @@
DepLinMapLoc, ColonLoc, VarList, Locs);
 break;
   case OMPC_to:
-Res = ActOnOpenMPToClause(VarList, Locs);
+Res = ActOnOpenMPToClause(VarList, ReductionOrMapperIdScopeSpec,
+  ReductionOrMapperId, Locs);
 break;
   case OMPC_from:
 Res = ActOnOpenMPFromClause(VarList, Locs);
@@ -13140,17 +13141,23 @@
 static v

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

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

It still fails with the same error. `LINK_POLLY_INTO_TOOLS` is only set after 
the polly subdirectory is processed. Because it is an `option`, the ON value 
will be stored in the `CMakeCache.txt` and be available during the next run, it 
actually works after running the cmake configure step a second time. We should 
not expect users to do so. Because of this, any `option(...)` or `set(... 
CACHED)` should be done at the beginning of the file.




Comment at: llvm/CMakeLists.txt:497
 
-option(LLVM_POLLY_LINK_INTO_TOOLS "Statically link Polly into tools (if 
available)" ON)
-option(LLVM_POLLY_BUILD "Build LLVM with Polly" ON)

Note that there is `LLVM_POLLY_LINK_INTO_TOOLS` and `LINK_POLLY_INTO_TOOLS`. 
The former is the user-configurable option, the latter is for internal 
querying. At least, this is what is was meant for.



Comment at: llvm/CMakeLists.txt:928
 if( LLVM_INCLUDE_TOOLS )
   add_subdirectory(tools)
 endif()

Polly is included here. `LINK_POLLY_INTO_TOOLS` will not be visible in there if 
set only later.



Comment at: llvm/cmake/modules/AddLLVM.cmake:808
+
+option(LINK_${llvm_extension_upper}_INTO_TOOLS "Statically link 
${llvm_extension_project} into tools (if available)" ON)
+option(LLVM_${llvm_extension_upper}_BUILD "Build LLVM with 
${llvm_extension_project}" ON)

[remark] Use `LLVM_` prefix for LLVM-level options. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This fails with Polly/Linux regression tests:

  
  FAIL: Polly :: Simplify/gemm.ll (1148 of 1149)
   TEST 'Polly :: Simplify/gemm.ll' FAILED 

  Script:
  --
  : 'RUN: at line 1';   opt  -polly-process-unprofitable  
-polly-remarks-minimal  -polly-use-llvm-names  
-polly-import-jscop-dir=/home/meinersbur/src/llvm/tools/polly/test/Simplify  
-polly-codegen-verify  -polly-import-jscop
-polly-import-jscop-postfix=transformed -polly-simplify -analyze < 
/home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll| FileCheck 
/home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll
  --
  Exit Code: 2
  
  Command Output (stderr):
  --
  opt: for the   -o option: may not occur within a group!
  opt: Unknown command line argument '-polly-import-jscop'.  Try: 'opt --help'
  opt: Did you mean '  -o'?
  opt: for the   -p option: may only occur zero or one times!
  opt: for the   -o option: may not occur within a group!
  opt: Unknown command line argument '-polly-simplify'.  Try: 'opt --help'
  opt: Did you mean '  -o'?
  FileCheck error: '-' is empty.
  FileCheck command line:  FileCheck 
/home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll

I think this means that the Polly passes have not been registered 
(`initializePollyPasses` must be called someway in opt/clang_cc1/bugpoint). 
Linking from static libraries will NOT include `Polly.o` (and run its static 
initializers) unless it is needed to resolve at least one symbol.

PLEASE run `make/ninja check-polly` before uploading a patch.




Comment at: llvm/tools/opt/opt.cpp:533
-#ifdef LINK_POLLY_INTO_TOOLS
-  polly::initializePollyPasses(Registry);
-#endif

Where is the equivalent for this in your change? I see it's been done for 
`clang_cc1`, but not for `opt`/`bugpoint`.



Comment at: polly/CMakeLists.txt:210
 set_target_properties(polly-update-format PROPERTIES FOLDER "Polly")
-

[nit] Whitespace change



Comment at: polly/lib/CMakeLists.txt:27
 add_library(PollyCore OBJECT
+  Polly.cpp
   Analysis/DependenceInfo.cpp

Why this change? `Polly.cpp` should only be necessary for the loadable module, 
but not for `LLVM_LINK_POLLY_INTO_TOOLS`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Sorry, you might have tested it with `LLVM_LINK_POLLY_INTO_TOOLS=OFF` and/or 
`BUILD_SHARED_LIBS`/`LLVM_LINK_LLVM_DYLIV` where it might work, but 
unfortunately not with the default configuration using static component 
libraries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Just an idea: We could avoid the explicit calls to 'initializeXYZPass' in 
opt/bugpoint/clang by adding Polly.cpp as a source file or object library to 
the executables. This would guarantee that its static initializer is called 
without dynamic library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

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

In D61446#1533076 , @serge-sans-paille 
wrote:

> That's what I tried to do when always adding Polly.cpp to PollyCore, but it 
> doesn't seem to be enough, I still need to invstigate why (it is actually 
> enough when using BUILD_SHARED_LIBS=ON)


With static libraries, you had the following structure:

  PollyCore.a:
RegisterPasses.o
Polly.o <-- static initializer here, but never used
  
  LLVMPolly.so (for use with -load mechanism)
RegisterPasses.o
Polly.o <-- static initializer here, executed when loading the .so
  
  opt:
main.o <-- call to initializePollyPass removed

With static linking, there is no reason for the linker to add Polly.o to the 
`opt` executable because no symbol from Polly.o (or any of PollyCore) was 
required to resolve any symbol in `opt`. Building a shared library using the 
object library files will include all object files, since it does not know 
which symbols will be used at runtime. I recommend reading 
http://www.lurklurk.org/linkers/linkers.html#staticlibs and 
https://www.bfilipek.com/2018/02/static-vars-static-lib.html.

What I proposed was

  PollyCore.a:
RegisterPasses.o
  
  LLVMPolly.so (for use with -load mechanism)
RegisterPasses.o
Polly.o
  
  opt: // or any ENABLE_PLUGINS tool
main.o
Polly.o // via add_executable(opt Polly.cpp) or target_sources

Adding the object file explicitly to the add_executable will add it 
unconditionally, even if no none of its symbol is required, like for 
LLVMPolly.so.

In the latest update you changed the location of the static initalizer to 
`RegisterPasses.o`, which contains the symbol `llvmGetPassPluginInfo` which is 
pulled-in by the new pass manager plugin system. This makes an unfortunate 
dependence of the static initializer on the `llvmGetPassPluginInfo` mechanism 
(which I think only works for `opt` in the current patch).

There is a reason why pass loading in Polly is organized as it is.




Comment at: llvm/cmake/modules/AddLLVM.cmake:812
+#   llvm::PassPluginLibraryInfo ${entry_point}();
+add_custom_target(LLVM_PLUGINS)  # target used to hold global properties 
referencable from generator-expression
+function(register_llvm_extension llvm_extension entry_point)

beanz wrote:
> Change this to `llvm-plugins` to match our convention and wrap it in `if (NOT 
> TARGET...)` so it doesn't error if AddLLVM is included twice.
[serious] I get multiple of these errors by cmake:
```
CMake Error at cmake/modules/AddLLVM.cmake:812 (add_custom_target):
  add_custom_target cannot create target "LLVM_PLUGINS" because another
  target with the same name already exists.  The existing target is a custom
  target created in source directory "/home/meinersbur/src/llvm".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  projects/compiler-rt/unittests/CMakeLists.txt:2 (include)
```



Comment at: llvm/tools/opt/NewPMDriver.cpp:292
+#define HANDLE_EXTENSION(Ext)  
\
+  get##Ext##PluginInfo().RegisterPassBuilderCallbacks(PB);
+#include "llvm/Support/Extension.def"

[serious] This will pull-in the symbol `llvmGetPassPluginInfo` from the plugin 
for `opt`, but what about the other tools (bugpoint/clang)?



Comment at: polly/lib/Support/RegisterPasses.cpp:727
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
+llvmGetPassPluginInfo() {
+  return getPassPluginInfo();

[serious] Unfortunately, the new pass manager's plugin system relies on the 
function name to be `llvmGetPassPluginInfo` in each plugin. This works with 
multiple dynamic libraries all declaring the same name using the 
`PassPlugin::Load` mechanism, but linking them all statically will violate the 
one-definition-rule.

IMHO, Polly.cpp would have been a better place for this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D64744: Loop #pragma tail_predicate

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

IMHO, this loops like an option of a particular transformation, not an 
independent pragma. E.g.

  #pragma clang loop vectorize(enable) vectorize_remainder(predicated)

There could be multiple choices for how to execute remainder iterations, e.g. 
instead of an epilogue, the first iterations could be executed in an prologue. 
Or an option where the compiler may assume that the iteration-count is always a 
multiple of the vector width, etc.

Also consider interactions with other transformations: What would happen with 
the `llvm.loop.tailpredicate` metadata after, e.g. loop fusion/distribution? 
How does the user know whether the pragma had an effect?


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

https://reviews.llvm.org/D64744



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

[PATCH] D64744: #pragma clang loop predicate(enable|disable)

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

[serious] Need to update docs.

I'd expect `predicate` to be an option of another transformation. For 
vectorization, by convention the #pragma name would be `vectorize_predicate` 
and the metadata `llvm.loop.vectorize.predicate`. As a standalone, it is not 
clear what it is supposed to do (docs missing) and I am against mixing similar 
semantics of options of different loop transformations.

I am currently trying to update the semantics of loop transformations such that 
a transformation order can be defined. An option without transformation it 
belongs to does not fit well into that. E.g. what happends with 
`llvm.loop.predicate` after 
unrolling/vectorization/distribution/interchange/unroll-and-jam/fusion?

Michael




Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:426
+  MDNode *N = createPredicateMetadata(Attrs, LoopProperties, 
HasUserTransforms);
+  if(N)
+LoopProperties.push_back(N);

[nit] run clang-format over your patch


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

https://reviews.llvm.org/D64744



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

[PATCH] D64744: #pragma clang loop vectorize_predicate(enable|disable)

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I prefer having the documentation change to be in the same patch as the 
functional change. Makes it easier to check whether they match.




Comment at: clang/test/CodeGenCXX/pragma-loop.cpp:163
 
-// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
-// CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
-
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
-// CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
-// CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
-// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
-// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
-
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], 
![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
-// CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_3:.*]]}
-// CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
-// CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
-// CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
-
-// CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], 
![[INTERLEAVE_2:.*]]}
-// CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
-// CHECK: ![[INTERLEAVE_2]] = !{!"llvm.loop.interleave.count", i32 2}
-
-// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_1:.*]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
-
-// CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[WIDTH_2:.*]], 
![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_6:.*]]}
-// CHECK: ![[FOLLOWUP_VECTOR_6]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_6:.*]]}
-// CHECK: ![[AFTER_VECTOR_6]] = distinct !{![[AFTER_VECTOR_6]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
-
-// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[WIDTH_5:.*]]}
-// CHECK: ![[WIDTH_5]] = !{!"llvm.loop.vectorize.width", i32 5}
-
-// CHECK: ![[LOOP_8]] = distinct !{![[LOOP_8]], ![[WIDTH_5:.*]]}
-
-// CHECK: ![[LOOP_9]] = distinct !{![[LOOP_9]], ![[WIDTH_8:.*]], 
![[INTERLEAVE_8:.*]], ![[FOLLOWUP_VECTOR_9:.*]]}
-// CHECK: ![[FOLLOWUP_VECTOR_9]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_9:.*]]}
-// CHECK: ![[AFTER_VECTOR_9]] = distinct !{![[AFTER_VECTOR_9]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
-
-// CHECK: ![[LOOP_10]] = distinct !{![[LOOP_10]], ![[WIDTH_2:.*]], 
![[INTERLEAVE_2:.*]], ![[FOLLOWUP_VECTOR_10:.*]]}
-// CHECK: ![[FOLLOWUP_VECTOR_10]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_10:.*]]}
-// CHECK: ![[AFTER_VECTOR_10]] = distinct !{![[AFTER_VECTOR_10]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
-
-// CHECK: ![[LOOP_11]] = distinct !{![[LOOP_11]], ![[WIDTH_2:.*]], 
![[INTERLEAVE_4:.*]], ![[FOLLOWUP_VECTOR_11:.*]]}
-// CHECK: ![[FOLLOWUP_VECTOR_11]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_11:.*]]}
-// CHECK: ![[AFTER_VECTOR_11]] = distinct !{![[AFTER_VECTOR_11]], 
![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
-
-// CHECK: ![[LOOP_12]] = distinct !{![[LOOP_12]], ![[WIDTH_6:.*]], 
![[INTERLEAVE_10:.*]], ![[FOLLOWUP_VECTOR_12:.*]]}
-// CHECK: ![[FOLLOWUP_VECTOR_12]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_12:.*]]}
-// CHECK: ![[AFTER_VECTOR_12]] = distinct !{![[AFTER_VECTOR_12]], 
![[ISVECTORIZED:.*]], ![[UNROLL_24:.*]]}
-// CHECK: ![[UNROLL_24]] = !{!"llvm.loop.unroll.count", i32 24}
-
-// CHECK: ![[LOOP_13]] = distinct !{![[LOOP_13]], ![[WIDTH_8:.*]], 
![[INTERLEAVE_16:.*]], ![[FOLLOWUP_VECTOR_13:.*]]}
-// CHECK: ![[INTERLEAVE_16]] = !{!"llvm.loop.interleave.count", i32 16}
-// CHECK: ![[FOLLOWUP_VECTOR_13]] = !{!"llvm.loop.vectorize.followup_all", 
![[AFTER_VECTOR_13:.*]]}
-// CHECK: ![[AFTER_VECTOR_13]] = distinct !{![[AFTER_VECTOR_13]], 
![[ISVECTORIZED:.*]], ![[UNROLL_32:.*]]}
-// CHECK: ![[UNROLL_32]] = !{!"llvm.loop.unroll.count", i32 32}
-
-// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], ![[WIDTH_10:.*]]}
-// CHECK: ![[WIDTH_10]] = !{!"llvm.loop.vectorize.width", i32 10}
+// CHECK: !2 = distinct !{!2, !3}
+// CHECK: !3 = !{!"llvm.loop.unroll.full"}

I spent quite some time in the past to ensure that the MDNode matchers use 
regexes instead of literals. I'd suggest you create a new test file for 
`vectorize_predicate`, then you don't have to change anything here.



Comment at: clang/test/Parser/pragma-loop.cpp:253
+#pragma clang loop vectorize_predicate(enable)
+/* expected-error {{duplicate directives 'vectorize_predicate(enable)' and 
'vectorize_predicate(disable)'}} */ #pragma clang loop 
vectorize_predicate(disable)
 #pragma clang loop unroll(full)

Can you make the `#pragma` have its own line. You can use `expected-error@+1` 
to make it match another line.

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

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

Thank you for adding the Bye pass. It is really useful! Is there a specific 
reason to not modify the Hello pass?

---

If I enable both passes statically (`LLVM_BYE_LINK_INTO_TOOLS=ON` and 
`LLVM_POLLY_LINK_INTO_TOOLS=ON`), the following regression tests fail (`ninja 
check-llvm`):

  Failing Tests (8):
  LLVM :: DebugInfo/debugify-each.ll
  LLVM :: Other/new-pm-defaults.ll
  LLVM :: Other/new-pm-thinlto-defaults.ll
  LLVM :: Other/opt-O0-pipeline.ll
  LLVM :: Other/opt-O2-pipeline.ll
  LLVM :: Other/opt-O3-pipeline.ll
  LLVM :: Other/opt-Os-pipeline.ll
  LLVM :: Transforms/LoopVectorize/X86/reg-usage.ll

The pass output such as

  Bye: foo
  Bye: goo
  Bye: bar
  Bye: hoo

seem to interfere with the `CHECK` lines in the test cases.

---

Using the configuration `LLVM_LINK_LLVM_DYLIB=1` and 
`LLVM_POLLY_LINK_INTO_TOOLS=ON`, the following tests fail:

  Failing Tests (10):
  LLVM :: BugPoint/compile-custom.ll
  LLVM :: BugPoint/crash-narrowfunctiontest.ll
  LLVM :: BugPoint/func-attrs-keyval.ll
  LLVM :: BugPoint/func-attrs.ll
  LLVM :: BugPoint/invalid-debuginfo.ll
  LLVM :: BugPoint/metadata.ll
  LLVM :: BugPoint/named-md.ll
  LLVM :: BugPoint/remove_arguments_test.ll
  LLVM :: BugPoint/replace-funcs-with-null.ll
  LLVM :: BugPoint/unsymbolized.ll

The error output is:

  : CommandLine Error: Option 'disable-basicaa' registered more than once!
  LLVM ERROR: inconsistency in registered CommandLine options"  




---

As expected, on Windows, I get the following linker error with both 
`LLVM_BYE_LINK_INTO_TOOLS=ON` and `LLVM_POLLY_LINK_INTO_TOOLS=ON`:

  [1/1] Linking CXX executable bin\clang.exe
  FAILED: bin/clang.exe
  cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe 
--intdir=tools\clang\tools\driver\CMakeFiles\clang.dir 
--rc=C:\PROGRA~2\WI3CF2~1\10\bin\100177~1.0\x64\rc.exe 
--mt=C:\PROGRA~2\WI3CF2~1\10\bin\100177~1.0\x64\mt.exe --manifests  -- 
C:\PROGRA~2\MICROS~1\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\link.exe
 /nologo @CMakeFiles\clang.rsp  /out:bin\clang.exe /implib:lib\clang.lib 
/pdb:bin\clang.pdb /version:0.0  /machine:x64 /STACK:1000 /INCREMENTAL:NO 
/subsystem:console  && cmd.exe /C "cd /D 
C:\Users\meinersbur\build\llvm\release\tools\clang\tools\driver && "C:\Program 
Files\CMake\bin\cmake.exe" -E copy 
C:/Users/meinersbur/build/llvm/release/bin/clang.exe 
C:/Users/meinersbur/build/llvm/release/./bin/clang++.exe && cd /D 
C:\Users\meinersbur\build\llvm\release\tools\clang\tools\driver && "C:\Program 
Files\CMake\bin\cmake.exe" -E copy 
C:/Users/meinersbur/build/llvm/release/bin/clang.exe 
C:/Users/meinersbur/build/llvm/release/./bin/clang-cl.exe && cd /D 
C:\Users\meinersbur\build\llvm\release\tools\clang\tools\driver && "C:\Program 
Files\CMake\bin\cmake.exe" -E copy 
C:/Users/meinersbur/build/llvm/release/bin/clang.exe 
C:/Users/meinersbur/build/llvm/release/./bin/clang-cpp.exe""
  LINK: command 
"C:\PROGRA~2\MICROS~1\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\link.exe
 /nologo @CMakeFiles\clang.rsp /out:bin\clang.exe /implib:lib\clang.lib 
/pdb:bin\clang.pdb /version:0.0 /machine:x64 /STACK:1000 /INCREMENTAL:NO 
/subsystem:console /MANIFEST /MANIFESTFILE:bin\clang.exe.manifest" failed (exit 
code 1169) with the following output:
  Bye.lib(Bye.cpp.obj) : error LNK2005: llvmGetPassPluginInfo already defined 
in Polly.lib(RegisterPasses.cpp.obj)
  bin\clang.exe : fatal error LNK1169: one or more multiply defined symbols 
found
  ninja: build stopped: subcommand failed.




Comment at: llvm/cmake/modules/AddLLVM.cmake:856-858
+  #set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY SOURCES 
$)
+  #set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY 
LINK_LIBRARIES $)
+  #set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY 
INTERFACE_LINK_LIBRARIES 
$)

[nit] Please remove commented code entirely



Comment at: llvm/examples/Bye/Bye.cpp:14
+bool runBye(Function &F) {
+  errs() << "Bye: ";
+  errs().write_escaped(F.getName()) << '\n';

Could you add a test that verifies that the `Bye` test has been loaded and is 
working?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D64564: Loop pragma parsing. NFC.

[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


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

https://reviews.llvm.org/D64564



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

[PATCH] D64564: Loop pragma parsing. NFC.

[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: lib/Parse/ParsePragma.cpp:1011
+  Str = llvm::StringSwitch(Str)
+   .Case("loop", "clang loop " + Str.str())
+   .Case("unroll_and_jam", Str)

[serious] I know I already accepted the patch, but I just noticed something:
`"clang loop " + Str.str()` will allocate a temporary std::string, `Str` will 
potentially point to it, then the temporary string will be released. `Str` will 
then point to released memory and returned by this function, i.e. a 
use-after-free.



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

https://reviews.llvm.org/D64564



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Did you resolve the conflicting `llvmGetPassPluginInfo` symbols for windows?


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

https://reviews.llvm.org/D61446



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367773: [OpenMP 5.0] Codegen support for user-defined 
mappers. (authored by Meinersbur, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59474?vs=212399&id=213223#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59474

Files:
  cfe/trunk/include/clang/AST/GlobalDecl.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
  cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
  cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
  cfe/trunk/test/OpenMP/declare_mapper_codegen.cpp

Index: cfe/trunk/include/clang/AST/GlobalDecl.h
===
--- cfe/trunk/include/clang/AST/GlobalDecl.h
+++ cfe/trunk/include/clang/AST/GlobalDecl.h
@@ -59,6 +59,7 @@
   GlobalDecl(const CapturedDecl *D) { Init(D); }
   GlobalDecl(const ObjCMethodDecl *D) { Init(D); }
   GlobalDecl(const OMPDeclareReductionDecl *D) { Init(D); }
+  GlobalDecl(const OMPDeclareMapperDecl *D) { Init(D); }
   GlobalDecl(const CXXConstructorDecl *D, CXXCtorType Type) : Value(D, Type) {}
   GlobalDecl(const CXXDestructorDecl *D, CXXDtorType Type) : Value(D, Type) {}
   GlobalDecl(const VarDecl *D, DynamicInitKind StubKind)
Index: cfe/trunk/test/OpenMP/declare_mapper_codegen.cpp
===
--- cfe/trunk/test/OpenMP/declare_mapper_codegen.cpp
+++ cfe/trunk/test/OpenMP/declare_mapper_codegen.cpp
@@ -1,92 +1,414 @@
-///==///
-// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap %s
-// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap %s
-
-// RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-// RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=i386-pc-linux-gnu -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-
 // SIMD-ONLY0-NOT: {{__kmpc|__tgt}}
 
 // expected-no-diagnostics
 #ifndef HEADER
 #define HEADER
 
+///==///
+// RUN: %clang_cc1 -DCK0 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm -femit-all-decls -disable-llvm-passes %s -o - | FileCheck --check-prefix CK0 --check-prefix CK0-64 %s
+// RUN: %clang_cc1 -DCK0 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -femit-all-decls -disable-llvm-passes -o %t %s
+// RUN: %clang_cc1 -DCK0 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -femit-all-decls -disable-llvm-passes -include-pch %t -verify %s -emit-llvm -o - | FileCheck --check-prefix CK0 --check-prefix CK0-64 %s
+// RUN: %clang_cc1 -DCK0 -verify -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-un

[PATCH] D65776: [Clang] Pragma vectorize_predicate implies vectorize

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Mmmh, I would have expected this to work the same way as `vectorize_width`. 
According to the docs:

> The following example implicitly enables vectorization and interleaving by 
> specifying a vector width and interleaving count:
>  `#pragma clang loop vectorize_width(2) interleave_count(2)`
>  `for(...) {`
>  ` ...`
>  `}`

However, `vectorize_width` does not automatically set 
`llvm.loop.vectorize.enable`. Neither does `llvm.loop.vectorize.width` > 1 
imply `LoopVectorizeHints::getForce()`. At some places they are checked 
together, such as `LoopVectorizeHints::allowReordering()`. Other places, 
notably `LoopVectorizationCostModel::selectVectorizationFactor()`, only queries 
`getForce()`. That is, `vectorize_width(2)` does not implicitly force 
vectorization in practice.


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

https://reviews.llvm.org/D65776



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

[PATCH] D65776: [Clang] Pragma vectorize_predicate implies vectorize

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Looking at the similar situation of `unroll(enable)`/`unroll_count(4)`, 
`unroll_count` also does not set `llvm.loop.unroll.enable`, but it is handled 
by the LoopUnroll pass itself:

  bool ExplicitUnroll = PragmaCount > 0 || PragmaFullUnroll ||
PragmaEnableUnroll || UserUnrollCount;

(LoopUnrollPass.cpp line 770f)

I do not know whether/how "setting a transformation option implicitly enables 
the transformation" should be implemented, maybe we should discuss this. It is 
currently inconsistent. Also consider that non-Clang frontends and .ll files in 
the wild might also expect a specific behavior.


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

https://reviews.llvm.org/D65776



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

[PATCH] D64564: Loop pragma parsing. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



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

[serious] Use-after-free here again. This line will do the following:
```
StringRef ClangLoopStr;
{
std::string tmp = "clang loop " + Str.str()
ClangLoopStr = tmp;
// tmp.~string() 
}
// Any use of ClangLoopStr will use memory released by tmp.~string()
```

Let me suggest a solution:
```
std::string ClangLoopStr = (Twine("clang loop ") + Str).str();
std::string Result = llvm::StringSwitch(Str)
   .Case("loop", ClangLoopStr)
   .Case("unroll_and_jam", Str)
   .Case("unroll", Str)
   .Default("");
return Result; // NRVO, ClangLoopStr will be released here, but if it was 
chosen by the StringSwitch, Result will hold a copy, so ClangLoopStr is not 
referenced anymore.
```

Note that this will alloc one more std::string in the non-ClangLoopStr cases 
than before the patch, but I don't think it's important.


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

https://reviews.llvm.org/D64564



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

[PATCH] D66199: [docs] loop pragmas

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Since this is user documentation, we should only add it here once it is true.




Comment at: docs/LanguageExtensions.rst:3068-3069
 
+There are loop hints that control transformations (e.g. vectorization, loop
+unrolling) and there loop hints that set transformation options (e.g.
+``vectorize_width``, ``unroll_count``).  Pragmas setting transformation options

`vectorize_width` also "controls" a transformation. Is it that in our 
interpretation, `vectorize(enable)` overrides the profitability heuristic by 
taking out the "do not apply at all" option without setting any other option?


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

https://reviews.llvm.org/D66199



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

[PATCH] D66199: [docs] loop pragmas

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: docs/LanguageExtensions.rst:3068-3069
 
+There are loop hints that control transformations (e.g. vectorization, loop
+unrolling) and there loop hints that set transformation options (e.g.
+``vectorize_width``, ``unroll_count``).  Pragmas setting transformation options

SjoerdMeijer wrote:
> Meinersbur wrote:
> > `vectorize_width` also "controls" a transformation. Is it that in our 
> > interpretation, `vectorize(enable)` overrides the profitability heuristic 
> > by taking out the "do not apply at all" option without setting any other 
> > option?
> I think I need to think a little bit more about this:
> 
> > Is it that in our interpretation, vectorize(enable) overrides the 
> > profitability heuristic by taking out the "do not apply at all" option 
> > without setting any other option?
> 
> but I would answer this with "yes". But just checking, what exactly do you 
> mean with "do not apply at all option"?
I unfortunately mixed two meanings of "option" here:
1. option as in optimization setting
2. option as in candidate to choose from.

Let's take a look on the loop vectorizer. The profitability heuristic's job is 
too choose the "best" candidate out of the following list (independently of how 
LoopVectorize's heuristic is actually implemented; these might be decision made 
independently of each other, but as the user still sees one of the following 
outcomes):

0) no vectorization (the "do not apply at all" option/candidate)
1a) vec width=2 with epilogue
1b) vec width=2 without epilogue
2a) vec width=4 with epilogue
2b) vec width=4 without epilogue
3a) vec width=8 with epilogue
3b) vec width=8 without epilogue
...

`vectorize(enable)` with remove 0) as a candidate. `vecorize(disable)` will 
remove all expect 0) as a candidate. 

The option/settings are `vectorize_width` and `vectorize_predicate` (and 
`interleave_count`). For instance `vectorize_width(4)` would remove 1) and 3)+ 
from the candidate list. `vectorize_predicate(enable)` takes any a) candidate 
from the list. What we were discussing was that these settings would remove 0) 
from the candidate list as well.


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

https://reviews.llvm.org/D66199



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

[PATCH] D64564: Loop pragma parsing. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added inline comments.
This revision is now accepted and ready to land.



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

[nit] I am surprised it works without `llvm::` qualifier for `llvm::Twine`. 
Maybe add it for consistency?



Comment at: clang/lib/Parse/ParsePragma.cpp:1016
+ .Default("");
+  return Result;
 }

[nit] One could just `return llvm::StringSwitch...` without a `Result` 
variable. I used it in the suggestion so I could comment on what happens when 
returning. Personally, there is nothing that I could care less about, so use 
what you prefer.


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

https://reviews.llvm.org/D64564



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:808
 
+if(NOT llvm-pass-plugins)
+# Target used to hold global properties referencable from 
generator-expression

[serious] I get the following error:
```
CMake Error at cmake/modules/AddLLVM.cmake:813 (add_custom_target):
  add_custom_target cannot create target "llvm-pass-plugins" because another
  target with the same name already exists.  The existing target is a custom
  target created in source directory "/home/meinersbur/src/llvm".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  projects/compiler-rt/test/CMakeLists.txt:2 (include)
```

What you meant to use is
```
if (NOT TARGET llvm-pass-plugins)
```
See https://cmake.org/cmake/help/latest/command/if.html



Comment at: llvm/cmake/modules/AddLLVM.cmake:851
+"extern \"C\" ::llvm::PassPluginLibraryInfo 
LLVM_ATTRIBUTE_WEAK llvmGetPassPluginInfo() { return ${entry_point}(); }")
+  target_sources(${register_llvm_pass_plugin_EXTRA_LOADABLE} PRIVATE 
${CMAKE_CURRENT_BINARY_DIR}/${register_llvm_pass_plugin_EXTRA_LOADABLE}_plugin_entry_point.cpp)
+endif()

[serious] Under Windows, I get the following error:
```
CMake Error at cmake/modules/AddLLVM.cmake:853 (target_sources):
  target_sources called with non-compilable target type
Call Stack (most recent call first):
  tools/polly/lib/CMakeLists.txt:164 (register_llvm_pass_plugin)
```

This is because "LLVMPolly" is not a library, but a dummy "add_custom_target" 
since loadable modules are not supported under Windows.



Comment at: llvm/docs/WritingAnLLVMPass.rst:1222
 
+Building out-of-tree passes
+===

"out-of-tree" is the wrong term. This registration only works if the plugin if 
configured in the same cmake-run. "out-of-tree" would describe a processes with 
a separate cmake source- and build-tree using `LLVM_MAIN_SRC_DIR` or 
https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project



Comment at: llvm/tools/CMakeLists.txt:45
 
+add_llvm_external_project(polly)
+

What is the reason to have this after `add_llvm_implicit_projects` in contrast 
to the other LLVM subprojects?



Comment at: polly/lib/Support/RegisterPasses.cpp:727
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
+llvmGetPassPluginInfo() {
+  return getPassPluginInfo();

serge-sans-paille wrote:
> Meinersbur wrote:
> > [serious] Unfortunately, the new pass manager's plugin system relies on the 
> > function name to be `llvmGetPassPluginInfo` in each plugin. This works with 
> > multiple dynamic libraries all declaring the same name using the 
> > `PassPlugin::Load` mechanism, but linking them all statically will violate 
> > the one-definition-rule.
> > 
> > IMHO, Polly.cpp would have been a better place for this function.
> > but linking them all statically will violate the one-definition-rule.
> 
> They are unused when liked statically, and flagged as weak to avoid link-time 
> conflict.
> 
> > IMHO, Polly.cpp would have been a better place for this function.
> I still agree it's more explicit if linked conditionaly.
You seem to have removed the weak attribute. Did you mean to put it into the 
`polly` namespace to avoid name clashing with other plugins?


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

https://reviews.llvm.org/D61446



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

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

Windows seems to work. Good job!

Linux works with static libraries, but not with `BUILD_SHARED_LIBS=ON`:

  $ bin/opt
  : CommandLine Error: Option 'polly-dependences-computeout' registered more 
than once!
  LLVM ERROR: inconsistency in registered CommandLine options

This error is typical for having translation units (in this case: Polly's 
`DependenceInfo.cpp`) multiple times in the address space.

See https://groups.google.com/forum/#!topic/polly-dev/vxumPMhrSEs for the 
configurations I intend to test and which currently work.




Comment at: llvm/cmake/modules/AddLLVM.cmake:815
+function(add_llvm_pass_plugin name)
+cmake_parse_arguments(ARG "" "" "" ${ARGN})
+

Why use `cmake_parse_arguments` if there are no options to parse?



Comment at: llvm/cmake/modules/AddLLVM.cmake:825
+set_property(GLOBAL APPEND PROPERTY LLVM_COMPILE_EXTENSIONS ${name})
+get_property(llvm_plugin_targets GLOBAL PROPERTY LLVM_PLUGIN_TARGETS)
+foreach(llvm_plugin_target ${llvm_plugin_targets})

[concern] This requires that all plugin-able tool have been registered before. 
It might be confusing that not all plugins will be loaded into every tool if 
the relative order is not  .

Is it possible to error-out if a ENABLE_PLUGINS happens after a plugin has been 
added via add_llvm_pass_plugin?



Comment at: llvm/cmake/modules/AddLLVM.cmake:827
+foreach(llvm_plugin_target ${llvm_plugin_targets})
+  set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY SOURCES 
$)
+  set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY LINK_LIBRARIES 
${name})

This injects all plugin sources directly into tool executable (in addition to 
loading them as a library with `LINK_LIBRARIES`), probably the reason for the 
error I see with `BUILD_SHARED_LIBS`.

It ignores the library separation, which is not a nice solution for the same 
reasons why LLVM does not simply add all object files from all its libraries to 
each of the add_llvm_executables, but instead uses `target_link_libraries`.



Comment at: llvm/cmake/modules/LLVMProcessSources.cmake:112
   endif()
+  message(STATUS "${listed} gp:${gp}")
   message(SEND_ERROR "Found unknown source file ${fn_relative}

[nit] unrelated?



Comment at: llvm/docs/WritingAnLLVMPass.rst:1240
+
+Out-of-tree passes are compiled and link statically by default, but it's
+possible to set the following variables to change this behavior:

"Out-of-tree" still mentioned here.



Comment at: polly/lib/Support/RegisterPasses.cpp:727
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
+llvmGetPassPluginInfo() {
+  return getPassPluginInfo();

serge-sans-paille wrote:
> Meinersbur wrote:
> > serge-sans-paille wrote:
> > > Meinersbur wrote:
> > > > [serious] Unfortunately, the new pass manager's plugin system relies on 
> > > > the function name to be `llvmGetPassPluginInfo` in each plugin. This 
> > > > works with multiple dynamic libraries all declaring the same name using 
> > > > the `PassPlugin::Load` mechanism, but linking them all statically will 
> > > > violate the one-definition-rule.
> > > > 
> > > > IMHO, Polly.cpp would have been a better place for this function.
> > > > but linking them all statically will violate the one-definition-rule.
> > > 
> > > They are unused when liked statically, and flagged as weak to avoid 
> > > link-time conflict.
> > > 
> > > > IMHO, Polly.cpp would have been a better place for this function.
> > > I still agree it's more explicit if linked conditionaly.
> > You seem to have removed the weak attribute. Did you mean to put it into 
> > the `polly` namespace to avoid name clashing with other plugins?
> There are two entry points: `llvmGetPassPluginInfo`  for new PM (marked as 
> weak) and `get##name##PluginInfo` for legacy PM (name is supposed to be 
> unique, no need for weak linkage)
Unfortunately, the Windows platform has no concept of weak symbols.

`get##name##PluginInfo` is not related to the legacy pass manager. The legacy 
passe manager uses `llvm::PassRegistry` and `llvm::RegisterStandardPasses`. 
`polly::RegisterPollyPasses` is only used by the new pass manager.

Could you create a second plugin using `add_llvm_pass_plugin`, for instance 
convert `LLVMHello`? Then we could check whether this works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D64564: Loop pragma parsing. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/Parse/ParsePragma.cpp:1009
 static std::string PragmaLoopHintString(Token PragmaName, Token Option) {
-  std::string PragmaString;
-  if (PragmaName.getIdentifierInfo()->getName() == "loop") {
-PragmaString = "clang loop ";
-PragmaString += Option.getIdentifierInfo()->getName();
-  } else if (PragmaName.getIdentifierInfo()->getName() == "unroll_and_jam") {
-PragmaString = "unroll_and_jam";
-  } else {
-assert(PragmaName.getIdentifierInfo()->getName() == "unroll" &&
-   "Unexpected pragma name");
-PragmaString = "unroll";
-  }
-  return PragmaString;
+  std::string Str = PragmaName.getIdentifierInfo()->getName();
+  Str = llvm::StringSwitch(Str)

`getName()` returns `StringRef`. No need to use a `std::string` yet.



Comment at: clang/lib/Parse/ParsePragma.cpp:1011-1014
+   .Case("loop", std::string("clang loop ") + Str)
+   .Case("unroll_and_jam", Str)
+   .Case("unroll", Str)
+   .Default("");

This unconditionally creates (at least) 5 `std::string` objects.



Comment at: clang/lib/Parse/ParsePragma.cpp:1040
   // without an argument.
-  bool PragmaUnroll = PragmaNameInfo->getName() == "unroll";
-  bool PragmaNoUnroll = PragmaNameInfo->getName() == "nounroll";
-  bool PragmaUnrollAndJam = PragmaNameInfo->getName() == "unroll_and_jam";
-  bool PragmaNoUnrollAndJam = PragmaNameInfo->getName() == "nounroll_and_jam";
-  if (Toks.empty() && (PragmaUnroll || PragmaNoUnroll || PragmaUnrollAndJam ||
-   PragmaNoUnrollAndJam)) {
+  const bool LoopHint = llvm::StringSwitch(PragmaNameInfo->getName())
+.Cases("unroll", "nounroll", "unroll_and_jam",

[style] We don't use `const` for local variables. Could also use `auto` here 
since the type is already explicit on the RHS.

[suggestion] `IsLoopHint` would indicate the meaning of the boolean variable.


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

https://reviews.llvm.org/D64564



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

[PATCH] D61643: [PragmaHandler] Expose `#pragma` location

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

+1

Such a solution also came up in https://bugs.llvm.org/show_bug.cgi?id=41514#c1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61643



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

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

1. Is there a diagnostic that would point to the `omp` token? As much as I like 
complete info (such as SourceLoc of semicolons), I cannot think of a use case 
for it.
2. I would like to see all of them all adjusted. There is an immediate 
improvement in that improves the diagnostic output.


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

https://reviews.llvm.org/D61509



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

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D61509#1512311 , @jdenny wrote:

> 2. I too think it likely makes sense to adjust them all eventually.  But do 
> people think it's important to write patches adjusting all pragmas before 
> pushing the adjustment for any of them?


I am not sure I understand. Do you mean whether you need all patches for each 
pragma to be accepted before you can commit the first? This is not that case. 
IMHO you can even put all of it into a single patch as it should be very 
straightforward. The most work is adapting the tests.


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

https://reviews.llvm.org/D61509



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

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D61509#1512330 , @jdenny wrote:

> In D61509#1512321 , @Meinersbur 
> wrote:
>
> > In D61509#1512311 , @jdenny wrote:
> >
> > > 2. I too think it likely makes sense to adjust them all eventually.  But 
> > > do people think it's important to write patches adjusting all pragmas 
> > > before pushing the adjustment for any of them?
> >
> >
> > I am not sure I understand. Do you mean whether you need all patches for 
> > each pragma to be accepted before you can commit the first? This is not 
> > that case.
>
>
> @lebedev.ri expressed concern that it might not be acceptable to migrate all 
> pragmas in the same way.  That would suggest we must handle them all before 
> committing any.
>
> > IMHO you can even put all of it into a single patch as it should be very 
> > straightforward. The most work is adapting the tests.
>
> I would think different people would want to review different pragmas, so 
> separate patches would be better, but I'm happy to be corrected as I haven't 
> explored who owns what here.


AFICS it is changing ` Tok.setLocation(FirstTok.getLocation());` to ` 
Tok.setLocation(Introducer.Loc);` for most PragmaHandlers that emit an 
annotation token. To be on the safe side, you can create a patch for each 
PragmaHandler individually (otherwise a review may request to spit them up). 
You can commit each accepted patch immediately unless a reviewer mentions that 
you should wait for $event to happen, like @aaron.ballman just did.


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

https://reviews.llvm.org/D61509



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

[PATCH] D49281: [Unroll/UnrollAndJam/Vectorizer/Distribute] Add followup loop attributes.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348944: [Unroll/UnrollAndJam/Vectorizer/Distribute] Add 
followup loop attributes. (authored by Meinersbur, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49281?vs=177820&id=177870#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D49281

Files:
  test/Misc/backend-optimization-failure-nodbg.cpp
  test/Misc/backend-optimization-failure.cpp


Index: test/Misc/backend-optimization-failure.cpp
===
--- test/Misc/backend-optimization-failure.cpp
+++ test/Misc/backend-optimization-failure.cpp
@@ -7,7 +7,7 @@
 void test_switch(int *A, int *B, int Length,int J) {
 #pragma clang loop vectorize(enable) unroll(disable)
   for (int i = 0; i < Length; i++) {
-/* expected-warning@-1 {{loop not vectorized: failed explicitly specified loop 
vectorization}} */ switch (A[i]) {
+/* expected-warning@-1 {{loop not vectorized: the optimizer was unable to 
perform the requested transformation; the transformation might be disabled or 
specified as part of an unsupported transformation ordering}} */ switch (A[i]) {
 case 0:
   B[i] = 1;
   break;
Index: test/Misc/backend-optimization-failure-nodbg.cpp
===
--- test/Misc/backend-optimization-failure-nodbg.cpp
+++ test/Misc/backend-optimization-failure-nodbg.cpp
@@ -4,7 +4,7 @@
 // Test verifies optimization failures generated by the backend are handled
 // correctly by clang. LLVM tests verify all of the failure conditions.
 
-void test_switch(int *A, int *B, int Length, int J) { /* expected-warning 
{{loop not vectorized: failed explicitly specified loop vectorization}} */
+void test_switch(int *A, int *B, int Length, int J) { /* expected-warning 
{{loop not vectorized: the optimizer was unable to perform the requested 
transformation; the transformation might be disabled or specified as part of an 
unsupported transformation ordering}} */
 #pragma clang loop vectorize(enable) unroll(disable)
   for (int i = 0; i < Length; i++) {
 switch (A[i]) {


Index: test/Misc/backend-optimization-failure.cpp
===
--- test/Misc/backend-optimization-failure.cpp
+++ test/Misc/backend-optimization-failure.cpp
@@ -7,7 +7,7 @@
 void test_switch(int *A, int *B, int Length,int J) {
 #pragma clang loop vectorize(enable) unroll(disable)
   for (int i = 0; i < Length; i++) {
-/* expected-warning@-1 {{loop not vectorized: failed explicitly specified loop vectorization}} */ switch (A[i]) {
+/* expected-warning@-1 {{loop not vectorized: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering}} */ switch (A[i]) {
 case 0:
   B[i] = 1;
   break;
Index: test/Misc/backend-optimization-failure-nodbg.cpp
===
--- test/Misc/backend-optimization-failure-nodbg.cpp
+++ test/Misc/backend-optimization-failure-nodbg.cpp
@@ -4,7 +4,7 @@
 // Test verifies optimization failures generated by the backend are handled
 // correctly by clang. LLVM tests verify all of the failure conditions.
 
-void test_switch(int *A, int *B, int Length, int J) { /* expected-warning {{loop not vectorized: failed explicitly specified loop vectorization}} */
+void test_switch(int *A, int *B, int Length, int J) { /* expected-warning {{loop not vectorized: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering}} */
 #pragma clang loop vectorize(enable) unroll(disable)
   for (int i = 0; i < Length; i++) {
 switch (A[i]) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

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

- Fix typo


Repository:
  rC Clang

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

https://reviews.llvm.org/D52117

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/OpenMP/for_codegen.cpp
  test/OpenMP/for_simd_codegen.cpp
  test/OpenMP/loops_explicit_clauses_codegen.cpp
  test/OpenMP/ordered_codegen.cpp
  test/OpenMP/parallel_for_simd_codegen.cpp
  test/OpenMP/schedule_codegen.cpp
  test/OpenMP/simd_codegen.cpp
  test/OpenMP/simd_metadata.c
  test/OpenMP/target_parallel_for_simd_codegen.cpp
  test/OpenMP/target_simd_codegen.cpp
  test/OpenMP/taskloop_simd_codegen.cpp

Index: test/OpenMP/taskloop_simd_codegen.cpp
===
--- test/OpenMP/taskloop_simd_codegen.cpp
+++ test/OpenMP/taskloop_simd_codegen.cpp
@@ -83,17 +83,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP1]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP1]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK2]](
@@ -113,17 +113,17 @@
 // CHECK: [[LB_I32:%.+]] = trunc i64 [[LB_VAL]] to i32
 // CHECK: store i32 [[LB_I32]], i32* [[CNT:%.+]],
 // CHECK: br label
-// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2:!.+]]
+// CHECK: [[VAL:%.+]] = load i32, i32* [[CNT]],{{.*}}!llvm.access.group
 // CHECK: [[VAL_I64:%.+]] = sext i32 [[VAL]] to i64
-// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: [[UB_VAL:%.+]] = load i64, i64* [[UB]],{{.*}}!llvm.access.group
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: store i32 %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: load i32, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
+// CHECK: store i32 %{{.*}}!llvm.access.group
+// CHECK: load i32, i32* %{{.*}}!llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
-// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.mem.parallel_loop_access [[LOOP2]]
-// CHECK: br label %{{.*}}!llvm.loop [[LOOP2]]
+// CHECK: store i32 %{{.+}}, i32* %{{.*}}!llvm.access.group
+// CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
 // CHECK: define internal i32 [[TASK3]](
@@ -142,7 +142,7 @@
 // CHECK: [[LB_VAL:%.+]] = load i64, i64* [[LB]],
 // CHECK: store i64 [[LB_VAL]], i64* [[CNT:%.+]],
 // CHECK: br label
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
@@ -192,14 +192,14 @@
 // CHECK: [[CMP:%.+]] = icmp ule i64 [[VAL_I64]], [[UB_VAL]]
 // CHECK: br i1 [[CMP]], label %{{.+}}, label %{{.+}}
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: store i32 %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: load i32, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: add nsw i32 %{{.+}}, 1
 // CHECK: store i32 %{{.+}}, i32* %
-// CHECK-NOT: !llvm.mem.parallel_loop_access
+// CHECK-NOT: !llvm.access.group
 // CHECK: br label %{{.*}}!llvm.loop
 // CHECK: ret i32 0
 
Index: test/OpenMP/target_simd_codegen.cpp
===
--- test/OpenMP/target_simd_codegen.cpp
+++ test/OpenMP/target_simd_codegen.cpp
@@ -342,7 +342,7 @@
 // CHECK-64:[[AA_CADDR:%

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349823: [CodeGen] Generate llvm.loop.parallel_accesses 
instead of llvm.mem. (authored by Meinersbur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52117?vs=178944&id=179141#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D52117

Files:
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  test/CodeGenCXX/pragma-loop-safety-nested.cpp
  test/CodeGenCXX/pragma-loop-safety-outer.cpp
  test/CodeGenCXX/pragma-loop-safety.cpp
  test/OpenMP/for_codegen.cpp
  test/OpenMP/for_simd_codegen.cpp
  test/OpenMP/loops_explicit_clauses_codegen.cpp
  test/OpenMP/ordered_codegen.cpp
  test/OpenMP/parallel_for_simd_codegen.cpp
  test/OpenMP/schedule_codegen.cpp
  test/OpenMP/simd_codegen.cpp
  test/OpenMP/simd_metadata.c
  test/OpenMP/target_parallel_for_simd_codegen.cpp
  test/OpenMP/target_simd_codegen.cpp
  test/OpenMP/taskloop_simd_codegen.cpp

Index: lib/CodeGen/CGLoopInfo.h
===
--- lib/CodeGen/CGLoopInfo.h
+++ lib/CodeGen/CGLoopInfo.h
@@ -84,6 +84,9 @@
   /// Get the set of attributes active for this loop.
   const LoopAttributes &getAttributes() const { return Attrs; }
 
+  /// Return this loop's access group or nullptr if it does not have one.
+  llvm::MDNode *getAccessGroup() const { return AccGroup; }
+
 private:
   /// Loop ID metadata.
   llvm::MDNode *LoopID;
@@ -91,6 +94,8 @@
   llvm::BasicBlock *Header;
   /// The attributes for this loop.
   LoopAttributes Attrs;
+  /// The access group for memory accesses parallel to this loop.
+  llvm::MDNode *AccGroup = nullptr;
 };
 
 /// A stack of loop information corresponding to loop nesting levels.
Index: lib/CodeGen/CGLoopInfo.cpp
===
--- lib/CodeGen/CGLoopInfo.cpp
+++ lib/CodeGen/CGLoopInfo.cpp
@@ -21,7 +21,7 @@
 
 static MDNode *createMetadata(LLVMContext &Ctx, const LoopAttributes &Attrs,
   const llvm::DebugLoc &StartLoc,
-  const llvm::DebugLoc &EndLoc) {
+  const llvm::DebugLoc &EndLoc, MDNode *&AccGroup) {
 
   if (!Attrs.IsParallel && Attrs.VectorizeWidth == 0 &&
   Attrs.InterleaveCount == 0 && Attrs.UnrollCount == 0 &&
@@ -122,6 +122,12 @@
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 
+  if (Attrs.IsParallel) {
+AccGroup = MDNode::getDistinct(Ctx, {});
+Args.push_back(MDNode::get(
+Ctx, {MDString::get(Ctx, "llvm.loop.parallel_accesses"), AccGroup}));
+  }
+
   // Set the first operand to itself.
   MDNode *LoopID = MDNode::get(Ctx, Args);
   LoopID->replaceOperandWith(0, LoopID);
@@ -150,7 +156,8 @@
 LoopInfo::LoopInfo(BasicBlock *Header, const LoopAttributes &Attrs,
const llvm::DebugLoc &StartLoc, const llvm::DebugLoc &EndLoc)
 : LoopID(nullptr), Header(Header), Attrs(Attrs) {
-  LoopID = createMetadata(Header->getContext(), Attrs, StartLoc, EndLoc);
+  LoopID =
+  createMetadata(Header->getContext(), Attrs, StartLoc, EndLoc, AccGroup);
 }
 
 void LoopInfoStack::push(BasicBlock *Header, const llvm::DebugLoc &StartLoc,
@@ -328,6 +335,21 @@
 }
 
 void LoopInfoStack::InsertHelper(Instruction *I) const {
+  if (I->mayReadOrWriteMemory()) {
+SmallVector AccessGroups;
+for (const LoopInfo &AL : Active) {
+  // Here we assume that every loop that has an access group is parallel.
+  if (MDNode *Group = AL.getAccessGroup())
+AccessGroups.push_back(Group);
+}
+MDNode *UnionMD = nullptr;
+if (AccessGroups.size() == 1)
+  UnionMD = cast(AccessGroups[0]);
+else if (AccessGroups.size() >= 2)
+  UnionMD = MDNode::get(I->getContext(), AccessGroups);
+I->setMetadata("llvm.access.group", UnionMD);
+  }
+
   if (!hasInfo())
 return;
 
@@ -343,18 +365,4 @@
   }
 return;
   }
-
-  if (I->mayReadOrWriteMemory()) {
-SmallVector ParallelLoopIDs;
-for (const LoopInfo &AL : Active)
-  if (AL.getAttributes().IsParallel)
-ParallelLoopIDs.push_back(AL.getLoopID());
-
-MDNode *ParallelMD = nullptr;
-if (ParallelLoopIDs.size() == 1)
-  ParallelMD = cast(ParallelLoopIDs[0]);
-else if (ParallelLoopIDs.size() >= 2)
-  ParallelMD = MDNode::get(I->getContext(), ParallelLoopIDs);
-I->setMetadata("llvm.mem.parallel_loop_access", ParallelMD);
-  }
 }
Index: test/OpenMP/target_simd_codegen.cpp
===
--- test/OpenMP/target_simd_codegen.cpp
+++ test/OpenMP/target_simd_codegen.cpp
@@ -342,7 +342,7 @@
 // CHECK-64:[[AA_CADDR:%.+]] = bitcast i[[SZ]]* [[AA_ADDR]] to i32*
 // CHECK-64:[[AA:%.+]] = load i32, i32* [[AA_CADDR]], align
 // CHECK-32:[[AA:%.+]] = load i32, i32* [[AA_ADDR]], align
-// CHECK:   !llvm

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[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: llvm/test/Feature/load_extension.ll:1
+; RUN: not test -f %llvmshlibdir/libBye%shlibext || opt %s 
-load=%llvmshlibdir/libBye%shlibext -goodbye -wave-goodbye \
+; RUN:   -disable-output 2>&1 | grep Bye

It would be preferable to have a "REQUIRES" line that checks that the Bye pass 
has been linked-in dynamically.

Alternatively, use the lit.cfg to expend to the required `-load=` argument if 
required, so this test checks with and without LLVM_BYE_LOAD_INTO_TOOLS



Comment at: llvm/test/Feature/load_extension.ll:2
+; RUN: not test -f %llvmshlibdir/libBye%shlibext || opt %s 
-load=%llvmshlibdir/libBye%shlibext -goodbye -wave-goodbye \
+; RUN:   -disable-output 2>&1 | grep Bye
+; REQUIRES: plugins

[serious] Use FileCheck



Comment at: llvm/test/lit.cfg.py:193-196
+if config.linked_bye_extension:
+llvm_config.with_environment('LLVM_CHECK_EXT', 'CHECK-EXT')
+else:
+llvm_config.with_environment('LLVM_CHECK_EXT', 'CHECK-NOEXT')

[serious] This kind of shell expansion does not work on Windows:
```
$ "c:\users\meinersbur\build\llvm\release\bin\filecheck.exe" 
"C:\Users\meinersbur\src\llvm\test\Other\new-pm-thinlto-defaults.ll" 
"--check-prefixes=CHECK-O,CHECK-O1,CHECK-POSTLINK-O,${LLVM_CHECK_EXT},CHECK-POSTLINK-O1"
# command stderr:
Supplied check-prefix is invalid! Prefixes must be unique and start with a 
letter and contain only alphanumeric characters, hyphens and underscores

error: command failed with exit status: 2
```

Polly 'solves' this by adding itself to the pass manager only when another 
command line flag is added (`-polly`) which would be less intrusive.



Comment at: llvm/test/lit.site.cfg.py.in:49
 config.has_plugins = @LLVM_ENABLE_PLUGINS@
+config.linked_bye_extension = "@LLVM_BYE_LINK_INTO_TOOLS@".lower() in ("on", 1)
 

See https://cmake.org/cmake/help/latest/command/if.html for which values cmake 
considers true-ish.



Comment at: polly/lib/Support/RegisterPasses.cpp:726
+
+#ifndef LLVM_POLLY_LINK_INTO_TOOLS
+extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo

[serious] `LLVM_POLLY_LINK_INTO_TOOLS` is a cmake configuration parameter, but 
not a preprocessor symbol. Hence, `LLVM_POLLY_LINK_INTO_TOOLS` is never defined.

Error on Windows:
```
Bye.lib(Bye.cpp.obj) : error LNK2005: llvmGetPassPluginInfo already defined in 
Polly.lib(RegisterPasses.cpp.obj)
bin\opt.exe : fatal error LNK1169: one or more multiply defined symbols found
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Keep in mind that for static linking you will need something that pulls-in a 
symbol from the pass static library. In this patch, `NewPMDriver.cpp` does it 
for `opt` by calling `get##Ext##PluginInfo()`. In clang, it is 
`BackendUtil.cpp`. But nothing for `bugpoint` hence it doesn't contain either 
Polly not the Goodbye pass (However, llvm-reduce is in the works, we might 
consider bugpoint deprecated).

Could you add documentation for how to write a tool that uses loadable plugins 
to document the way it should be done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D67978: [OpenMP 5.0] Fix user-defined mapper lookup in sema

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL373023: [OpenMP 5.0] Fix user-defined mapper lookup in sema 
(authored by Meinersbur, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67978?vs=221991&id=222041#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67978

Files:
  cfe/trunk/lib/Sema/SemaOpenMP.cpp
  cfe/trunk/test/OpenMP/declare_mapper_messages.c
  cfe/trunk/test/OpenMP/declare_mapper_messages.cpp

Index: cfe/trunk/test/OpenMP/declare_mapper_messages.cpp
===
--- cfe/trunk/test/OpenMP/declare_mapper_messages.cpp
+++ cfe/trunk/test/OpenMP/declare_mapper_messages.cpp
@@ -64,6 +64,7 @@
 {
 #pragma omp declare mapper(id: vec v) map(v.len)
   vec vv, v1;
+  vec arr[10];
 #pragma omp target map(mapper)  // expected-error {{use of undeclared identifier 'mapper'}}
   {}
 #pragma omp target map(mapper:vv)   // expected-error {{expected '(' after 'mapper'}}
@@ -82,7 +83,9 @@
   {}
 #pragma omp target map(mapper(N1::aa) alloc:vv) // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'aa'}}
   {}
-#pragma omp target map(mapper(aa) to:vv) map(close mapper(aa) from:v1)
+#pragma omp target map(mapper(N1::aa) alloc:arr[0:2])   // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'aa'}}
+  {}
+#pragma omp target map(mapper(aa) to:vv) map(close mapper(aa) from:v1) map(mapper(aa) to:arr[0])
   {}
 #pragma omp target map(mapper(N1::stack::id) to:vv)
   {}
@@ -96,8 +99,9 @@
 #pragma omp target update to(mapper(N1:: :vv)   // expected-error {{illegal OpenMP user-defined mapper identifier}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update to(mapper(N1::aa) :vv)// expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'aa'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update to(mapper(ab):vv) // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'ab'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update to(mapper(ab):arr[0:2])   // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'ab'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update to(mapper(aa) a:vv)   // expected-warning {{missing ':' after ) - ignoring}}
-#pragma omp target update to(mapper(aa):vv)
+#pragma omp target update to(mapper(aa):vv) to(mapper(aa):arr[0])
 #pragma omp target update to(mapper(N1::stack::id) :vv)
 
 #pragma omp target update from(mapper)  // expected-error {{expected '(' after 'mapper'}} expected-error {{expected expression}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
@@ -109,8 +113,9 @@
 #pragma omp target update from(mapper(N1:: :vv) // expected-error {{illegal OpenMP user-defined mapper identifier}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update from(mapper(N1::aa) :vv)  // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'aa'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update from(mapper(ab):vv)   // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'ab'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+#pragma omp target update from(mapper(ab):arr[0:2]) // expected-error {{cannot find a valid user-defined mapper for type 'vec' with name 'ab'}} expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
 #pragma omp target update from(mapper(aa) a:vv) // expected-warning {{missing ':' after ) - ignoring}}
-#pragma omp target update from(mapper(aa):vv)
+#pragma omp target update from(mapper(aa):vv) from(mapper(aa):arr[0])
 #pragma omp target update from(mapper(N1::stack::id) :vv)
 }
 #pragma omp declare mapper(id: vec v) map(v.len)   

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: polly/lib/Support/RegisterPasses.cpp:726
+
+#ifndef LLVM_POLLY_LINK_INTO_TOOLS
+extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo

Meinersbur wrote:
> [serious] `LLVM_POLLY_LINK_INTO_TOOLS` is a cmake configuration parameter, 
> but not a preprocessor symbol. Hence, `LLVM_POLLY_LINK_INTO_TOOLS` is never 
> defined.
> 
> Error on Windows:
> ```
> Bye.lib(Bye.cpp.obj) : error LNK2005: llvmGetPassPluginInfo already defined 
> in Polly.lib(RegisterPasses.cpp.obj)
> bin\opt.exe : fatal error LNK1169: one or more multiply defined symbols found
> ```
Before you try to fix this preprocessor symbol, consider that Polly compiles a 
loadable module (to be used with `-load`) __and__ a library (static or dynamic 
depending on `BUILD_SHARED_LIBS`) __independent__ of 
`LLVM_POLLY_LINK_INTO_TOOLS`. That is, the loadable module must contain 
`llvmGetPassPluginInfo`, but not the library. That is, a preprocessor symbol 
that is the same for both will not work.

PLEASE, just keep the Polly.cpp (and move `llvmGetPassPluginInfo` in there; the 
static initializer indeed has to stay here as it will be used by both), it will 
make things easier as it already has been shown to work already. Do the same 
for Bye. 

If you really insist on removing the `Polly.cpp`, do so in a follow-up patch. 
In that case you will have to rework the CMakeLists.txt to only build one, 
either the loadable module or the library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: test/Sema/attr-micromips.c:9
 
-__attribute__((micromips,mips16)) void foo5();  // expected-error 
{{'micromips' and 'mips16' attributes are not compatible}} \
+__attribute__((micromips,mips16)) void foo5();  // expected-error {{'mips16' 
and 'micromips' attributes are not compatible}} \
 // expected-note {{conflicting 
attribute is here}}

echristo wrote:
> This seems to reverse? What's going on here? There are other occurrences too.
This is caused by the same phenomenon as the 'previous' marker changing to the 
attribute that is textually after the main marker. The order within the same 
message seems less important to me.

For following happens when attributes are merged:
1. If the attributes are of the same declaration, compatibility is checked with 
the existing attributes. With the current order: textually later attributes are 
already in the list of accepted attributes and textually earlier attributes are 
added to it.
2. If the attributes are in two different declarations, the new (textually 
later) `clang::Decl` with its attributes is taken and the attributes of the old 
(textually earlier) clang::Decl are added to it while checking for 
compatibility.

That is, in both cases the textually earlier attribute is added to the existing 
list of textually later attributes. The diagnostics are printed with the 
exiting attribute on the right and the attribute-to-be added on the left (at 
least in this case. `test/Sema/attr-swiftcall.c:12` is a counterexample where 
this patch changes it to the textual order).

Conflicts are resolved by throwing away the conflicting attribute in the list 
(i.e. textually later) to make room for the new (textually earlier) attribute.

This does not seem to have evolved intentionally when picking in which order to 
print to two conflicting attributes. I would prefer if the the AST represents 
the source as accurately as possible, including ordering of attributes (e.g. 
`test/Sema/attr-print.c`), which in some cases carry semantic meaning: 
`EnableIfAttr` and `LoopHintAttr`. Today, when dumping the AST, these are just 
incorrect.

I spent some time trying to reverse the order in which the attributes appear in 
the diagnostic messages (including `previous occurance here`), but this would 
also require changing case 2., which is more difficult.



Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D48100#1142866, @erichkeane wrote:

> I'm currently attempting to remove the AttributeList's linked-listness.


Thank you. This should also resolve the non-optimal asymptotic execution time 
to append new attributes at the end of the list.


Repository:
  rC Clang

https://reviews.llvm.org/D48100



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

[PATCH] D48734: [Sema] Consider all format_arg attributes.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur created this revision.
Meinersbur added reviewers: dlj, rsmith, Jean-Daniel.

If a function has multiple format_arg attributes, clang only considers the 
first it finds (because AttributeLists are in reverse order, it is textual 
last) and ignores all others.

Loop over all FormatArgAttr to print warnings for all declared format_arg 
attributes.

For instance, GNU gettext's ngettext (select plural or singular version of 
format string) has two __format_arg__ attributes.


Repository:
  rC Clang

https://reviews.llvm.org/D48734

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/attr-format_arg.c


Index: test/Sema/attr-format_arg.c
===
--- test/Sema/attr-format_arg.c
+++ test/Sema/attr-format_arg.c
@@ -4,10 +4,15 @@
 
 const char* f(const char *s) __attribute__((format_arg(1)));
 
+const char *h(const char *msg1, const char *msg2)
+__attribute__((__format_arg__(1))) __attribute__((__format_arg__(2)));
+
 void g(const char *s) {
   printf("%d", 123);
   printf("%d %d", 123); // expected-warning{{more '%' conversions than data 
arguments}}
 
   printf(f("%d"), 123);
   printf(f("%d %d"), 123); // expected-warning{{more '%' conversions than data 
arguments}}
+
+  printf(h("", ""), 123); // expected-warning 2{{format string is empty}}
 }
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -5518,13 +5518,22 @@
   case Stmt::CXXMemberCallExprClass: {
 const CallExpr *CE = cast(E);
 if (const NamedDecl *ND = 
dyn_cast_or_null(CE->getCalleeDecl())) {
-  if (const FormatArgAttr *FA = ND->getAttr()) {
+  bool IsFirst = true;
+  StringLiteralCheckType CommonResult;
+  for (const FormatArgAttr *FA : ND->specific_attrs()) {
 const Expr *Arg = CE->getArg(FA->getFormatIdx().getASTIndex());
-return checkFormatStringExpr(S, Arg, Args,
- HasVAListArg, format_idx, firstDataArg,
- Type, CallType, InFunctionCall,
- CheckedVarArgs, UncoveredArg, Offset);
-  } else if (const FunctionDecl *FD = dyn_cast(ND)) {
+StringLiteralCheckType Result = checkFormatStringExpr(
+S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
+if (IsFirst) {
+  CommonResult = Result;
+  IsFirst = false;
+}
+  }
+  if (!IsFirst)
+return CommonResult;
+
+  if (const FunctionDecl *FD = dyn_cast(ND)) {
 unsigned BuiltinID = FD->getBuiltinID();
 if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString ||
 BuiltinID == Builtin::BI__builtin___NSStringMakeConstantString) {


Index: test/Sema/attr-format_arg.c
===
--- test/Sema/attr-format_arg.c
+++ test/Sema/attr-format_arg.c
@@ -4,10 +4,15 @@
 
 const char* f(const char *s) __attribute__((format_arg(1)));
 
+const char *h(const char *msg1, const char *msg2)
+__attribute__((__format_arg__(1))) __attribute__((__format_arg__(2)));
+
 void g(const char *s) {
   printf("%d", 123);
   printf("%d %d", 123); // expected-warning{{more '%' conversions than data arguments}}
 
   printf(f("%d"), 123);
   printf(f("%d %d"), 123); // expected-warning{{more '%' conversions than data arguments}}
+
+  printf(h("", ""), 123); // expected-warning 2{{format string is empty}}
 }
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -5518,13 +5518,22 @@
   case Stmt::CXXMemberCallExprClass: {
 const CallExpr *CE = cast(E);
 if (const NamedDecl *ND = dyn_cast_or_null(CE->getCalleeDecl())) {
-  if (const FormatArgAttr *FA = ND->getAttr()) {
+  bool IsFirst = true;
+  StringLiteralCheckType CommonResult;
+  for (const FormatArgAttr *FA : ND->specific_attrs()) {
 const Expr *Arg = CE->getArg(FA->getFormatIdx().getASTIndex());
-return checkFormatStringExpr(S, Arg, Args,
- HasVAListArg, format_idx, firstDataArg,
- Type, CallType, InFunctionCall,
- CheckedVarArgs, UncoveredArg, Offset);
-  } else if (const FunctionDecl *FD = dyn_cast(ND)) {
+StringLiteralCheckType Result = checkFormatStringExpr(
+S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
+if (IsFirst) {
+  CommonResult = Result;
+  IsFirst = false;
+}
+  }
+  if (!IsFirst)
+return CommonResult;
+
+  if (const FunctionDecl *FD = dyn_cast(ND)) {
 unsigned BuiltinID = FD->