<div dir="ltr">Hi Tim.<div><br></div><div>Please look at the attached changes.</div><div>I created a custor AsmMatchConverter and simplified the logic a bit.</div><div>Also added extra boundary condition tests. (and fixed style)</div>
<div><br></div><div>Regards,</div><div>Mihai</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 6, 2013 at 4:45 PM, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Mihai,<br>
<br>
This really is a nasty one, isn't it? I don't like the fact that<br>
validateInstruction is now modifying its argument; seems like a bad<br>
precedent to set (all of the others just report on what's present as<br>
far as I can see). How about moving the mutation logic into an<br>
AsmMatchConverter method applied to these four instructions? You'd<br>
still need a stub in validateInstruction to actually check immediates,<br>
but it could be much simpler and really just do validation.<br>
<br>
Other than that, the biggest thing I saw was a possible error:<br>
<br>
+    if(op->isImm() && !op->isSignedOffset<11, 1>())<br>
+    {<br>
+      if(!isThumbTwo() || !op->isSignedOffset<25, 1>())<br>
<br>
Shouldn't that be isSignedOffset<24, 1>? Which suggests some error<br>
tests would be a good idea (well, the would even if this is correct).<br>
A few just-out-of-range immediates to make sure the messages are doing<br>
what they should be.<br>
<br>
For the first block of classification logic, might a more<br>
instruction-centric block be clearer?<br>
<br>
IsAL = ...;<br>
switch(Inst.getOpcode()) {<br>
case ARM::t2Bcc:<br>
  if (inITBlock() || IsAL) Inst.setOpcode(ARM::t2B);<br>
  break;<br>
case ARM::t2B:<br>
  if (!inITBlock() && !IsAL) Inst.setOpcode(ARM::t2Bcc);<br>
  break;<br>
[...same for tB and tBcc...]<br>
}<br>
<br>
I'll leave it to your discretion, but that seems to make it more<br>
obvious that we only care about these 4 instructions, and that the two<br>
conditions are disjoint & complementary.<br>
<br>
Regardless of what you pick there, a minor nit is that the braces are<br>
not in LLVM style.<br>
<br>
Cheers.<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span></blockquote></div><br></div>