[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation

Fri Jun 18 17:31:12 PDT 2021

modimo added a comment.

In D36850#2827176 <https://reviews.llvm.org/D36850#2827176>, @tejohnson wrote:

> Sorry for not being responsive. I've been out of office but will be back Monday.

No worries!

> I skimmed through your notes, thanks for all the stats, it looks like nounwind is a good direction. Regarding linkonce_odr, I would think you should be able to take the union of their attributes, since they should be interchangeable.

Looking into this more my example in `linkonce_functionattrs_comdat.ll` is UB given that it's violating language ODR with 2 functionally different definitions. I think under the langref we're safe to propagate off of any single copy of a linkonce_odr however practically speaking taking the union of all of them may be safer and also handle cases where different attributes appear due to de-refinement.

> There should not be a requirement to use the local copy for linkonce_odr. After propagation, wouldn't their attributes be the same (i.e. regardless of inlining, since the callee attributes should presumably propagate up into the callee)?

In the original approach without taking the union a different copy could propagate "norecurse" to the caller but the local definition could actually recurse. If this is inlined, we'll get a caller with the "norecurse" attribute incorrectly which in reality does recurse. Taking the conservative union fixes this issue.

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

https://reviews.llvm.org/D36850