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

[Sebastian Pop via Phabricator via llvm-commits]

sebpop added a comment.

I will post a patch to fix the comments from @tejohnson.



================
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;
----------------
brzycki wrote:
> 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;
> ```
> Why is an Indirect branch considered a return?

This function is similar to blockEndsInUnreachable which is used to detect cold code for the forward propagation. The documentation of blockEndsInUnreachable says:

> Also consider a block that ends in an indirect branch to be a return block, since many targets use plain indirect branches to return.

I am not sure why the back propagation should stop on an instruction which is `I->isEHPad`.



================
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))
----------------
brzycki wrote:
> Should we consider adding a per-instruction side-effect check?
> ```
> if (I.mayHaveSideEffects())
>   return true;
> ```
I would say no because
```
  bool mayHaveSideEffects() const { return mayWriteToMemory() || mayThrow(); }
```
Code that stores to memory or that throws can be cold.



https://reviews.llvm.org/D52904