<div dir="ltr">Jim,<div><br></div><div style>I will address your concerns in a subsequent patch. </div><div style>You are correct - the same issue is present in the Thumb encoding space as well. </div><div style>I have left that alone simply because ARM encodings are of higher priority at the moment, but will fix in the near future.</div>
<div style><br></div><div style>Mihai</div><div style><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Apr 17, 2013 at 11:52 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div>Thanks, much clearer now. This is a nice tightening up of things.<div>
<br></div><div>Also, thanks for reviewing this Quentin. I just a have a few additional comments on some of the details.</div><div><br></div><div>+def Imm0_4AsmOperand : ImmAsmOperand { let Name = "Imm0_4"; }</div>
<div><br></div><div>This should have a custom DiagnosticType as well to help get better error messages for out of range values. We don’t use this facility as much as we should in the ARM backend, but it’s worth doing. Imm0_15AsmOperand has an example of how this works.</div>
<div><br></div><div><br></div><div><div>-def HINT : AI<(outs), (ins imm0_255:$imm), MiscFrm, NoItinerary,</div><div>+def HINT : AI<(outs), (ins imm0_4:$imm), MiscFrm, NoItinerary,</div><div> "hint", "\t$imm", []>, Requires<[IsARM, HasV6]> {</div>
<div>- bits<8> imm;</div><div>- let Inst{27-8} = 0b00110010000011110000;</div><div>- let Inst{7-0} = imm;</div><div>+ bits<3> imm;</div><div>+ let Inst{27-3} = 0b0011001000001111000000000;</div><div>+ let Inst{2-0} = imm;</div>
<div> }</div><div><br></div><div>This is only for the ARM mode of the instruction. Don’t restrictions like this also apply to the Thumb2 encodings?</div><div><br></div><div><div> @ CHECK: yield @ encoding: [0x01,0xf0,0x20,0xe3]</div>
<div> @ CHECK: yieldne @ encoding: [0x01,0xf0,0x20,0x13]</div><div>-@ CHECK: hint<span style="white-space:pre-wrap"> </span>#5 @ encoding: [0x05,0xf0,0x20,0xe3]</div><div>+@ CHECK-NOT: hint<span style="white-space:pre-wrap"> </span>#5</div>
<div> @ CHECK: sev </div></div><div><br></div><div>A better way to handle this is that “hint #5” should be removed entirely from this test file since it’s not a valid encoding. It should be replaced with test cases in diagnostics.s and thumb-diagnostics.s which check that the expected diagnostic is produced. As is, for example, we get "error: instruction requires: thumb2” which isn’t correct, since this instruction shouldn’t be valid for thumb2 either. We should explicitly check for the new diagnostic we add via DiagnosticType.</div>
<div><br></div><div>Jim</div><div><br></div><div><br></div><div><div class="im"><div>On Apr 17, 2013, at 3:10 AM, Mihail Popa <<a href="mailto:mihail.popa@gmail.com" target="_blank">mihail.popa@gmail.com</a>> wrote:</div>
<br></div><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><div class="h5"><div dir="ltr">Hi.<div><br></div><div>I have amended the patch by removing white space trimming and replacing grep with FileCheck.</div>
<div><br></div><div>Regards,</div><div>Mihai</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 16, 2013 at 7:47 PM, Quentin Colombet<span> </span><span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span><span> </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 style="word-wrap:break-word">Oh yes, that’s true. I’ve forgotten about that!<div>
<span><font color="#888888"><br><div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
-Quentin</div></div></font></span><div><br><div><div>On Apr 16, 2013, at 11:36 AM, Tim Northover <<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>> wrote:</div><br><blockquote type="cite">
<div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite">Maybe I’m missing something but you’ve defined a new isImm0_4 method but<br>
you’re never using it.<br></blockquote><br>That's automatically used by TableGen based on the Name member in the<br>AsmOperandClass, isn't it?<br><br>Tim.</div></blockquote></div><br></div></div></div><br>_______________________________________________<br>
llvm-commits mailing list<br><a href="mailto: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><br>
<br></blockquote></div><br></div></div></div><span><LLVM-713.hints.patch></span>_______________________________________________<div class="im"><br>llvm-commits mailing list<br><a href="mailto: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></div></div></blockquote></div><br></div></div></blockquote></div><br></div>