[PATCH] D157124: [CodeGen][AArch64] Don't split jump table basic blocks

[Daniel Hoekwater via Phabricator via llvm-commits]

dhoekwater added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:8421
+
+  return !containsJumpTableLookup(MBB) && !isJumpTableTarget(MBB);
+}
----------------
dhoekwater wrote:
> snehasish wrote:
> > IMO structuring the code with functors makes it hard to follow. Additionally, there are several cases where we could return false early instead which would lower the compile time in practice.
> > 
> > I would prefer the code to be structured as follows:
> > 
> > ```
> > bool AArch64InstrInfo::isMBBSafeToSplitToCold(
> >     const MachineBasicBlock &MBB) const {
> >   // Check if MBB is JumpTableTarget
> >   const MachineJumpTableInfo *MJTI = MBB.getParent()->getJumpTableInfo();
> >  bool isJumpTableTarget = MJTI != nullptr &&
> >            llvm::any_of(MJTI->getJumpTables(), 
> >       [](const MachineJumpTableEntry &JTE){ return  llvm::find(JTE.MBBs, &MBB) != JTE.MBBs.end()});
> > 
> >   if (isJumpTableTarget) return false;
> >   
> >   // Check if contains lookup
> >   for(const auto &MI : MBB) {
> >         switch (MI.getOpcode()) {
> >     case TargetOpcode::G_BRJT:
> >     case AArch64::JumpTableDest32:
> >     case AArch64::JumpTableDest16:
> >     case AArch64::JumpTableDest8:
> >       return false;
> >     default:
> >       continue;
> >     }
> >   }
> >   // All checks passed.
> >   return true;
> > }
> > ```
> > 
> > What do you think?
> That sounds pretty reasonable to me; it's definitely an easier read.
I refactored the function per your feedback, but I did rework and keep one lambda. It follows the common pattern of defining a lambda then using it in an STL library on the next line as its only use, which I think is pretty readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157124