[PATCH] D59872: [X86MacroFusion] Handle branch fusion (AMD CPUs).

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Looks good to me, but please wait a bit for @andreadb.



================
Comment at: llvm/lib/Target/X86/X86.td:347-357
 // Sandy Bridge and newer processors have many instructions that can be
 // fused with conditional branches and pass through the CPU as a single
 // operation.
 def FeatureMacroFusion
     : SubtargetFeature<"macrofusion", "HasMacroFusion", "true",
                  "Various instructions can be fused with conditional branches">;
 
----------------
courbet wrote:
> lebedev.ri wrote:
> > I think it would be better to swap these around?
> > `FeatureMacroFusion` is a superset of `FeatureBranchFusion`.
> > (maybe even inherit `FeatureBranchFusion` by `FeatureMacroFusion`, not sure it is possible here.)
> I'd rather not inherit because they are actually different (e.g. w.r.t. CMP). Also they have different subtle limitations that are not currently modeled (see Agner), but we might want to model in the future.
> 
Sounds good.


================
Comment at: llvm/lib/Target/X86/X86MacroFusion.cpp:23
 
-  enum {
-    FuseTest,
-    FuseCmp,
-    FuseInc
-  } FuseKind;
+enum class FirstKind { Test, Cmp, And, ALU, IncDec, Invalid };
 
----------------
`FirstInstrKind`?


================
Comment at: llvm/lib/Target/X86/X86MacroFusion.cpp:25
 
-  unsigned FirstOpcode = FirstMI
-                         ? FirstMI->getOpcode()
-                         : static_cast<unsigned>(X86::INSTRUCTION_LIST_END);
-  unsigned SecondOpcode = SecondMI.getOpcode();
+enum class SecondKind {
+  ELG,
----------------
`JumpKind`?


================
Comment at: llvm/test/CodeGen/X86/testb-je-fusion.ll:1-3
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-- -mattr=-macrofusion | FileCheck %s --check-prefix=NOFUSION
-; RUN: llc < %s -mtriple=x86_64-- -mattr=+macrofusion | FileCheck %s --check-prefix=MACROFUSION
----------------
In trunk, i think you will get slightly more meaningful diff with 
```
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc < %s -mtriple=x86_64-- -mattr=-macrofusion,-branchfusion | FileCheck %s --check-prefixes=NOFUSION
; RUN: llc < %s -mtriple=x86_64-- -mattr=-macrofusion,+branchfusion | FileCheck %s --check-prefixes=NOFUSION,BRANCHFUSION,BRANCHFUSIONONLY
; RUN: llc < %s -mtriple=x86_64-- -mattr=+macrofusion,-branchfusion | FileCheck %s --check-prefixes=NOFUSION,BRANCHFUSION,MACROFUSION
```
(drop `[+-]branchfusion` for trunk, add it back in patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59872