<div dir="ltr">I hate that routine. We've been here before, repeatedly, each time with order of iteration bugs.<div><br></div><div>Your patch is certainly correct, but I think its worth setting this question to rest by making this a more direct body of code. Here is my suggestion:</div>
<div><br></div><div>- Remove the 'return' from the loop. Every time there is an early exit from this kind of thing, it all goes to shit complexity wise.</div><div>- Have the loop pick a best type *and* the best integral type.</div>
<div>- Keep an explicit "no longer viable" state for the basic "best type" case (due to non-load/store or conflicting types)</div><div><br></div><div>After we look at every slice, return the best type if viable, else the best integral type if viable, else 0.</div>
<div><br></div><div>Essentially, Ty and ITy should have the following transitions:</div><div><br></div><div>Ty: 0 -> <good> -> <bad></div><div>ITy: 0 -> <good></div><div><br></div><div>Ty saturates in the 'bad' state.</div>
<div><br></div><div>I think that would be more clear than even after your patch.</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jan 20, 2014 at 5:08 AM, James Molloy <span dir="ltr"><<a href="mailto:james.molloy@arm.com" target="_blank">james.molloy@arm.com</a>></span> 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"><div><p class="MsoNormal">Hi Chandler,<u></u><u></u></p><p class="MsoNormal">
<u></u> <u></u></p><p class="MsoNormal">The attached patch fixes a bug that could cause SROA to go into infinite recursion (stack overflow in debug mode, infinite loop in release mode).<u></u><u></u></p><p class="MsoNormal">
<u></u> <u></u></p><p class="MsoNormal">The function findCommonType() attempts to use a type from a load/store inst, and if there are non-load-store instructions or their types do not match it falls back to trying to find an integral type that covers all uses. Otherwise, it should return failure (NULL).<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">The mechanism to fall back to integral types does not reset the “winning” type to NULL. This means that depending on the order in which the input uses are iterated over changes the output of the function. If a non-load-store instruction comes first, it will return NULL (as Ty is never set). If a load/store inst comes first, it will return the Ty of that instruction.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">There is no testcase as the bug depends on memory layout; I could not force it to fail with opt/llc.<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">
Is it OK to commit?<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">Cheers,<span class="HOEnZb"><font color="#888888"><u></u><u></u></font></span></p><span class="HOEnZb"><font color="#888888"><p class="MsoNormal">
<u></u> <u></u></p><p class="MsoNormal">James<u></u><u></u></p></font></span></div></div></blockquote></div><br></div>