[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Oh, de-refining is pretty nifty / evil. This patch has background:
https://reviews.llvm.org/D18634

Since 2016, the optimizer is not allowed to do IPA on functions that can be de-refined (such as `linkonce_odr` functions).

Here's a hypothetical problematic scenario for the optimizer:

- original IR for B has a `throw` somewhere
- function A invokes function B
- in this TU, B is optimized and removes exceptions, and gets marked `nounwind`
- function A leverages the `nounwind` to turn the `invoke` into a `call`
- function B is de-refined at link/load time: linker chooses a *different* function B which still has a `throw`
- "evil magic" happens (see the discussions around the patch linked above)
- a crash is introduced

At first blush, it sounds like this could only be a problem if the code has UB in it. However, read the above patch (and follow-ups, and related discussion) for a variety of examples of non-UB cases where IPA on de-refineable functions introduces crashes. I don't know for sure this could be a problem for `nounwind` specifically, but in general the LLVM optimizer doesn't look at attributes of de-refineable functions.

(Note that if you're doing LTO (like AutoFDO), this usually won't block optimization, since at LTO time there are very few de-refineable functions (most `linkonce_odr` functions are internalized, and not exported as weak). So if we added a `-cc1` flag to prefer "stable IR" over "frontend peepholes", it would make sense for `-flto` to imply it.)

On the other hand, the frontend knows the token sequence from the source language. It knows whether function `B` is inherently `nounwind` based on its ODR token sequence; in which case, it's safe to use the attribute for an IPA peephole.

----

BTW, I'm not personally against removing this peephole from the frontend (even without a flag), or limiting it somehow to cases where it doesn't make IR output unstable. I like the idea of stable IRGen output.

Nevertheless, it feels like removing IPA-based peepholes from Clang in the name of stable IRGen output is a change in direction, which might deserve a discussion in the forums rather than in a particular patch review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83906