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

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368970: [Clang] Pragma vectorize_predicate implies vectorize 
(authored by SjoerdMeijer, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65776?vs=213462=215330#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65776

Files:
  cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp


Index: cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
@@ -253,12 +253,18 @@
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
   // Setting vectorize.predicate
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
+  bool IsVectorPredicateEnabled = false;
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
+  Attrs.VectorizeEnable != LoopAttributes::Disable &&
+  Attrs.VectorizeWidth < 1) {
+
+IsVectorPredicateEnabled =
+(Attrs.VectorizePredicateEnable == LoopAttributes::Enable);
+
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.predicate.enable"),
-ConstantAsMetadata::get(ConstantInt::get(
-llvm::Type::getInt1Ty(Ctx),
-(Attrs.VectorizePredicateEnable == LoopAttributes::Enable)))};
+ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt1Ty(Ctx),
+ IsVectorPredicateEnabled))};
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 
@@ -281,12 +287,15 @@
   }
 
   // Setting vectorize.enable
-  if (Attrs.VectorizeEnable != LoopAttributes::Unspecified) {
+  if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
+  IsVectorPredicateEnabled) {
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.enable"),
 ConstantAsMetadata::get(ConstantInt::get(
 llvm::Type::getInt1Ty(Ctx),
-(Attrs.VectorizeEnable == LoopAttributes::Enable)))};
+IsVectorPredicateEnabled
+? true
+: (Attrs.VectorizeEnable == LoopAttributes::Enable)))};
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 
Index: cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ cfe/trunk/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -27,9 +27,50 @@
 List[i] = i * 2;
 }
 
