[ARM] Mark MVE loads/store as not having side effects

[dmgreen]

Summary

The hasSideEffect parameter is usually automatically inferred from instruction patters. For some of our MVE instructions, we do not have patterns though, such as for the pre/post inc loads and stores. This instead specifies the flag manually on the base MVE_VLDRSTR_base tablegen class, making sure we get this correct.

This can help with scheduling multiple loads more optimally. Here I've added a unittest as a more direct form of testing.

[samparker]

Why isn't this handled properly by the scheduling infrastructure instead? This sounds a bit like a hack...

[samparker]

In D73117#1833090, @samparker wrote:

Why isn't this handled properly by the scheduling infrastructure instead? This sounds a bit like a hack...

Sorry Dave, I misread the title! My post-coffee question is, why do we attach SideEffects to patterns and not instruction descriptions anyway?

[simon_tatham]

why do we attach SideEffects to patterns and not instruction descriptions anyway?

I think the point is that a lot of patterns naturally come with information that indicates whether they have various kinds of side effect. If you write a pattern that describes what your instruction does in terms of standard LLVM IR (or rather SDNodes, but the principle's the same), then LLVM can work out for itself (based on its knowledge of those SDNodes' semantics) whether the instruction can load memory, store memory, or do anything else potentially weird, and therefore the Tablegen backend propagates that information from the pattern into the instruction to save you having to write it down again in the instruction definition.

So you only need to manually fill in flags like this if Tablegen can't work it out automatically – if your instruction is selected by C++ isel code, or generated from a target-specific SDNode that Tablegen doesn't know enough about, or inserted post-isel by some totally other phase, etc.

We've been gradually adding isel patterns for a lot of MVE instructions over the past few months, and one of the effects has been that the hasSideEffects flags have been turning from ? into well defined values.

[dmgreen]

Yep, what Simon said. For normal non-post/pre inc MVE loads we already get this correct. The load is marked as not having sideeffects as it is inferred from the pattern in the same way as any other instruction would be. But for Pre/Post inc loads, we do not have patterns as they have two outputs, so are lowered via dag2dag.

[simon_tatham]

Anyway: should this not also apply to writeback gather/scatter instructions, like MVE_VLDRWU32_qi_pre? Those also have two outputs, so they're selected by C++ code, and as far as I can see their hasSideEffects flags are still stuck on ? at the moment.

[dmgreen]

Yes... Gather loads should probably be in here too. I was just worried about exactly what the mayLoad would mean for an instruction that can load multiple chunks of data from around memory. I think it should be fine though. And, um, I might have accidentally added them without adding the testing. MVE_VLDRSTR_base is the base class of both.

I've included the interleaving loads too VLDn/VSTn, and added tests for all of them.

The other option would be to mark all MVE instructions as not having side effects at the top level, in MVE_MI (other than VCTP, which is specifically marked as having side effects). That's more of a change than I would like to make right here though.

llvm/lib/Target/ARM/ARMInstrMVE.td

class MVE_vldst24_base<bit writeback, bit fourregs, bits<2> stage, bits<2> size,
   let Inst{12-9} = 0b1111;
   let Inst{8-7} = size;
   let Inst{6-5} = stage;
   let Inst{4-1} = 0b0000;
   let Inst{0} = fourregs;
 
   let mayLoad = load;
   let mayStore = !eq(load,0);
