[PATCH] D67158: [ARM] Begin adding IR intrinsics for MVE instructions.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:312
+// Integer vector types that don't treat signed and unsigned differently.
+def v16i8_info : MVEVectorVTInfo<v16i8, v16i1, 0b00, "i8",  ?>;
+def v8i16_info : MVEVectorVTInfo<v8i16, v8i1,  0b01, "i16", ?>;
----------------
simon_tatham wrote:
> dmgreen wrote:
> > Maybe call these MVEv16i8? Or v16i8_t or v16i8info or something?
> > 
> > Otherwise it looks very useful.
> I copied the ``v16i8_info`` naming system from the similar x86 case you pointed out to me. But I can call them something with "MVE" in the name if you prefer, sure.
Ah righteo. The underscore threw me off. Whatever you think is best. MVE might not be the worst idea, just in case someone decides to do something similar in NEON.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-intrinsics/vaddq.ll:24
+
+define arm_aapcs_vfpcc <16 x i8> @test_vaddq_m_s8(<16 x i8> %inactive, <16 x i8> %a, <16 x i8> %b, i16 zeroext %p) {
+; CHECK-LABEL: test_vaddq_m_s8:
----------------
simon_tatham wrote:
> dmgreen wrote:
> > dmgreen wrote:
> > > For the rest of the tests, at least for codegen we have tried to fill in all the combinations for type and operations (at least the legal types). It can be useful for making sure nothing is missed (here or in the future when some refactoring happens).
> > > 
> > > Whether you want to do the same thing here is up to you, or whether you think that having interesting combinations is enough (adds with v16i8, subs with v4f32 for example).
> > This could probably do with a reply, one way or the other.
> > 
> > I was previously in the "don't mind either way" camp, now I feel more in the "why not just add them" camp, unless there is some reason not to.
> I had intended to stop at "interesting subset of combinations", along the lines of one of each operation, and one of each type, but not the full cross product unless absolutely necessary.
> 
> (There are about 2000 of these to come in future work, so at some point adding a test for every single one won't deserve the word "just" any more!)
The add int and add float are two different instructions, same for sub, so it probably best to make sure we have at least one of each of those combinations.

I was looking for prior art of only adding subsets of the combinations for codegen, I don't immediately see anywhere where we have done that. I'm not too worried about this set of tests not catching problems now. But in the future as things are refactored (in ways that might be difficult to predict), it might be expected that the test coverage is more complete and lead to things getting missed.

I'd suggest we at least fill in the combinations for add and sub. Later on when we get to vrmlsldavhax it probably wont be as important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67158