<div dir="ltr">Thanks for the review. Committed in r218064.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 18, 2014 at 11:22 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">LGTM. Thanks!<span class="HOEnZb"><font color="#888888"><div><br></div><div>-eric</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 18, 2014 at 11:17 AM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The attached patch is a follow-up to r217994. I defined a new function validateOperandSize, which is used to check both input and output sizes.<div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 6:07 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Cool, thanks.<span><font color="#888888"><div><br></div><div>-eric</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 6:02 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">OK, I'll check in a patch that fixes X86_32TargetInfo::validateInputSize first then.<div><div><div><br></div><div class="gmail_extra"><div class="gmail_quote">On Tue, Sep 16, 2014 at 5:55 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Sep 16, 2014 at 5:53 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span>On Tue, Sep 16, 2014 at 5:12 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br></span><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Sep 16, 2014 at 4:00 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span>On Tue, Sep 16, 2014 at 1:06 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br></span><div class="gmail_extra"><div class="gmail_quote"><span><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">You'll want to split out the new contraints for input size into a separate patch. (And just commit it).<br>A small comment of why we're ignoring dependent types would be good.<div><br></div><div>One question: Why not just add all of the contraints first rather than piecemeal as you get testcases? (Related to the comment above).</div><div><br></div></blockquote><div><br></div></span><div>Just to make sure I'm not misunderstanding your question, are you suggesting I use "=abcdSD" instead of "=a" in the test case and do the check in one line?</div><div> </div><div><span style="font-family:Menlo;font-size:11px">uint64_t val;</span></div><div><p style="margin:0px;font-size:11px;font-family:Menlo">__asm__ volatile("addl %1, %0" : "=abcdSD" (val) : "a" (msr)); // expected-error {{invalid output size for constraint '=abcdSD'}}</p><p style="margin:0px;font-size:11px;font-family:Menlo"><span style="font-family:arial;font-size:small"><br></span></p><p style="margin:0px">Are you also suggesting that we should have clang print just the constraints that are invalid in the error message? For example, if we added "A" and use "=abcdSDA" instead, clang would remove "A", since it can be bound to a 64-bit variable, and print "=abcdSD" or "abcdSD" instead?</p><p style="margin:0px;font-size:11px;font-family:Menlo"><br></p></div></div></div></div></blockquote><div><br></div></span><div>No, I'm curious why you're adding S and D now, but not any other constraint that has a size associated with the register.</div><span><font color="#888888"><div><br></div></font></span></div></div></div></blockquote><div><br></div></span><div>OK, I see. I just felt that S and D should be added too, since they are single register constraints that have to be bound to variables smaller than 64-bit, as constraints a-d are.</div><div><br></div><div>I can probably add R, q, Q, to the switch-case statement too. Also, in my next patch, I was going to add checks for constraints x and y.</div><div> <br></div><div>Should I add the all the constraints I mentioned above to X86_32TargetInfo::validateInputSize or X86TargetInfo::validateInputSize first and then add the checks for output constraints?</div></div></div></div></blockquote><div><br></div></span><div>Seems like a reasonable way to go yes?</div><span><font color="#888888"><div><br></div><div>-eric</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div></div><div>-eric</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><p style="margin:0px;font-size:11px;font-family:Menlo"></p><p style="margin:0px;font-size:11px;font-family:Menlo"><br></p></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></div><div>Thanks!</div><span><font color="#888888"><div><br></div><div>-eric</div></font></span><div><div><br><div class="gmail_quote">On Fri Aug 29 2014 at 4:46:37 PM Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br><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">Does the latest patch look fine? I am working on another patch which fixes a similar bug and I need to commit this patch first.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 28, 2014 at 10:08 AM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<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="auto"><div><span></span></div><div><div dir="ltr">Latest version of the patch is attached which fixes a couple of oversights. I had to add a line which checks whether Ty is a dependent type before getTypeSize is called. Also, in the test case, "=" was missing before constraint "a", so fixed that too.<div class="gmail_extra">
<br><div class="gmail_quote"><div>On Wed, Aug 27, 2014 at 3:22 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><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>New patch looks good to me.</div><div><br></div>It sounds like we have two cases of size mismatch:<div>- The output operand lvalue is smaller than the constraint, meaning the store will write out of bounds. Your patch adds this.</div>
<div>- The output operand lvalue is bigger than the constraint, meaning the whole value won't be initialized. We currently warn here via validateConstraintModifier.</div><div><br></div><div>This code probably deserves some cleanup, but your patch is consistent with what we do for input operands, so let's go with that.</div>
</div><div><div><div class="gmail_extra"><br></div></div></div></blockquote><div><br></div></div><div>The reason llvm is crashing in the backend is that it's trying to use a 64-bit register in 32-bit mode. It's not because a store is writing out of bounds or there is a value left uninitialized. In the test case, if we declare the variable bound to constraint "=a" to be a unit32_t or an integer type that is smaller than 32-bit, clang compiles the program fine.</div>
<div><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><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 27, 2014 at 12:35 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<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">The commit log in r166737 doesn't say much about why this is a warning instead of an error, but I know there are cases where warnings are needed. For example, clang has to issue warnings instead of errors for the inline-asm statements in the test case committed in r216260. If it's not desirable to change validateConstraintModifier, we can add a function which checks the output size that is similar to validateInputSize in r167717 (see attached patch), which was suggested in the post-commit review.<div>
<div><br></div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121112/067945.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121112/067945.html</a><br></div><div>
<br></div><div>
I am not sure whether we can use fixit in this case. Fixit hints should be used only if we know the user's intent and it's very clear that applying the fixit hint is the right thing to do. Changing the type of variable "r" to a 32-bit int will avoid crashing, but it doesn't look like that's what the user wants.</div>
<div><br></div></div><div><div><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 25, 2014 at 6:20 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br>
<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">Can you investigate why we are warning in the first place? I think we should either only warn or only error. Currently we have a warning with a fixit but we don't recover as though we had applied the fixit. If we did that, we would not crash.<div>
<br></div><div>In addition to the Clang-side changes, LLVM should probably be returning an error or reporting a fatal error instead of hitting unreachable.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div>
On Mon, Aug 25, 2014 at 2:10 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br></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><div>
<div dir="ltr">Rebased patches attached.<div><br></div><div>I also made changes to the clang patch so that clang can error-out after a size mismatch is found as soon as possible.TargetInfo::validateConstraintModifier has an extra parameter IsError, which is set when it decides there is no point in continuing compilation and it should stop compilation immediately. The error message clang prints looks better than lllvm's message, but if it isn't right to change the warning to an error, then I guess we have to detect the error later just before isel, as is done in the llvm patch.</div>
<div><div>
<div><br></div><div><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 23, 2014 at 3:56 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<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">llvm should error-out when a 64-bit variable is bound to a single register in x86 32-bit mode, but ToT clang/llvm fails to detect this error and continues compilation until it crashes in type-legalization:<div>
<br></div><div><div><p style="margin:0px;font-size:11px;font-family:Menlo">$ llc test/CodeGen/X86/inline-asm-regsize.ll -O3 -mtriple=i386-apple-darwin -o -</p></div><div><br></div><div><p style="margin:0px;font-size:11px;font-family:Menlo">
inline-asm-regsize.ll -O3 -mtriple=i386-apple-darwin -o -</p>
<p style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap"> </span>.section<span style="white-space:pre-wrap"> </span>__TEXT,__text,regular,pure_instructions</p>
<p style="margin:0px;font-size:11px;font-family:Menlo">ExpandIntegerResult #0: 0x7fa2d1041728: i64 = Register %RCX [ID=0]</p>
<p style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></p>
<p style="margin:0px;font-size:11px;font-family:Menlo">Do not know how to expand the result of this operator!</p>
<p style="margin:0px;font-size:11px;font-family:Menlo">UNREACHABLE executed at /Users/ahatanaka/projects/llvm/git/llvm3/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1116!</p></div><div><br></div><div>The attached patch fixes llvm to error-out and print this error message:</div>
<div><br></div><div><p style="margin:0px;font-size:11px;font-family:Menlo">error: Cannot bind a variable larger than 32-bit to a single register in 32-bit mode</p><p style="margin:0px;font-size:11px;font-family:Menlo"><br>
</p><div>My initial solution was to have clang detect this error in TargetInfo::validateConstraintModifier. However, the code in SemaStmtAsm.cpp has to be changed to error-out instead of issuing a warning, which I wasn't sure was the right thing to do. I am attaching this patch too in case someone has a suggestion or an opinion on it.</div>
<div><br></div></div><div><<a>rdar://problem/17476970</a>><br></div><div><br></div></div></div>
</blockquote></div><br></div></div></div></div></div>
<br></div></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</div><div></div></div><br><div dir="auto"><div></div></div><br></blockquote></div><br></div>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>