[PATCH] D26967: Put opt-viewer critical items in parallel

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

Hi Brian,

I will also do some experiments with this patch but here are my comments on the patch itself.  In general feel free to peel off your formatting changes and just commit them right away.

Adam



================
Comment at: utils/opt-viewer/opt-viewer.py:60-61
 
-    # Map function names to their source location for function where inlining happened
+    # Map function names to their source location for function where inlining
+    # happened
     caller_loc = dict()
----------------
Please commit formatting changes separately.


================
Comment at: utils/opt-viewer/opt-viewer.py:68-69
             return 0
-        raise AttributeError
+
+        raise AttributeError("'Remark' object has no attribute: '{}'".format(name))
 
----------------
This too, please commit separately. 


================
Comment at: utils/opt-viewer/opt-viewer.py:134-143
     @property
     def key(self):
-        return (self.__class__, self.Pass, self.Name, self.File, self.Line, self.Column, self.message)
+        return (
+            self.__class__,
+            self.Pass,
+            self.Name,
+            self.File,
----------------
Looks like another formatting change.


================
Comment at: utils/opt-viewer/opt-viewer.py:203
 
-        self.html_formatter = HtmlFormatter()
+        self.html_formatter = HtmlFormatter(encoding='utf-8')
         self.cpp_lexer = CppLexer()
----------------
I am still not sure that this is the right thing to do but either way it should go into a separate review.


================
Comment at: utils/opt-viewer/opt-viewer.py:221-222
             link = Remark.make_link(dl['File'], dl['Line'] - 2)
-            inlining_context = "<a href={link}>{r.DemangledFunctionName}</a>".format(**locals())
+            inlining_context = "<a href={link}>{r.DemangledFunctionName}</a>".format(
+                **locals())
 
----------------
Formatting-only change.


================
Comment at: utils/opt-viewer/opt-viewer.py:346
 
-        file_remarks.setdefault(remark.File, dict()).setdefault(remark.Line, []).append(remark)
+    def merge_dicts(dicts):
+        merged = defaultdict(functools.partial(defaultdict, list))
----------------
merge_file_remark_dicts


================
Comment at: utils/opt-viewer/opt-viewer.py:351
+            for k_, v_ in v.items():
+                merged[k][k_] += v_
 
----------------
k -> file
k_ -> line
v_ -> remarks


================
Comment at: utils/opt-viewer/opt-viewer.py:378-396
+def generate_report(all_remarks, output_dir):
+    if Remark.should_display_hotness():
+        sorted_remarks = sorted(all_remarks.itervalues(), key=lambda r: r.Hotness,
+            reverse=True)
+    else:
+        sorted_remarks = sorted(all_remarks.itervalues(), key=lambda r: (
+                r.File, r.Line, r.Column))
----------------
Since you're passing all_remarks, it's confusing that you're not passing file_remarks.


Repository:
  rL LLVM

https://reviews.llvm.org/D26967