[PATCH] D93305: [AMDGPU][GlobalISel] Transform (fadd (fmul x, y), z) -> (fma x, y, z)

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:112
   /// Query is legal on the target.
+  bool isLegal(const LegalityQuery &Query) const;
   bool isLegalOrBeforeLegalizer(const LegalityQuery &Query) const;
----------------
isLegal needs its own comment.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:333
 
+  /// Transform (fadd (fmul x, y), z) -> (fma x, y, z)
+  bool matchCombineFaddFmulToFmadOrFma(
----------------
Comment should mention fmad.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:334
+  /// Transform (fadd (fmul x, y), z) -> (fma x, y, z)
+  bool matchCombineFaddFmulToFmadOrFma(
+      MachineInstr &MI,
----------------
Capitalise them like "FAdd" and "FMul" and "FMA" and "FMad". There are akready examples of this in GlobalISel.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:550
       const unsigned MemSizeInBits);
+  bool isContractableFMUL(const MachineInstr &MI, bool AllowFusionGlobally);
 };
----------------
"FMul"


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:795
 
+  virtual bool enableAggressiveFMAFusion(LLT Ty) const {
+    switch (Ty.getSizeInBits()) {
----------------
Function needs a comment.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:798
+    case 16:
+      return enableAggressiveFMAFusion(EVT(MVT::f16));
+    case 32:
----------------
Do you need EVT() here? I thought MVT would implicitly convert to EVT.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:1114
 
+  bool isOperationLegalOrCustom(unsigned Op, const LLT Ty,
+                                bool LegalOnly = false) const {
----------------
Function needs a comment.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2693
 
+  virtual bool isFPExtFoldable(const MachineInstr &MI, unsigned Opcode,
+                               LLT DestTy, LLT SrcTy) const {
----------------
Function needs a comment.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2734
 
+  virtual bool isFMAFasterThanFMulAndFAdd(const MachineFunction &MF,
+                                          const LLT) const {
----------------
Function needs a comment.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2739
+
+  virtual bool isFMADLegal(const MachineInstr &MI, const LLT Ty) const {
+    assert((MI.getOpcode() == TargetOpcode::G_FADD ||
----------------
Function needs a comment.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:560
+    GIDefMatchData<"std::tuple<Register, Register, Register, unsigned>">;
+def combine_fadd_fmul_to_fma: GICombineRule<
+  (defs root:$root, combine_fadd_fmul_to_fma_info:$info),
----------------
"combine_fadd_fmul_to_fmad_or_fma"?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3575
 
+bool CombinerHelper::isContractableFMUL(const MachineInstr &MI,
+                                        bool AllowFusionGlobally) {
----------------
Function needs a comment.


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

https://reviews.llvm.org/D93305