<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>Hi, Francesco,</p>
<p>I've taken a look at the patch, and I think that this is a good
approach. One thing that I want to point out here is that the
recipe mentioned in the patch description, including:</p>
<ul class="remarkup-list" style="margin: 12px 0px 12px 30px;
padding: 0px; border: 0px; list-style: disc; color: rgb(0, 0, 0);
font-family: "Segoe UI", "Segoe UI Emoji",
"Segoe UI Symbol", Lato, "Helvetica Neue",
Helvetica, Arial, sans-serif; font-size: 13px; font-style: normal;
font-variant-ligatures: normal; font-variant-caps: normal;
font-weight: 400; letter-spacing: normal; orphans: 2; text-align:
left; text-indent: 0px; text-transform: none; white-space: normal;
widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;
background-color: rgb(255, 255, 255); text-decoration-style:
initial; text-decoration-color: initial;">
<li class="remarkup-list-item" style="margin: 0px; padding: 0px;
border: 0px; line-height: 1.7em;"><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere; margin-top: 0px;">VF == 1</tt><span> </span>replaced
with<span> </span><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere;">VF.isScalar()</tt>.</li>
<li class="remarkup-list-item" style="margin: 0px; padding: 0px;
border: 0px; line-height: 1.7em;"><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere; margin-top: 0px;">VF > 1</tt><span> </span>and<span> </span><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere;">VF >=2</tt><span> </span>replaced
with<span> </span><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere;">VF.isVector()</tt>.</li>
<li class="remarkup-list-item" style="margin: 0px; padding: 0px;
border: 0px; line-height: 1.7em;"><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere; margin-top: 0px;">VF <=1</tt><span> </span>is
replaced with<span> </span><tt class="remarkup-monospaced" style="color: rgb(0, 0, 0); background: rgba(71, 87, 120, 0.1); padding: 1px 4px; border-radius: 3px; white-space: pre-wrap; line-break: anywhere;">VF.isZero() || VF.isScalar()</tt>.</li>
</ul>
<p>appears to me to be a nice readability enhancement to the code
independent of anything else. There might be a lot of changes, but
they seem mostly mechanical, and they seem to make the code more
readable, so they pay for themselves in that regard.</p>
<p>Thanks again,</p>
<p>Hal</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 8/11/20 5:58 PM, Francesco
Petrogalli via llvm-dev wrote:<br>
</div>
<blockquote type="cite" cite="mid:CB40ECF1-588B-4887-AD8C-AFFD16DB2900@arm.com">
<pre class="moz-quote-pre" wrap="">Hi all,
I am working on replacing `unsigned VF` with `ElementCount VF` in the vectorizer.
I have submitted an RFC patch to fabricator just to show how it looks like: <a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D85794">https://reviews.llvm.org/D85794</a>
Just to be clear. I am not trying to get this patch in as it is, I am just asking for some feedback on the approach.
The changes in the vectorizer are numerous, but mostly mechanical.
I have explained in the description of the patch the changes, which fall in two categories:
1. changes outsize the vectorizer, which are needed to be able to query things like "how many lanes" are in this "vectorization factor", or to print things to debug streams or remarks.
2. changes in the vectorizer that are needed to guarantee a _non functional change_, for example asserting that the ElementCount instance being used to compute a vector reverse cannot be scalable. The code essentially looks the same as before this patch, other than for the fact that I have added such assertions.
I am aware the changes in LoopVectorizer.cpp might be a lot. I am open to consider different incremental approaches.
One data point here is that my initial approach was to add the `bool Scalable` property to the VectorizationFactor class.
However, using ElementCount instead of the pair {unsigned, bool} for dealing with the number of lanes inside the VectorizationFactor seemed a natural choice that was worth investigating.
Please let me know what you think!
Kind regards,
Francesco
_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
<pre class="moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</body>
</html>