FW: [PATCH] D16353: [mips] MIPS32R6 compact branch support

[Simon Dardis via llvm-commits]

Adding Daniel to this thread.

> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf
> Of Simon Dardis via llvm-commits
> Sent: 27 January 2016 13:39
> To: Vasileios Kalintiris
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D16353: [mips] MIPS32R6 compact branch support
> 
> sdardis added inline comments.
> 
> ================
> Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:549-613
> @@ -566,1 +548,67 @@
> 
> +// The forbidden slot is the instruction immediately following a
> +compact // branch. A forbidden slot hazard occurs when a compact branch
> +instruction // is executed and the adjacent instruction in memory is a
> +control transfer // instruction such as a branch or jump, ERET, ERETNC,
> +DERET, WAIT and // PAUSE.
> +//
> +// In such cases, the processor is required to signal a Reserved
> +Instruction // exception. Forbidden slot hazards are defined for MIPSR6, no
> microMIPS.
> +//
> +// There are three sources of forbidden slot hazards:
> +//
> +// A) Transforming a delay slot branch into compact branch.
> +// B) A previous pass has created a compact branch directly.
> +// C) Filling a delay slot using a backwards search when the instruction
> +//    moved was in a forbidden slot. This case will create hazards in already
> +//    processed code.
> +//
> +bool Filler::clearFSHazards(MachineFunction &F) {
> +  bool Changed = false;
> +  const MipsSubtarget &STI = F.getSubtarget<MipsSubtarget>();
> +  const MipsSEInstrInfo *TII =
> +      static_cast<const MipsSEInstrInfo *>(STI.getInstrInfo());
> +
> +  for (MachineFunction::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI) {
> +    for (Iter I = (*FI).begin(); I != (*FI).end(); ++I) {
> +      if (TII->HasForbiddenSlot(I)) {
> +        if (std::next(I) != (*FI).end() &&
> +            !TII->SafeInForbiddenSlot(&*std::next(I))) {
> +          BuildMI((*FI), std::next(I), I->getDebugLoc(), TII->get(Mips::NOP));
> +          Changed = true;
> +        } else {
> +          for (auto *Succ : (*FI).successors()) {
> +            if ((*FI).isLayoutSuccessor(Succ) &&
> +                (Succ->getFirstNonDebugInstr()) != Succ->end() &&
> +                !TII->SafeInForbiddenSlot((Succ->getFirstNonDebugInstr()))) {
> +              BuildMI(&(*FI), I->getDebugLoc(), TII->get(Mips::NOP));
> +              Changed = true;
> +            }
> +          }
> +        }
> +        if (Changed)
> +          MIBundleBuilder(*FI, I, std::next(I, 2));
> +      }
> +    }
> +  }
> +  return Changed;
> +}
> +
> +bool Filler::runOnMachineFunction(MachineFunction &F) {
> +  bool Changed = false;
> +  for (MachineFunction::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI)
> +    Changed |= runOnMachineBasicBlock(*FI);
> +
> +  // Make a second pass over the instructions to clear hazards.
> +  const MipsSubtarget &STI = F.getSubtarget<MipsSubtarget>();  if
> + (STI.hasMips32r6() && !STI.inMicroMipsMode())
> +    Changed |= clearFSHazards(F);
> +
> +  // This pass invalidates liveness information when it reorders  //
> + instructions to fill delay slot. Without this, -verify-machineinstrs
> + // will fail.
> +  if (Changed)
> +    F.getRegInfo().invalidateLiveness();
> +
> +  return Changed;
> +}
> ----------------
> vkalintiris wrote:
> > The (C) case will happen when filling the delay slot of a normal branch, ie.
> inside the MipsDelaySlotFiller pass. We can teach the "search-backwards"
> part of the delay slot filler (DSF) algorithm to handle this by checking the last
> instruction of the BB's layout predecessor. If there are other issues that
> conceptually belong to the DSF pass, then we should add them too by
> modifying the pass accordingly.
> >
> > I don't see how (B) can happen as we only emit compact branches at this
> pass for the time being. From my perspective, if a previous pass would like to
> add a compact-branch, then I can think of two options: (a) insert the compact
> branch only if the next instruction is not a CTI-hazard and leave the task of
> fixing the corner cases that happen during the filling of normal branches by
> the DSF, or (b) insert a NOP and just offload everything to the DSF. I can't
> think of any reason for a post-DSF pass to insert new/additional compact-
> branches or move non-CTI instructions from a forbidden slot.
> >
> > Even if we find ourselves constrained by having the DSF pass handling
> everything, then we can add a separate/final pass that fixes every case left
> over from previous passes. However, with this design we would achieve the
> minimum number of places were our code is wrong/invalid.
> >
> > If we'd want to be on the really safe side and make sure that we are aware
> of every possible case that we might haven't consider yet, then we can add a
> separate pass and enable it only for debug builds. It could assert upon finding
> a compact-branch that contains a CTI in its forbidden slot.
> >
> >  Even if we find ourselves constrained by having the DSF pass handling
> everything, then we can add a separate/final pass that fixes every case left
> over from previous passes. However, with this design we would achieve the
> minimum number of places were our code is wrong/invalid.
> 
> Rather than wiring FS hazard handling logic into the DSF, we could split it off
> into a separate pass and enable it after the delay slot filler, like the first diff.
> Thoughts dsanders? It keeps the implementation safer and simpler.
> 
> 
> http://reviews.llvm.org/D16353
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits