[PATCH] D140530: [RISCV] Add integer scalar instructions to isAssociativeAndCommutative

Sun Dec 25 22:57:08 PST 2022

craig.topper added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1379
+  case RISCV::XOR:
+  case RISCV::MUL:
+  case RISCV::MULW:
----------------
HsiangKai wrote:
> asi-sc wrote:
> > One thing that I'm a little worried is the described in the spec possibility for microarchitecture fusion(7.1): 
> > ```
> > If both the high and low bits of the same product are required, then the recommended code sequence is: MULH[[S]U] rdh, rs1, rs2; MUL rdl, rs1, rs2 (source register specifiers must be in same order and rdh cannot be the same as rs1 or rs2). Microarchitectures can then fuse these into a single multiply operation instead of performing two separate multiplies.
> > ```
> > As I know, we do nothing to support this fusion, however we quite often generate the recommended sequence (I checked some binaries I have on hand, it's not thorough research). Also some mul/mulh pairs we can easily fix by a simple DAG mutation at the scheduling stage:
> > ```
> > %10:gpr = MULHU %9:gpr, %5:gpr
> > %11:gpr = MUL %9:gpr, %5:gpr
> > --->
> > %11:gpr = MUL %9:gpr, %5:gpr
> > %10:gpr = MULHU %9:gpr, %5:gpr
> > ```
> > 
> > Machine combiner aggressively changes operands of multiplications, so I expect it'll worsen code from the fusion point of view.
> > 
> > Actually, I not sure that it's a real problem and just want to ensure that we are OK with it.
> > 
> Thank you for pointing it out. It is a good point. I am not aware the possibilities of mulh/mul fusion in the microarchitecture design. It is hard to say which one is better unless we have more performance data. How about removing mul/mulw in this patch to be conservative?
I'm not sure we need to worry about fusion until the compiler supports the DAG mutation for such fusion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140530