[PATCH] Global Structure Vectorization

[Arnold Schwaighofer]

> 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.


Take the following as future improvement suggestions, then :)

On Feb 20, 2013, at 12:51 PM, Renato Golin <renato.golin at linaro.org> wrote:

> On 20 February 2013 18:31, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:
> This looks like unnecessary code duplication:
> > (...) 
> Can you put this into a function?
> 
> 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.
> 
> 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.
> 

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)


> 
> 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 :).
> 
> 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.

Okay. You can always use --check-prefix=ALIAS1 if you want to register a failure as an individual failure.

ALIAS1-CHECK:


> Am I missing something?
> 
> 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

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 :).

Your

+      std::pair<StoreAliasMap::iterator, StoreAliasMap::iterator> range =
+          WriteObjects.equal_range(*UI);
+      for (StoreAliasMap::iterator it=range.first, end=range.second;
+            it != end; ++it) {

would become

SmallVector<Inst*,4> &EqualRange = WriteObjects[*UI];
for (it = EqualRange.begin(), end = EqualRange[*UI].end();

The remaing code should stay mostly the same except for the changed types?

> 
> Punctuation.
> 
> 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. ;)

I said nitpicking :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130220/94389d2b/attachment.html>