[PATCH] D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken

[Xun Li via Phabricator via llvm-commits]

lxfind created this revision.
lxfind added a reviewer: aeubanks.
Herald added subscribers: hoy, modimo, wenlei, hiraditya.
lxfind requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This patch is to address https://bugs.llvm.org/show_bug.cgi?id=50610.
In computed goto pattern, there are usually a list of basic blocks that are all targets of indirectbr instruction, and each basic block also has address taken and stored in a variable.
CHR pass could potentially clone these basic blocks, which would generate a cloned version of the indirectbr and clonved version of all basic blocks in the list.
However these basic blocks will not have their addresses taken and stored anywhere. So latter SimplifyCFG pass will simply remove all tehse cloned basic blocks, resulting in incorrect code.
Although we may be able to be more precise and only disallow running CHR on scopes that contain BBs that fall into the computed goto pattern, I think that would be over-complex and this simple condition should be good enough
to disable it at function level when we see any BB has address taken.
Unfortunately I wasn't able to come up with a small reduced test case to trigger this. There is a full repro IR in the bug.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103867

Files:
  llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp


Index: llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
===================================================================
--- llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
+++ llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
@@ -437,6 +437,16 @@
 }
 
 static bool shouldApply(Function &F, ProfileSummaryInfo& PSI) {
+  // If any of the basic blocks have address taken, we should skip this pass
+  // because the cloned blocks by this pass will not have address taken, and
+  // hence they may be optimized out by SimplifyCFG if they are targets of
+  // indirect branches.
+  bool HasBBAddressTaken =
+      llvm::any_of(F.getBasicBlockList(),
+                   [](const BasicBlock &BB) { return BB.hasAddressTaken(); });
+  if (HasBBAddressTaken)
+    return false;
+
   if (ForceCHR)
     return true;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103867.350491.patch
Type: text/x-patch
Size: 855 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210608/b283ef12/attachment.bin>