+  let hasSideEffects = 0;
 }
 
 // A parameter class used to encapsulate all the ways the writeback
 // variants of VLD20 and friends differ from the non-writeback ones.
 class MVE_vldst24_writeback<bit b, dag Oo, dag Io,
                             string sy="", string c="", string n=""> {
   bit writeback = b;
   dag Oops = Oo;
: MVE_p<oops, iops, NoItinerary, asm, suffix, ops, vpred_n, cstr, pattern> {
   let Inst{21} = W;
   let Inst{20} = dir.load;
   let Inst{15-13} = Qd{2-0};
   let Inst{12} = opc;
   let Inst{11-9} = 0b111;
 
   let mayLoad = dir.load;
   let mayStore = !eq(dir.load,0);
+  let hasSideEffects = 0;
   let validForTailPredication = 1;
 }
 
 // Contiguous load and store instructions. These come in two main
 // categories: same-size loads/stores in which 128 bits of vector
 // register is transferred to or from 128 bits of memory in the most
 // obvious way, and widening loads / narrowing stores, in which the
 // size of memory accessed is less than the size of a vector register,

llvm/test/CodeGen/Thumb2/LowOverheadLoops/mve-float-loops.ll

 ; CHECK-NEXT:    sub.w r6, r12, #4
 ; CHECK-NEXT:    mov r4, r0
 ; CHECK-NEXT:    add.w lr, r5, r6, lsr #2
 ; CHECK-NEXT:    mov r5, r1
 ; CHECK-NEXT:    mov r6, r2
 ; CHECK-NEXT:    dls lr, lr
 ; CHECK-NEXT:  .LBB8_4: @ %vector.body
 ; CHECK-NEXT:    @ =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    vldrh.u32 q0, [r5], #8
 ; CHECK-NEXT:    ldr.w r9, [r4]
 ; CHECK-NEXT:    ldr.w r8, [r4, #4]
-; CHECK-NEXT:    vldrh.u32 q0, [r5], #8
 ; CHECK-NEXT:    adds r4, #8
 ; CHECK-NEXT:    vmov r7, s0
 ; CHECK-NEXT:    vmov.16 q1[0], r7
 ; CHECK-NEXT:    vmov r7, s1
 ; CHECK-NEXT:    vmov.16 q1[1], r7
 ; CHECK-NEXT:    vmov r7, s2
 ; CHECK-NEXT:    vmov.16 q1[2], r7
 ; CHECK-NEXT:    vmov r7, s3

llvm/unittests/Target/ARM/MachineInstrTest.cpp

for (auto &Op : Desc.operands()) {
       bool Valid = (Flags & ARMII::ValidForTailPredication) != 0;
       ASSERT_EQ(IsValidTPOpcode(i), Valid)
                 << MII->getName(i)
                 << ": mismatched expectation for tail-predicated safety\n";
       break;
     }
   }
 }
