<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div>Hi Aaron,</div><div><br></div><div>Thanks for the review! Here is the updated patch and responses to some of your comments. The new tests are a couple of lines added to test/Parser/pragma-loop.cpp. This patch is just refactoring in preparation for new features so there really isn’t a need for new tests.</div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">+ ["default", "enable", "disable"],<br>+ ["Default", "Enable", "Disable"]>,<br></blockquote><br>This isn't really a "default" so much as "it's not Enabled or<br>Disabled", correct? Perhaps UsesValue instead? I know that's not quite<br>accurate either since it'll be used for #pragma unroll with no<br>arguments…</div></blockquote><div><br></div><div>I’m pretty sure Default or something like it is correct. This value is used for #pragma unroll and #pragma nounroll. The default behavior with unroll enables the unroller and nounroll disables it. UsesValue would imply that examining the state is required to determine if the Value parameter is used. What I would like to see in the future is for the state and value to be unioned. And the Option would determine if the state or value was needed. AFAIK Tablegen doesn’t have the ability to express unions right now.</div><div><br></div><div><br></div><div><blockquote type="cite"><blockquote type="cite">+ return true;<br>+ }<br>+<br>+ bool StateOption = false;<br>+ if (OptionInfo) // Pragma unroll does not specify an option.<br></blockquote><br>It doesn't always specify an option, but it sometimes does (otherwise<br>there wouldn't be a .Case below).<br></blockquote><div><br></div><div>The naming of the pragmas is unfortunately a little confusing. #pragma unroll and #pragma nounroll don’t have options. There is no need because they only control unrolling. On the other hand #pragma clang loop has options which can be vectorize, interleave, unroll, etc. So when pragma unroll is used we don’t do the check that determines if the option has an argument for the state or value of the loop hint.</div><div><br></div><div>#pragma unroll -> enables unrolling</div><div>#pragma nounroll -> disables unrolling</div><div>#pragma unroll(…) or #pragma unroll … -> specifies the amount of unrolling</div><div><br></div><div>The first 2 cases are handled by the early test: if (!Info->HasValue && (PragmaUnroll || PragmaNoUnroll)) { … return true; }</div><div><div style="margin: 0px;"><br></div><div style="margin: 0px;">After that we can always assume the 3rd case. So we leave StateOption false and drop down to the part that assumes the argument is a value.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"><br></div></div></div><div>Tyler</div><div><br></div><div></div></div></body></html>