<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Sep 13, 2010, at 11:04 AM, Gabor Greif wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On 12 Sep., 02:04, Bill Wendling <wendl...@apple.com> wrote:<br><blockquote type="cite">I think the TST instruction can take a compare value other than 0. The CMP instructions take 0, right?</blockquote></div></blockquote><blockquote type="cite"><div>TST instructions have 2 operands and the second is a mask. It is a<br>hybrid of "and" and "cmp" against 0.<br></div></blockquote><br><div>The third operand is a "shift" when using a register-register test (according to my documentation). But we covered that in a previous email.</div><br><blockquote type="cite"><div><blockquote type="cite">Below is what I have in my tree right now. It's just the skeleton of how I'd like to handle this (it does nothing right now). I like it because of how it separates out different optimization potentialities for each type of instruction, but it may be overkill...</blockquote><br>Yes, I think it is a bit of overkill. Logically TST is a compare<br>instruction. In my last patch I successfully handle both with one<br>sweep. If you think my approach is sufficient and works well on your<br>testcases, I gladly check it in. Then I would like to establish<br>something like<br><br>dyn_cast<ARMri<ARM::ANDri> ><br>queries (with additional intelligence) for checking opcodes and<br>getting operands. Would simplify my code a great amount.<br><br></div></blockquote>Here's your patch from another email. My comments are in-line:</div><div><br></div><div><span class="Apple-style-span" style="font-family: monospace; ">gabor@google8:~/llvm-build$ svn diff /home/gabor/llvm/lib/Target/ARM/</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">ARMBaseInstrInfo.cpp</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">Index: /home/gabor/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">===================================================================</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">--- /home/gabor/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">(revision 113747)</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+++ /home/gabor/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">(working copy)</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">@@ -1372,6 +1372,19 @@</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> SrcReg = MI->getOperand(0).getReg();</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> CmpValue = MI->getOperand(1).getImm();</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> return true;</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ case ARM::TSTri: {</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ if (MI->getParent()->begin() == </span><span class="Apple-style-span" style="font-family: monospace; ">MachineBasicBlock::const_iterator(MI))</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ return false;</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ const MachineInstr *AND = llvm::prior(MI);</span></div><div><span class="Apple-style-span" style="font-family: monospace; "><br></span></div><div><span class="Apple-style-span" style="font-family: monospace; ">The AND doesn't have to be the immediately prior instruction. I could happen further up in the BB just as long as there isn't an intervening instruction which sets CPSR.</span></div><div><span class="Apple-style-span" style="font-family: monospace; "></span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ if (AND->getOpcode() != ARM::ANDri)</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ return false;</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ if (MI->getOperand(0).getReg() == AND->getOperand(1).getReg() </span><span class="Apple-style-span" style="font-family: monospace; ">&& // FIXME: or == AND</span></div><div><span class="Apple-style-span" style="font-family: monospace; ">+ MI->getOperand(1).getImm() == AND->getOperand(2).getImm()) </span><span class="Apple-style-span" style="font-family: monospace; ">{// FIXME: subset</span></div><div><span class="Apple-style-span" style="font-family: monospace; ">+ SrcReg = AND->getOperand(0).getReg();</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ CmpValue = 0;</span></div><div><span class="Apple-style-span" style="font-family: monospace; "><br></span></div><div><span class="Apple-style-span" style="font-family: monospace; ">CmpValue might not be 0 here.</span></div><div><font class="Apple-style-span" face="monospace"><br></font></div><div><span class="Apple-style-span" style="font-family: monospace; ">+ return true;</span></div><div><span class="Apple-style-span" style="font-family: monospace; ">+ }</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ }</span></div><div><span class="Apple-style-span" style="font-family: monospace; "><br></span></div><div><span class="Apple-style-span" style="font-family: monospace; ">There's no need for this to be as complex here. The logic should be pretty much identical to the CMP cases. In fact, I think it is identical when you have a TST of an immediate. This logic should reside in </span><font class="Apple-style-span" face="monospace">ARMBaseInstrInfo::</font><span class="Apple-style-span" style="font-family: monospace; ">OptimizeCompareInstr. Once you do that, the tests of SrcReg & CmpValue become trivial.</span></div><div><span class="Apple-style-span" style="font-family: monospace; "><br></span></div><div><span class="Apple-style-span" style="font-family: monospace; ">I need to comment this method better. It's supposed to return the source register and compare values of the comparison instruction.</span></div><div><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> }</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> return false;</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">@@ -1421,6 +1434,7 @@</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> switch (MI->getOpcode()) {</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> default: break;</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> case ARM::ADDri:</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; ">+ case ARM::ANDri:</span></div><div><span class="Apple-style-span" style="font-family: monospace; "><br></span></div><div><span class="Apple-style-span" style="font-family: monospace; ">Also do t2ANDri.</span></div><div><span class="Apple-style-span" style="font-family: monospace; "></span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> case ARM::SUBri:</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> case ARM::t2ADDri:</span><span class="Apple-style-span" style="font-family: monospace; "><br></span><span class="Apple-style-span" style="font-family: monospace; "> case ARM::t2SUBri:</span><span class="Apple-style-span" style="font-family: monospace; "><br></span></div><div><br></div><div>To do the optimization you suggest should be almost as easy as simply adding the TST* cases to the switch statements and removing the "if (CmpValue != 0) return false;" from OptimizeCompareInstr. :-)</div><div><br></div><div>-bw</div><div><br></div></body></html>