[PATCH] D136764: [MachineCombiner][RISCV] Add fmadd/fmsub/fnmsub instructions patterns

[Anton Sidorenko via Phabricator via llvm-commits]

asi-sc added a comment.

In D136764#3928934 <https://reviews.llvm.org/D136764#3928934>, @craig.topper wrote:

> Do you know why sin, cos and  exp, sqrt seems to take longer?

It's a good question, thanks. I should say that the math functions we call are unchanged, so it's definitely not the reason.  The caller site for exp/sqrt (N8) also matches between time measurements, so it regressed not because of any change in instructions. One idea I have is that the unrolled loop that executes exp/sqrt is better aligned in the baseline version (0x1d64 vs 0x1d86). What do you think, can this be the reason?

As for sin/cos (N5), machine combiner generates `fmadd` (leaving `fmul` as it has one more use) that results in one additional `fmv` for each loop iteration. The reason for this is the extended liverange of the `atan` return value.
Baseline

  call    atan at plt
  fmul.d  fs1, fa0, fs6
  ...
  fmv.d   fa0, fs0
  call    sincos at plt
  ...
  fadd.d  fa0, fs1, fs0
  ...

This patch

  call    atan at plt
  fmv.d   fs1, fa0           <--- we combined fmadd that uses fa0 but it is placed after sincos call. Move is required to extend fa0 liverange
  fmul.d  fs2, fa0, fs8
  ...
  fmv.d   fa0, fs0
  call    sincos at plt
  ...
  fmadd.d fa0, fs1, fs8, fs0
  ...

I do have a draft work that will improve machine combiner logic to deal with this problem. In my opinion, combining instructions that are separated by a call is a doubtful from performance point of view thing and we must do additional check in these situations. However, I don't see how we can fix it exactly in this patch. Do we agree that sometimes additional moves are inserted, their origin is clear, and the problem is likely to be addressed in further patches? I don't expect major performance issues caused by exactly this behavior and didn't observe any. Or maybe there are any suggestions on how to address it in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136764