<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div></div></div><blockquote type="cite">Thanks for working on this. The code looks correct. I agree with Arnold that this function is becoming even more complicated and it would be nice to refactor it somehow, but we can do it later.</blockquote><br><div><br></div>Take the following as future improvement suggestions, then :)<div><br><div><div><div>On Feb 20, 2013, at 12:51 PM, Renato Golin <<a href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">On 20 February 2013 18:31, Arnold Schwaighofer <span dir="ltr"><<a href="mailto:aschwaighofer@apple.com" target="_blank">aschwaighofer@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">This looks like unnecessary code duplication:<br></blockquote><div>> (...) </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">Can you put this into a function?<br></blockquote><div><br></div><div>So, this one deserves a bit more of explanation. I haven't done it because that part of the code uses a typedef from inside the function. If I move it to a separate function I'd have to have the typedef external, which is not a problem per se (since it'll be local to a file), but some people consider it code pollution.</div><div><br></div><div>If you guys don't mind, I thought about adding a few typedefs regarding the types and iterators, so I can use them by name and make the code simpler.</div><div><br></div></div></div></div></blockquote><div><br></div>Since you are in an anonymous namespace (that is, not a public interface) you can have the typedef in the enclosing class? (This file already does that)</div><div><br></div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">Can you move the test cases into one file? The more single file .ll tests we have the more we pay for file open/close operations. And we want make check to stay fast :).<br></blockquote><div><br></div><div>I had one case, but specifically moved to one per case, as my previous test Nadav asked to split into smaller tests (maybe I got it wrong). I actually prefer one big test if the items are closely related. If every one agrees, I'll merge them back.</div></div></div></div></blockquote><div><br></div><div>Okay. You can always use --check-prefix=ALIAS1 if you want to register a failure as an individual failure.</div><div><br></div><div>ALIAS1-CHECK:</div><div><br></div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="im"><span style="color: rgb(34, 34, 34);">Am I missing something?</span></div></blockquote><div><br></div><div>The main thing is the iteration that goes from one to two nested loops, and I was already adding too many nested loops to the code. With that being in a separate function, it might not be a problem any more. I'll see if I can merge them</div></div></div></div></blockquote><div><br></div>I still don't see where we would add a loop/complexity in a solution using DenseMap<value*, Vector> stead of a multimap without an example :).</div><div><br></div><div>Your</div><div><br></div><div>+ std::pair<StoreAliasMap::iterator, StoreAliasMap::iterator> range =<br>+ WriteObjects.equal_range(*UI);<br>+ for (StoreAliasMap::iterator it=range.first, end=range.second;<br>+ it != end; ++it) {<br></div><div><br></div><div>would become</div><div><br></div><div>SmallVector<Inst*,4> &EqualRange = WriteObjects[*UI];</div><div>for (it = EqualRange.begin(), end = EqualRange[*UI].end();</div><div><br></div><div>The remaing code should stay mostly the same except for the changed types?</div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="im"><span style="color: rgb(34, 34, 34);">Punctuation.</span></div></blockquote><div><br></div><div>Hum, that's a weird thing, but I see from the developer's policy that all comments have full stop. I guess I'll just have to get used to. ;)</div></div></div></div></blockquote><br></div></div><div>I said nitpicking :)</div></div></body></html>