[PATCH] D70651: [Power8] Add the MacroFusion support for Power8

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:159
+
+  for (auto &Feature : FusionFeatures) {
+    // Early return if the feature is not supported.
----------------
steven.zhang wrote:
> kbarton wrote:
> > Unless I'm missing something, for each call to shouldScheduleAdjacent for a given pair of instructions, we are going to walk **all** features in the FusionFeatures list. Is that correct?
> > If that's the case, I think this could be problematic (N*M where N is the number of pairs we want to check and M is the number of fusion features we have). 
> > 
> > Is it possible to change the FusionFeatures list to use a map instead of an array, and use one of the instructions as a key into the map? 
> > 
> > Also, how often do we want to check if the SecondMI is fusable without specifying a FirstMI? Is this a common case or an exceptional case?
> > 
> You are completely correct and yes, we need N*M complexity as you point out. However, this is NOT a 1:1 map, because, one instruction could be fused by multiple fusion pair. i.e. ( addi + load, addi + addis). 
> 
> And in fact, it is a map between two groups. { inst1, inst2, inst3, ... } and { inst4, inst 6, ...}. Any instructions in group 1 is fused with any instructions in group2. And the FusionFeature is the data structure to map these two group.  And we have an array of these maps. So, we have to walk all the element to find out all the maps with the instruction as the key. It would be great if we can reduce the complexity with better data structure. 
> 
> For the case that without specifying the FirstMI, it is used to find the anchor instruction that we want to fuse. Then, MacroFusion will walk all the instructions pred that anchor instruction to check if they can be fused. It is not special case but the way that MacroFusion used to get the FirstMI.
> 
>   - List Item
> 
> 
I think it's feasible to build a radix tree instead of array of sets for better performance, which can be done by either using helper methods (like `defineRule(definePattern(...), definePattern(...))`) or using tblgen. But since the number of patterns here aren't so large, this looks fine to me.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:177
+      auto DepOpIdx = Feature.depOpIdx();
+      if (DepOpIdx.hasValue()) {
+        // Checking if the result of the FirstMI is the desired operand of the
----------------
I think we still need some comments here explaining what would be checked in the mutation if we provide a negative `depOpIdx`. It might confuse someone to think here we have NO checks on registers at all.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70651/new/

https://reviews.llvm.org/D70651