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

[Daniel Hoekwater via Phabricator via llvm-commits]

dhoekwater marked 3 inline comments as done.
dhoekwater 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);
----------------
aeubanks wrote:
> aeubanks wrote:
> > snehasish wrote:
> > > 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!
> > ah ok that sounds reasonable
> + at nikic 
> I don't think we have anything official, but we do yell at people who noticeably regress compile times, especially if it shows up on https://llvm-compile-time-tracker.com/ as red. (although I believe that only tracks x86-64 for now)
> One 0.15% regression might be fine, but they add up over time so we'd really like to not introduce noticeable regressions when possible. We probably should have a document somewhere that talks about acceptable compile time tradeoffs.
Builds which enable function splitting on AArch64 without this fix can break unless they disable jump tables. For AArch64 MFS builds, compiling with this fix takes virtually the same time (//technically// 0.07% faster) as compiling with jump tables disabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157124