[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer abandoned this revision.
SjoerdMeijer added a comment.

Every day is a school day, and Hal taught me something ;-)

As I wrote in the thread on the llvm dev list, with Hal's explanations, I don't 
think I have a case for this anymore, and so will abandon it (please let me 
know if you disagree).

Many thanks for reviewing and the discussions!


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> Now on the practical side. If whatever we decide here changes how the pragma 
> behaves from today, we need to have a wider discussion and agreement at 
> llvm-dev.

Yep, forgot about that, thanks for the suggestion. I've just posted this 
message to the list:

http://lists.llvm.org/pipermail/llvm-dev/2019-September/135016.html


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Hideki Saito via Phabricator via cfe-commits]

hsaito added a comment.

In D66796#1663868 , @SjoerdMeijer 
wrote:

> That's exactly the reason why I think `vectorize(disable)` should disable 
> vectorisation for that loop. I just don't see what else a user would expect.


I agree with you.

Now on the practical side. If whatever we decide here changes how the pragma 
behaves from today, we need to have a wider discussion and agreement at 
llvm-dev.


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

And thanks Florian for getting this discussion going again! Will definitely 
make this clear(er) in this description and commit message once we agree on it.


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Hi Hideki,

I think you're comments are spot on:

> It all depends on to whom we are providing these pragmas.

Pragma's are user-facing "options" to override or force  compiler decision 
making. I don't think there's another way to look at it, but please correct me 
if I'm wrong.

That's exactly the reason why I think `vectorize(disable)` should disable 
vectorisation for that loop. I just don't see what else a user would expect.


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Hideki Saito via Phabricator via cfe-commits]

hsaito added a comment.

There are two ways to think.

1. vectorize(disable)   as in disable the LoopVectorize pass itself.
2. vectorize(disable)   as in disabling the loop vectorization transformation

It all depends on to whom we are providing these pragmas.

If we are providing pragmas for programmers, they don't care LoopVectorize or 
something else. They care about vectorization of loop. So, 2) makes more sense.

If we are providing pragmas for compiler writers, we should be making this more 
like

  #pragma clang loop LoopVectorize(disable)

so that what is controlling what crisply known to everyone.

For anything that is programmer visible, I think it's time for LLVM vectorizer 
to focus on how the programmers use rather than how vectorizer writers think 
about things.


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

IMO it is fine to say ` pragma vectorize(disable)` disables the vectorizer 
completely, including interleaving. @Meinersbur, what do you think? I think it 
would be good to make that clear in the commit message.


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

friendly ping.

Shall we get this in, so that I can commit this and  D66290 
? Then, we can perhaps continue the interleave 
discussion separately? 
I will then return to the doc patch first in D66199 
, so that we can start wrapping up the loop 
pragma business, and hopefully it is a lot more consistent!


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> Therefore, vectorize(disable) would also disable interleaving?

I don't have strong opinions on this. I think there's something to say for both 
options that we have (i.e. `vectorize(disable)` disables interleaving or 
enables it).
But I think it was @fhahn who mentioned that it would be possible to disable 
vectorisation, but still do interleaving. Perhaps bit of a edge use use, but 
again, it's possible currently. That would mean we don't need to disable 
interleaving here I think.


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Therefore, `vectorize(disable)` would also disable interleaving?


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

https://reviews.llvm.org/D66796



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

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

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

This changes the behaviour of vectorize(disable). I.e., disabling vectorization
now means disabling vectorization, and not setting the vectorization width to 1.

This is a follow up of the discussion on the behaviour of different loop
pragmas, and that setting transformation options should imply enabling the
transformation, see also
http://lists.llvm.org/pipermail/cfe-dev/2019-August/063054.html.

Not only is this change in the behaviour of vectorize(disable) probably more 
sensible,
it also helps in implementing options implying transformations.


https://reviews.llvm.org/D66796

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


Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -178,8 +178,8 @@
 // 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_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], 
![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_DISABLE:.*]]}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // 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:.*]]}
Index: clang/test/CodeGenCXX/pragma-loop-safety.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-safety.cpp
+++ clang/test/CodeGenCXX/pragma-loop-safety.cpp
@@ -53,6 +53,6 @@
 // CHECK: ![[INTERLEAVE_1]] = !{!"llvm.loop.interleave.count", i32 1}
 // CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[ACCESS_GROUP_8]] = distinct !{}
-// CHECK: ![[LOOP2_HINTS]] = distinct !{![[LOOP2_HINTS]], 
![[PARALLEL_ACCESSES_11:[0-9]+]], ![[UNROLL_DISABLE]], ![[WIDTH_1:[0-9]+]], 
![[INTENABLE_1]]}
+// CHECK: ![[LOOP2_HINTS]] = distinct !{![[LOOP2_HINTS]], 
![[PARALLEL_ACCESSES_11:[0-9]+]], ![[UNROLL_DISABLE]], 
![[VECTORIZE_DISABLE:[0-9]+]]}
 // CHECK: ![[PARALLEL_ACCESSES_11]] = !{!"llvm.loop.parallel_accesses", 
![[ACCESS_GROUP_8]]}
-// CHECK: ![[WIDTH_1]] = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
Index: clang/test/CodeGenCXX/pragma-loop-predicate.cpp
===
--- clang/test/CodeGenCXX/pragma-loop-predicate.cpp
+++ clang/test/CodeGenCXX/pragma-loop-predicate.cpp
@@ -71,6 +71,6 @@
 // CHECK-NEXT: ![[LOOP3]] = distinct !{![[LOOP3]], !5, !3}
 
 // CHECK-NEXT: ![[LOOP4]] = distinct !{![[LOOP4]], !10}
-// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.width", i32 1}
+// CHECK-NEXT: !10 = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // CHECK-NEXT: ![[LOOP5]] = distinct !{![[LOOP5]], !10}
Index: clang/lib/CodeGen/CGLoopInfo.cpp
===
--- clang/lib/CodeGen/CGLoopInfo.cpp
+++ clang/lib/CodeGen/CGLoopInfo.cpp
@@ -617,8 +617,7 @@
 case LoopHintAttr::Disable:
   switch (Option) {
   case LoopHintAttr::Vectorize:
-// Disable vectorization by specifying a width of 1.
-setVectorizeWidth(1);
+setVectorizeEnable(false);
 break;
   case LoopHintAttr::Interleave:
 // Disable interleaving by speciyfing a count of 1.


Index: clang/test/CodeGenCXX/pragma-loop.cpp
===
--- clang/test/CodeGenCXX/pragma-loop.cpp
+++ clang/test/CodeGenCXX/pragma-loop.cpp
@@ -178,8 +178,8 @@
 // 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_5]] = distinct !{![[LOOP_5]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[VECTORIZE_DISABLE:.*]]}
+// CHECK: ![[VECTORIZE_DISABLE]] = !{!"llvm.loop.vectorize.enable", i1 false}
 
 // 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:.*]]}
Index: clang/test/CodeGenCXX/pragma-loop-safety.cpp
===
--- clang/test/CodeGenCXX/