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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
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:
> aeubanks wrote:
> > snehasish wrote:
> > > dhoekwater wrote:
> > > > 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?
> > > Seems fine in the interim while we wait for jump table specific fixes to land. 
> > 0.15% is a fairly large regression. do you mean that enabling function splitting on its own regresses compile times compared to not enabling it, or that this patch regresses function splitting builds by 0.15%?
> Enabling function splitting for an AArch64 target makes the build 0.15% slower than leaving it disabled. This patch has no impact on compile times of builds that don't use machine function splitting for AArch64.
> 
> I'll collect the impact of just this patch to see its individual impact.
I believe that builds which enable function splitting on AArch64 can break without this fix. So it seemed reasonable to me to submit a fix while we wait for a more optimized approach to land.

Also @aeubanks do we have some guidelines on compile time increases across configurations that we can refer to in the future? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157124