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

[qshanz via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:63
+
+  FusionKind kind() const { return Kd; }
+};
----------------
kbarton wrote:
> function names should be verb phrases (e.g., getKind()).
> https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
Done.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:79
+// Checking more for each fusion kind to see, if the FirstMI meets the
+// constraints of SecondMI according to fusion specification.
+static bool checkOpConstraints(FusionFeature::FusionKind Kd,
----------------
kbarton wrote:
> Could you document what the return type means?
Done


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:136
+  llvm_unreachable("All the cases should have been handled");
+  return true;
+}
----------------
kbarton wrote:
> From reading the code, it seems like returning true from this function means that they should be scheduled together.
> Thus, this function defaults to fusing instructions if it comes across something it doesn't understand (and asserts are not enabled). Would it not be better to return false here, and let the existing scheduling heuristics decide whether to make the instructions adjacent?
When the checkOpConstraints() is called, we have got the fusion pair(FirstMI and SecondMI are fusion pair according to opcode). What we need to do is to kick some situation that we are sure that, it is NOT a fusion pair according to the spec. So, we have to return true by default unless we have the reason(i.e. the switch case) to say, it is NOT a fusion pair, as this is the last check. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:139
+
+/// \brief Check if the instr pair, FirstMI and SecondMI, should be fused
+/// together. Given SecondMI, when FirstMI is unspecified, then check if
----------------
kbarton wrote:
> The preference is to not use \brief, and instead use a single sentence for the abstract and then follow-on details in a separate paragraph:
> 
> 
> > The first sentence (or a paragraph beginning with \brief) is used as an abstract. Try to use a single sentence as the \brief adds visual clutter. Put detailed discussion into separate paragraphs.
> 
> (https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments)
> 
> 
Good catch. Done.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:159
+
+  for (auto &Feature : FusionFeatures) {
+    // Early return if the feature is not supported.
----------------
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




================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:160
+  for (auto &Feature : FusionFeatures) {
+    // Early return if the feature is not supported.
+    if (!Feature.isSupported())
----------------
kbarton wrote:
> I don't consider this an early return, this is just skipping it if it is not supported. 
Good catch.


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:161
+    // Early return if the feature is not supported.
+    if (!Feature.isSupported())
+      continue;
----------------
kbarton wrote:
> Is it possible to check for this earlier (e.g., when creating the FusionFeatures structure) and if it is not supported simply do not add it into the structure?
We cannot, as we need to enable/disable each feature at runtime by option -mattr=[feature].


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

https://reviews.llvm.org/D70651