[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);
+}
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:8391-8398
+  auto isInJumpTable = [&](const MachineJumpTableEntry &JTE) {
+    return llvm::find(JTE.MBBs, &MBB) != JTE.MBBs.end();
+  };
+  auto isJumpTableTarget = [&](const MachineBasicBlock &MBB) {
+    const MachineJumpTableInfo *MJTI = MBB.getParent()->getJumpTableInfo();
+    return MJTI != nullptr &&
+           llvm::any_of(MJTI->getJumpTables(), isInJumpTable);
----------------
dhoekwater wrote:
> mingmingl wrote:
> > Mostly from compile-time perspective,  could this be pre-computed per MachineFunction in machine-function-splitter pass for AArch64?
> > 
> > Something like:
> > 
> > ```
> > // precompute 'JumpTableTargets' for each machine function 
> > const auto& MJTI = F->getJumpTableInfo();
> > 
> > SmallSet<MachineBasicBlock*> JumpTableTargetsSet;
> > 
> > for (auto JT : MJTI->getJumpTables()) {
> >   for (auto* MBB : JT.MBBs) { 
> >     JumpTableTargetsSet.insert(MBB);
> >   }
> > }
> > ```
> > 
> > // As long as 'JumpTableTargetsSet' remains valid (i.e., not invalidated by any code transformations, like branch folding, etc)
> > // For each 'AArch64InstrInfo::isMBBSafeToSplitToCold' method call
> > ```
> > auto isJumpTableTarget = [&](const MachineBasicBlock& MBB) {
> >   return JumpTableTargets.count(MBB);
> > }
> > ```
> Yeah, that would definitely be more efficient (`O(n)` worst case complexity in the number of blocks instead of `O(n^2)`). 
> 
> However, I don't think this has a huge performance impact in real builds, and this logic only exists temporarily before I push changes that allow splitting jump table dispatch and targets. I'll collect a breakdown of how it impacts compile time.
I just timed the backend compile. Although I don't have per-pass numbers, enabling function splitting only increases the build time of a large binary by 0.15%.

@mingmingl @snehasish Thoughts on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157124