On Thursday, May 1, 2014, Richard Barton <<a href="mailto:richard.barton@arm.com">richard.barton@arm.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hi James<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">My assumption is that the underlying type of Value is defined by the MCExpr class which presumably needs to be able to handle multiple value sizes and signednesses, so int64 was chosen as it can accommodate all values.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I was taking the assert statement on face value “expression value must be representable in 32 bits” which means signed or unsigned to me. There are obviously loads of ways that you can code the check.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hopefully Saleem can update us on whether the value should indeed be unsigned.</span></p>
</div></blockquote><div>Correct. The underlying type is int64_t as we are evaluating a machine expression. Treating the value as unsigned here is correct. The constraint is that we must be able to represent the immediate as part of the instruction. The assertion is there since the underlying type is larger and would simply pass through unnoticed and that worried me.<span></span></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Ta<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Rich<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm"><p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="javascript:_e(%7B%7D,'cvml','mankeyrabbit@gmail.com');" target="_blank">mankeyrabbit@gmail.com</a> [mailto:<a href="javascript:_e(%7B%7D,'cvml','mankeyrabbit@gmail.com');" target="_blank">mankeyrabbit@gmail.com</a>] <b>On Behalf Of </b>James Molloy<br>
<b>Sent:</b> 01 May 2014 13:01<br><b>To:</b> Richard Barton<br><b>Cc:</b> LLVM Commits<br><b>Subject:</b> Re: [llvm] r207747 - Correction to assert statemtent to allow 32-bit unsigned numbers with the top bit set.<u></u><u></u></span></p>
</div></div><p class="MsoNormal"><u></u> <u></u></p><div><p class="MsoNormal">Hi Rich,<u></u><u></u></p><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">The assert statement now looks rather odd. From the testcase it looks like Value needs to be unsigned, something like:<u></u><u></u></p>
</div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">+ assert((uint64_t)Value <= UINT32_MAX);<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">
Or does that code also need to handle signed values for other cases?<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">Cheers,<u></u><u></u></p></div><div><p class="MsoNormal">
<u></u> <u></u></p></div><div><p class="MsoNormal">James<u></u><u></u></p></div></div><div><p class="MsoNormal" style="margin-bottom:12.0pt"><u></u> <u></u></p><div><p class="MsoNormal">On 1 May 2014 12:37, Richard Barton <<a href="javascript:_e(%7B%7D,'cvml','richard.barton@arm.com');" target="_blank">richard.barton@arm.com</a>> wrote:<u></u><u></u></p>
<p class="MsoNormal">Author: rbarton<br>Date: Thu May 1 06:37:44 2014<br>New Revision: 207747<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=207747&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=207747&view=rev</a><br>
Log:<br>Correction to assert statemtent to allow 32-bit unsigned numbers with the top bit set.<br><br>This fixes an ARM assembler crash - regression test added.<br><br><br>Modified:<br> llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp<br>
llvm/trunk/test/MC/ARM/thumb2-diagnostics.s<br><br>Modified: llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp?rev=207747&r1=207746&r2=207747&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp?rev=207747&r1=207746&r2=207747&view=diff</a><br>
==============================================================================<br>--- llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (original)<br>+++ llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp Thu May 1 06:37:44 2014<br>
@@ -9487,8 +9487,8 @@ unsigned ARMAsmParser::validateTargetOpe<br> int64_t Value;<br> if (!SOExpr->EvaluateAsAbsolute(Value))<br> return Match_Success;<br>- assert((Value >= INT32_MIN && Value <= INT32_MAX) &&<br>
- "expression value must be representiable in 32 bits");<br>+ assert((Value >= INT32_MIN && Value <= UINT32_MAX) &&<br>+ "expression value must be representable in 32 bits");<br>
}<br> break;<br> case MCK_GPRPair:<br><br>Modified: llvm/trunk/test/MC/ARM/thumb2-diagnostics.s<br>URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/thumb2-diagnostics.s?rev=207747&r1=207746&r2=207747&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/thumb2-diagnostics.s?rev=207747&r1=207746&r2=207747&view=diff</a><br>
==============================================================================<br>--- llvm/trunk/test/MC/ARM/thumb2-diagnostics.s (original)<br>+++ llvm/trunk/test/MC/ARM/thumb2-diagnostics.s Thu May 1 06:37:44 2014<br>@@ -81,3 +81,10 @@ foo2:<br>
@ CHECK-ERRORS: ^<br> @ CHECK-ERRORS: error: immediate expression for mov requires :lower16: or :upper16<br> @ CHECK-ERRORS: ^<br>+<br>+ and sp, r1, #80008000<br>+ and pc, r1, #80008000<br>
+@ CHECK-ERRORS: error: invalid operand for instruction<br>+@ CHECK-ERRORS: error: invalid operand for instruction<br>+<br>+<br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="javascript:_e(%7B%7D,'cvml','llvm-commits@cs.uiuc.edu');" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><u></u><u></u></p></div><p class="MsoNormal"><u></u> <u></u></p></div></div></div>
</blockquote><br><br>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org<br>