[PATCH] D52904: [hot-cold-split] fix static analysis of cold regions

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hi @sebpop, I have a few comments inline regarding the function determining side effect status.



================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:150
+  const TerminatorInst *I = BB.getTerminator();
+  if (isa<ReturnInst>(I) || isa<IndirectBrInst>(I) || isa<InvokeInst>(I))
+    return true;
----------------
Why is an Indirect branch considered a return? The instruction is required to branch to an address of a  label within the current function.

I am also wondering about the EH TIs: `catchswitch`, `resume`, `catchret`, `cleanupret`. Should we just pessimize all EH code with:
```
if (I->isEHPad())
  return true;
```


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:154
+  for (const Instruction &I : BB)
+    if (const CallInst *CI = dyn_cast<CallInst>(&I)) {
+      if (CI->hasFnAttr(Attribute::NoReturn))
----------------
Should we consider adding a per-instruction side-effect check?
```
if (I.mayHaveSideEffects())
  return true;
```


https://reviews.llvm.org/D52904