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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Looks mostly good to me. But maybe we should seperate a new NFC patch before this.
Also can we get some testcases?



================
Comment at: llvm/lib/Target/PowerPC/PPC.td:168
+def FeatureFusion : SubtargetFeature<"fusion", "HasFusion", "true",
+                                     "Target supports instruction fusion.">;
+def FeatureAddiLoadFusion : SubtargetFeature<"fuse-addi-load",
----------------
No `.` for others, why we need one here? :)


================
Comment at: llvm/lib/Target/PowerPC/PPC.td:255
+
+  list<SubtargetFeature> Power8InheritFeatureList =
+      !listconcat(Power7FeatureList, Power8SpecificFeatures);
----------------
I like the idea of separating the inheritable feature and sub-target specific feature.
However, I think this should be done in an NFC patch before this.

And it looks confusing to me about having `Power8SpecificFeatures` and `Power8OnlyFeature`, 
are P8 specific feature supposed to be P8 only?

Maybe we should align with X86, name them: `P8InheritableFeatures` , `P*AdditionalFeatures` , `P*SpecificFeatures`/`P*OnlyFeatures`? 


================
Comment at: llvm/lib/Target/PowerPC/PPCMacroFusion.cpp:20
+using namespace llvm;
+namespace {
+
----------------
Is it better to put this into PPC namespace instead of anonymous namespace?


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

https://reviews.llvm.org/D70651