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

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1379
+  case RISCV::XOR:
+  case RISCV::MUL:
+  case RISCV::MULW:
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140530