[PATCH] D117592: [ARM][AArch64] Introduce qrdmlah and qrdmlsh intrinsics

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

Thanks for working on this!

qrdmilh -> qrdmulh in the commit message.

I'm out of my depth math wise here so can't approve overall but perhaps you can clarify the aim here.

> Since it's introduction, the qrdmlah has been represented as a qrdmilh and a sadd_sat. This doesn't produce the same result for all input values though. This patch fixes that by introducing a qrdmlah (and qrdmlsh) intrinsic specifically for the vqrdmlah and sqrdmlah instructions. The old test cases will now produce a qrdmulh and sqadd, as expected.



- We had `qrdmlah` which output `qrdmulh` then `sadd_sat`. This didn't behave as expected for one set of inputs.
- You've introduced an intrinsic for `qrdmlah`. So now it'll output the one `vqrdmlah` instruction. This behaves as expected for all inputs.
- The existing test cases will now (have always?) output `qrdmulh` and `sqadd`.

That last bit I don't understand. Wouldn't those test cases output the new single instruction? I think I am misunderstanding which cases these are and what they test.



================
Comment at: llvm/test/CodeGen/ARM/neon-v8.1a.ll:31-32
+
+; The sadd intrinsics in this file previously transformed into sqrdmlah where they
+; shouldn't. They should produce sqrdmulh and sqadd.
+
----------------
Should this refer to `vqadd` etc instead?


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

https://reviews.llvm.org/D117592