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

[Kit Barton via Phabricator via llvm-commits]

kbarton requested changes to this revision.
kbarton added inline comments.
This revision now requires changes to proceed.


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


================
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,
----------------
Could you document what the return type means?


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:136
+  llvm_unreachable("All the cases should have been handled");
+  return true;
+}
----------------
>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?


================
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
----------------
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)




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



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


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:161
+    // Early return if the feature is not supported.
+    if (!Feature.isSupported())
+      continue;
----------------
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?


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

https://reviews.llvm.org/D70651