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


================
Comment at: llvm/test/CodeGen/AArch64/machine-function-splitter.ll:32-34
+cold1:                                            ; preds = %0
+  %3 = tail call i32 @bam()
+  ret i32 %3
----------------
mingmingl wrote:
> For my understanding, is `cold1` split out to `nosplit_jumptable.cold` before this patch?  
Yeah, `cold1` and `cold2` are split out to the cold section before this patch.


================
Comment at: llvm/test/CodeGen/AArch64/machine-function-splitter.ll:63
+!15 = !{!"function_section_prefix", !"hot"}
+!16 = !{!"branch_weights", i32 7000, i32 0}
+!17 = !{!"branch_weights", i32 0, i32 4000, i32 4000, i32 0, i32 1000}
----------------
mingmingl wrote:
> nit: if this is not used, maybe remove this?
Resolved in updated test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157124