<div dir="ltr">Rafael,<div><br></div><div>Thanks for taking the time to do such a thorough review, including doing some benchmarking of the change. :)</div><div><br></div><div>Cheers,</div><div>Kaelyn</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Fri, Dec 6, 2013 at 4:11 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On 5 December 2013 15:40, Kaelyn Takata <<a href="mailto:rikka@google.com">rikka@google.com</a>> wrote:<br>
> I'm pretty sure it isn't because of the mutually recursive calls between<br>
> between isSized and isSizedDerivedType.<br>
<br>
</div>OK,  sorry for taking so long. I spent most of the time trying to<br>
figure out how to benchmark changes on this area once I noticed how<br>
hot this path was.<br>
<br>
In the end I went with<br>
<br>
sudo schedtool -F  -a 0x4 -p 99 -e perf stat -r 5 ./bin/opt -O2<br>
-disable-output -disable-verify gcc.bc<br>
<br>
Where gcc.bc is from<br>
<a href="http://people.csail.mit.edu/smcc/projects/single-file-programs/" target="_blank">http://people.csail.mit.edu/smcc/projects/single-file-programs/</a>.<br>
<br>
I used -disable-very since it is expected that the verifier would slow<br>
down a bit, I just wanted to make sure the rest didn't.<br>
<br>
Some observations:<br>
* On code that passes the verifier with your patch we would not hit a<br>
recursive type, so it is OK for non-verifier code to pass a NULL.<br>
* Experimenting with always creating a set and passing it to an impl<br>
function showed that it would be too expensive.<br>
* Doing this only on the verifier would require duplicating the logic<br>
or doing a second walk to find if the type is recursive.<br>
<br>
So, in the end, LGTM :-)<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>