[PATCH] D76766: [ARM][LowOverheadLoops] DoubleWidthResult instructions canGenerateZeros

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

A few irrelevant nits inlined, but also one question about the test.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:530
+  // Check for instructions which can write into a larger element size.
+  const MCInstrDesc &MCID = MI.getDesc();
+  uint64_t Flags = MCID.TSFlags;
----------------
nit: this is absolutely fine of course, but you do have a little helper function just below this function for the similar RetainsPreviousHalfElement MI description. Perhaps a bit more consistent to add this as a helper too, and that would nicely cluster these helpers.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:539
   // FIXME: FP minus 0?
   //case ARM::MVE_VNEGf16:
   //case ARM::MVE_VNEGf32:
----------------
Nit: probably better to just remove these commented out cases, and mention these opcodes in the FIXME, so that also the FIXME becomes a bit more descriptive.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:541
   //case ARM::MVE_VNEGf32:
   case ARM::MVE_VMVN:
   case ARM::MVE_VORN:
----------------
Nit: now that we have more classes of instructions that don't respect the lane values, the double width instructions added above, can you comment what's "wrong" with the opcodes below.


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/vaddv.mir:754
 
+  define hidden i32 @illegal_vmull_non_zero(i16* %x, i16* %y, i16* %z, i32 %n) {
+  entry:
----------------
Do we need to test some other opcodes too that have been marked as DoubleWidth? Like MVE_VSHLL, VMOVL, VQDMULL? Or is this perhaps already tested elsewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76766