+
+TEST(MachineInstr, HasSideEffects) {
+  using namespace ARM;
+  unsigned Opcodes[] = {
+      // MVE Loads/Stores
+      MVE_VLDRBS16,        MVE_VLDRBS16_post,   MVE_VLDRBS16_pre,
+      MVE_VLDRBS16_rq,     MVE_VLDRBS32,        MVE_VLDRBS32_post,
+      MVE_VLDRBS32_pre,    MVE_VLDRBS32_rq,     MVE_VLDRBU16,
+      MVE_VLDRBU16_post,   MVE_VLDRBU16_pre,    MVE_VLDRBU16_rq,
+      MVE_VLDRBU32,        MVE_VLDRBU32_post,   MVE_VLDRBU32_pre,
+      MVE_VLDRBU32_rq,     MVE_VLDRBU8,         MVE_VLDRBU8_post,
+      MVE_VLDRBU8_pre,     MVE_VLDRBU8_rq,      MVE_VLDRDU64_qi,
+      MVE_VLDRDU64_qi_pre, MVE_VLDRDU64_rq,     MVE_VLDRDU64_rq_u,
+      MVE_VLDRHS32,        MVE_VLDRHS32_post,   MVE_VLDRHS32_pre,
+      MVE_VLDRHS32_rq,     MVE_VLDRHS32_rq_u,   MVE_VLDRHU16,
+      MVE_VLDRHU16_post,   MVE_VLDRHU16_pre,    MVE_VLDRHU16_rq,
+      MVE_VLDRHU16_rq_u,   MVE_VLDRHU32,        MVE_VLDRHU32_post,
+      MVE_VLDRHU32_pre,    MVE_VLDRHU32_rq,     MVE_VLDRHU32_rq_u,
+      MVE_VLDRWU32,        MVE_VLDRWU32_post,   MVE_VLDRWU32_pre,
+      MVE_VLDRWU32_qi,     MVE_VLDRWU32_qi_pre, MVE_VLDRWU32_rq,
+      MVE_VLDRWU32_rq_u,   MVE_VLD20_16,        MVE_VLD20_16_wb,
+      MVE_VLD20_32,        MVE_VLD20_32_wb,     MVE_VLD20_8,
+      MVE_VLD20_8_wb,      MVE_VLD21_16,        MVE_VLD21_16_wb,
+      MVE_VLD21_32,        MVE_VLD21_32_wb,     MVE_VLD21_8,
+      MVE_VLD21_8_wb,      MVE_VLD40_16,        MVE_VLD40_16_wb,
+      MVE_VLD40_32,        MVE_VLD40_32_wb,     MVE_VLD40_8,
+      MVE_VLD40_8_wb,      MVE_VLD41_16,        MVE_VLD41_16_wb,
+      MVE_VLD41_32,        MVE_VLD41_32_wb,     MVE_VLD41_8,
+      MVE_VLD41_8_wb,      MVE_VLD42_16,        MVE_VLD42_16_wb,
+      MVE_VLD42_32,        MVE_VLD42_32_wb,     MVE_VLD42_8,
+      MVE_VLD42_8_wb,      MVE_VLD43_16,        MVE_VLD43_16_wb,
+      MVE_VLD43_32,        MVE_VLD43_32_wb,     MVE_VLD43_8,
+      MVE_VLD43_8_wb,      MVE_VSTRB16,         MVE_VSTRB16_post,
+      MVE_VSTRB16_pre,     MVE_VSTRB16_rq,      MVE_VSTRB32,
+      MVE_VSTRB32_post,    MVE_VSTRB32_pre,     MVE_VSTRB32_rq,
+      MVE_VSTRB8_rq,       MVE_VSTRBU8,         MVE_VSTRBU8_post,
+      MVE_VSTRBU8_pre,     MVE_VSTRD64_qi,      MVE_VSTRD64_qi_pre,
+      MVE_VSTRD64_rq,      MVE_VSTRD64_rq_u,    MVE_VSTRH16_rq,
+      MVE_VSTRH16_rq_u,    MVE_VSTRH32,         MVE_VSTRH32_post,
+      MVE_VSTRH32_pre,     MVE_VSTRH32_rq,      MVE_VSTRH32_rq_u,
+      MVE_VSTRHU16,        MVE_VSTRHU16_post,   MVE_VSTRHU16_pre,
+      MVE_VSTRW32_qi,      MVE_VSTRW32_qi_pre,  MVE_VSTRW32_rq,
+      MVE_VSTRW32_rq_u,    MVE_VSTRWU32,        MVE_VSTRWU32_post,
+      MVE_VSTRWU32_pre,    MVE_VST20_16,        MVE_VST20_16_wb,
+      MVE_VST20_32,        MVE_VST20_32_wb,     MVE_VST20_8,
+      MVE_VST20_8_wb,      MVE_VST21_16,        MVE_VST21_16_wb,
+      MVE_VST21_32,        MVE_VST21_32_wb,     MVE_VST21_8,
+      MVE_VST21_8_wb,      MVE_VST40_16,        MVE_VST40_16_wb,
+      MVE_VST40_32,        MVE_VST40_32_wb,     MVE_VST40_8,
+      MVE_VST40_8_wb,      MVE_VST41_16,        MVE_VST41_16_wb,
+      MVE_VST41_32,        MVE_VST41_32_wb,     MVE_VST41_8,
+      MVE_VST41_8_wb,      MVE_VST42_16,        MVE_VST42_16_wb,
+      MVE_VST42_32,        MVE_VST42_32_wb,     MVE_VST42_8,
+      MVE_VST42_8_wb,      MVE_VST43_16,        MVE_VST43_16_wb,
+      MVE_VST43_32,        MVE_VST43_32_wb,     MVE_VST43_8,
+      MVE_VST43_8_wb,
+  };
+
+  LLVMInitializeARMTargetInfo();
+  LLVMInitializeARMTarget();
+  LLVMInitializeARMTargetMC();
+
+  auto TT(Triple::normalize("thumbv8.1m.main-arm-none-eabi"));
+  std::string Error;
+  const Target *T = TargetRegistry::lookupTarget(TT, Error);
+  if (!T) {
+    dbgs() << Error;
+    return;
+  }
+
+  TargetOptions Options;
+  auto TM = std::unique_ptr<LLVMTargetMachine>(
+      static_cast<LLVMTargetMachine *>(T->createTargetMachine(
+          TT, "generic", "", Options, None, None, CodeGenOpt::Default)));
+  ARMSubtarget ST(TM->getTargetTriple(), TM->getTargetCPU(),
+                  TM->getTargetFeatureString(),
+                  *static_cast<const ARMBaseTargetMachine *>(TM.get()), false);
+  const ARMBaseInstrInfo *TII = ST.getInstrInfo();
+  auto MII = TM->getMCInstrInfo();
+
+  for (unsigned Op : Opcodes) {
+    const MCInstrDesc &Desc = TII->get(Op);
+    ASSERT_FALSE(Desc.hasUnmodeledSideEffects())
+        << MII->getName(Op) << " has unexpected side effects";
+  }
+}