[PATCH] D70066: [MacroFusion] Limit the max fused number as 2 to reduce the dependency

Mon Dec 2 11:57:08 PST 2019

fhahn added a comment.

In D70066#1761217 <https://reviews.llvm.org/D70066#1761217>, @steven.zhang wrote:

> I have updated the patch for easy to review. Personally, passing the SUnit and add one helper function for target to check the fused SU number is also ok for me. The only concern is that, other target might not know that, they need to check it manually if they happens to add some fusion pattern that could be fused more than once. As the hardware only supports the back-2-back macro-fusion as far as I see now,  it makes sense to assume it in MacroFusion. And we can change it in the future if some target want it.

Right, that's why I originally suggested to pass the SU and add an assertions making sure that exactly 2 instructions are chained together to all targets (except AArch64 with `+fuse-arith-logic`).

I still think that's slightly preferable to avoid any unnecessary checks in release builds. The impact on compile-time will likely be very small, but the backend is already notorious for being slow. So if there are any unnecessary checks we can skip without too much effort, IMO we should try to do so. And in this case we also give the backend specific heuristics more flexibility. But if you think the benefit of not passing the SU trumps the compile-time concern, I think that would be OK as well.

================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:35

+namespace {
+
----------------
nit: no need to put the static functions into an anonymous namespace.

================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:179

+    // Only back-2-back macro fusion is supported now.
     const MachineInstr *DepMI = DepSU.getInstr();
----------------
I think it is not too clear what back-2-back means here. IIUC it means more that we do not chain more than 2 instructions together. In a chain of 3 instructions, all instructions are still back-to-back.

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

https://reviews.llvm.org/D70066