+// vectorize_predicate(enable) implies vectorize(enable)
+void test3(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test3{{.*}}(
+// CHECK:   br label {{.*}}, !llvm.loop ![[LOOP3:.*]]
+
+  #pragma clang loop vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that disabling vectorization means a vectorization width of 1, and
+// also that vectorization_predicate isn't enabled.
+void test4(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test4{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP4:.*]]
+
+  #pragma clang loop vectorize(disable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that vectorize and vectorize_predicate are disabled.
+void test5(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test5{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP5:.*]]
+
+  #pragma clang loop vectorize(disable) vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], !3}
 // CHECK-NEXT: !3 = !{!"llvm.loop.vectorize.enable", i1 true}
+
 // CHECK-NEXT: ![[LOOP1]] = distinct !{![[LOOP1]], !5, !3}
 // CHECK-NEXT: !5 = !{!"llvm.loop.vectorize.predicate.enable", i1 true}
+
 // CHECK-NEXT: ![[LOOP2]] = distinct !{![[LOOP2]], !7, !3}
 // CHECK-NEXT: !7 = !{!"llvm.loop.vectorize.predicate.enable", i1 false}
+
+// CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
+
+// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
+// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
+
+// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}


Index: cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
@@ -253,12 +253,18 @@
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
   // Setting vectorize.predicate
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
+  bool IsVectorPredicateEnabled = false;
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
+  Attrs.VectorizeEnable != LoopAttributes::Disable &&
+  Attrs.VectorizeWidth < 1) {
+
+IsVectorPredicateEnabled =
+(Attrs.VectorizePredicateEnable == 

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks for your help! And I will wait a few days.

After this, I will look at that PR, will have a look at diagnostics, and then 
at the LLVM side of things.


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

[Florian Hahn via Phabricator via cfe-commits]

fhahn accepted this revision.
fhahn added reviewers: hfinkel, reames.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! Please wait a few days with committing, in case any additional 
comments come in via the mailing list.


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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

thanks @Meinersbur and @fhahn  for the discussions on this, also on the dev 
list.
It looks like we are happy with this behaviour?


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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> I do not know whether/how "setting a transformation option implicitly enables 
> the transformation" should be implemented, maybe we should discuss this.

Yep, agreed. I've sent a mail to the dev list:
http://lists.llvm.org/pipermail/cfe-dev/2019-August/063054.html


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] D65776: [Clang] Pragma vectorize_predicate implies vectorize

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Hi Florian, thanks for your input!

> IMO it would make sense to have the more specific pragmas imply 
> vectorize(enable) here (or update the docs accordingly).

Yep, fully agree with that, as I also wrote in my previous comment. And thanks 
for digging up that PR. I think that supports this case and that the proposed 
behaviour in this patch is what one would expect. If we agree on that, I 
wouldn't mind fixing that PR too as that seems low hanging-fruit to me.

And to be honest, at the moment I have no clue what the meaning is, or 
could/should be, when a more specific pragma like vector_width or 
vector_predicate is set, without also setting/implying vectorize(enable). I 
need to look into that...


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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D65776#1615834 , @Meinersbur wrote:

> 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.


The current behavior with respect to vectorize_width seems a bit 
counterintuitive. The docs say `vectorize_width implicitly enables 
vectorization`, I would expect it to behave as if `vectorize(enable)` was also 
passed. There's also a PR about that mismatch: 
https://bugs.llvm.org/show_bug.cgi?id=27643

IMO it would make sense to have the more specific pragmas imply 
vectorize(enable) here (or update the docs accordingly).


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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks for pointing this all out!

I am not entirely sure yet what to think about all this as I am new to the loop 
pragma business, but I think it looks inconsistent to me!

I think I find `allowReordering()` a little bit ugly, because it is also 
checking `getWidth() > 1`, and I am probably expecting the allowReordering 
decision to be based on 1 thing (the Force). But in the current implementation 
that probably makes sense though if `vectorizer_width` doesn't set 
`vectorizer.enable` (again, which is not what I would expect). Thus, it makes 
sense to me that `LoopVectorizationCostModel::selectVectorizationFactor()` only 
checks `getForce()`

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

So I don't see at this moment how it could work in the same way. And by working 
in the same way I think you mean adding `vectorize_predicate` checks to 
different places where `vectorize_width` is also checked, but that doesn't 
sound ideal to me.

Could a conclusion be that I was lucky? Lucky, because without all this 
knowledge that I have now, this patch looks to do what we would expect?


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.

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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Forgot that this requires a doc change too. Will add that once we're happy with 
the proposed behaviour.


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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer created this revision.
SjoerdMeijer added reviewers: Meinersbur, hsaito, fhahn.

New pragma "vectorize_predicate(enable)" now implies "vectorize(enable)",
and it is ignored when vectorization is disabled with e.g.
"vectorize(disable) vectorize_predicate(enable)".


https://reviews.llvm.org/D65776

Files:
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/test/CodeGenCXX/pragma-loop-predicate.cpp


Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -27,9 +27,50 @@
 List[i] = i * 2;
 }
 
+// vectorize_predicate(enable) implies vectorize(enable)
+void test3(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test3{{.*}}(
+// CHECK:   br label {{.*}}, !llvm.loop ![[LOOP3:.*]]
+
+  #pragma clang loop vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that disabling vectorization means a vectorization width of 1, and
+// also that vectorization_predicate isn't enabled.
+void test4(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test4{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP4:.*]]
+
+  #pragma clang loop vectorize(disable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that vectorize and vectorize_predicate are disabled.
+void test5(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test5{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP5:.*]]
+
+  #pragma clang loop vectorize(disable) vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+
 // CHECK:  ![[LOOP0]] = distinct !{![[LOOP0]], !3}
 // CHECK-NEXT: !3 = !{!"llvm.loop.vectorize.enable", i1 true}
+
 // CHECK-NEXT: ![[LOOP1]] = distinct !{![[LOOP1]], !5, !3}
 // CHECK-NEXT: !5 = !{!"llvm.loop.vectorize.predicate.enable", i1 true}
+
 // CHECK-NEXT: ![[LOOP2]] = distinct !{![[LOOP2]], !7, !3}
 // CHECK-NEXT: !7 = !{!"llvm.loop.vectorize.predicate.enable", i1 false}
+
+// CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
+
+// CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
+// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
+
+// CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}
Index: clang/lib/CodeGen/CGLoopInfo.cpp
===
--- clang/lib/CodeGen/CGLoopInfo.cpp
+++ clang/lib/CodeGen/CGLoopInfo.cpp
@@ -253,12 +253,18 @@
   Args.append(LoopProperties.begin(), LoopProperties.end());
 
   // Setting vectorize.predicate
-  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified) {
+  bool IsVectorPredicateEnabled = false;
+  if (Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified &&
+  Attrs.VectorizeEnable != LoopAttributes::Disable &&
+  Attrs.VectorizeWidth < 1) {
+
+IsVectorPredicateEnabled =
+(Attrs.VectorizePredicateEnable == LoopAttributes::Enable);
+
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.predicate.enable"),
-ConstantAsMetadata::get(ConstantInt::get(
-llvm::Type::getInt1Ty(Ctx),
-(Attrs.VectorizePredicateEnable == LoopAttributes::Enable)))};
+ConstantAsMetadata::get(ConstantInt::get(llvm::Type::getInt1Ty(Ctx),
+ IsVectorPredicateEnabled))};
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 
@@ -281,12 +287,15 @@
   }
 
   // Setting vectorize.enable
-  if (Attrs.VectorizeEnable != LoopAttributes::Unspecified) {
+  if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
+  IsVectorPredicateEnabled) {
 Metadata *Vals[] = {
 MDString::get(Ctx, "llvm.loop.vectorize.enable"),
 ConstantAsMetadata::get(ConstantInt::get(
 llvm::Type::getInt1Ty(Ctx),
-(Attrs.VectorizeEnable == LoopAttributes::Enable)))};
+IsVectorPredicateEnabled
+? true
+: (Attrs.VectorizeEnable == LoopAttributes::Enable)))};
 Args.push_back(MDNode::get(Ctx, Vals));
   }
 


Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -27,9 +27,50 @@
 List[i] = i * 2;
 }
 
+// vectorize_predicate(enable) implies vectorize(enable)
+void test3(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test3{{.*}}(
+// CHECK:   br label {{.*}}, !llvm.loop ![[LOOP3:.*]]
+
+  #pragma clang loop vectorize_predicate(enable)
+  for (int i = 0; i < Length; i++)
+List[i] = i * 2;
+}
+
+// Check that disabling vectorization means a vectorization width of 1, and
+// also that vectorization_predicate isn't enabled.
+void test4(int *List, int Length) {
+// CHECK-LABEL: @{{.*}}test4{{.*}}(
+// CHECK: br label {{.*}}, !llvm.loop ![[LOOP4:.*]]
+
+  #pragma clang loop vectorize(disable)
+