<div class="gmail_quote">On Mon, Jan 9, 2012 at 1:23 AM, Rotem, Nadav <span dir="ltr"><<a href="mailto:nadav.rotem@intel.com">nadav.rotem@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I am not familiar with the addressing mode code but I have a few small comments.<br></blockquote><div><br></div><div>Thanks for the feedback!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Why is this optimization disabled for vectors ?  You can use getScalarSizeInBits and I think that it should work.<br></blockquote><div><br></div><div>I'm not familiar w/ the semantics of the masked bits computation, etc. I'm happy to add vector support to this, but I'd like to do it in a second patch with appropriate tests that it does the correct thing.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Return true ? :)<br></blockquote><div><br></div><div>Yea, as I posted to the list, the first patch had a debug line of code in it...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">+  SDValue X = N == Shift ? And.getOperand(0) : Shift.getOperand(0);</div>
<br>
Can you add parenthesis here ?<br></blockquote><div><br></div><div>Done, I think. At least, the only precedence I find surprising is that of the entire conditional expression, not the condition itself...</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">+  // The amount of shift we're trying ot fit into the addressing mode is taken</div>
<br>
Typo ot -> to.<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+  SDValue NewShiftAmt = DAG.getConstant(ShiftAmt + AMShiftAmt, MVT::i8);<br>

<br>
Please use getShiftAmountTy.<br></blockquote><div><br></div><div>None of the other address mode computations use it. =/ It seems like goodness, but it requires threading a TargetLowing handle into this code. I've got 4 or 5 more patches queued up behind this that changing the interface in that way would make kind of annoying. I'd prefer to clean up all of the i8 shift types in one pass afterward. That sound good to you?</div>
<div><br></div><div><br></div><div>I've attached an updated patch, but I made a significant change beyond your comments. The previous patch could cause unfortunate re-computations because we never re-computed the final value in a non-addressing-mode context. I've changed the code to do so, and replace uses of the final value with that computation, while leaving the addressing mode able to select the simpler computation. Subsequent passes of ISel will trivially fold the left-shifts into addressing modes as well when possible.</div>
<div><br></div><div>I've also disable the transform when the pre-masked shifted value has multiple users as that will also force those users to re-compute the value. I'm actually a bit dubious on this one, as it seems like it might still be a win to do the transform, but I decided to start safer, and make this more aggressive in follow-ups if benchmarks indicate it is a good tradeoff.</div>
</div>