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

[Anton Sidorenko via Phabricator via llvm-commits]

asi-sc added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1379
+  case RISCV::XOR:
+  case RISCV::MUL:
+  case RISCV::MULW:
----------------
craig.topper wrote:
> 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.
>From my expectations, reassociation of mul instructions that can be fused with mulh should be a quite rare case. Whereas reassociation of mul instructions happens often (I did some experiments previously). So, I agree with Craig but I'd ask you to add a comment about the possible impact of MachineCombiner on the fusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140530