<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Hi Richard,</div><div><br></div><div>Thanks for the review. Sorry for the delayed response. I applied most of your suggestions. I have some comments on your suggestions for the diagnostic messages. Please review the attached patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+++ b/include/clang/Basic/DiagnosticParseKinds.td<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>+def err_pragma_loop_missing_argument : Error<</div><div>+  "missing argument; expected %select{a positive 32-bit integer value|"</div></div><div><br></div><div>This "expected a positive 32-bit integer value" wording seems like the wrong thing to be saying here, because it doesn't actually tell the user what they missed out. It'd be much friendlier to say "expected loop unroll count" or similar. Also, the sign and bit width information is a distraction from the message.</div></div></blockquote><div><br></div><div>I went with "expected an integer value". The loop hint option, such as vectorize_width is already indicated by the error message with a caret pointing to it. Including more information makes the error message kind of redundant. Let me know what you think.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div><div>+def err_pragma_loop_invalid_argument_type : Error<</div><div>+  "invalid argument %0; expected a 32-bit integer value">;</div></div><div><br></div><div>You should specifically say that the type is wrong here. Something like "invalid argument of type %0; loop %select{unroll|interleave}1 count should have integer type" would be better.</div></div></blockquote><div><br></div><div>I went with "expected an integer type". See my previous rational.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>+def err_pragma_loop_invalid_argument_value : Error<</div><div>+  "invalid argument '%0'; expected a positive 32-bit integer value">;</div></div><div><br></div><div>Arguably 0x80000000u is a positive 32-bit integer value, but you reject it. Something like "invalid argument %0; loop %select{unroll|interleave}1 count must be %select{positive|%3 or smaller}2" would be nice.</div></div></blockquote><div><br></div><div>We cannot know what the maximums are at this point. Fortunately, the optimization passes will generate error messages when the loop directive cannot be followed. For example, an error is produce when the specified vector width is too large. So we just need to make sure the input fits into the IR. But we also shouldn't mislead the programmer with incorrect maximums. For example, 'loop unroll must be 1000000 or smaller' is a poor error because that maximum probably won't work.</div><div><br></div><div>I went with 'invalid value '%0'; expected a positive 32-bit signed integer value'.</div><div><br></div><div>Tyler</div></div></div></div>