[PATCH] D71826: [Coroutines] Remove corresponding phi values when apply simplifyTerminatorLeadingToRet

Fri Dec 27 11:21:05 PST 2019

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

Sorry for the wait here, @junparser! I wanted to send my own patches out before looking at this one 😛

This is really nice, thanks for the change! As per your test plan, I confirmed that with this patch, cppcoro issue https://github.com/lewissbaker/cppcoro/issues/109 no longer reproduces for me. The jump threading issue you describe must have been responsible for Clang hanging indefinitely when compiling that test file.

I had one nitpick related to your test, please address it or not at your own discretion. This patch LGTM.

================
Comment at: llvm/test/Transforms/Coroutines/coro-split-musttail1.ll:99
+declare i8* @g()
+declare i8* @h()
----------------
One nitpick: this test includes a lot of attribute group annotations, like `#3` and `#5`, but these attribute groups aren't defined. If I run `opt -verify` on this IR file, I get this, which defines 3 attribute groups: https://reviews.llvm.org/P8178

As far as I know there's no practical reason to remove these non-existent attribute group references, but as a personal preference, I'd prefer if you modified the test such that it either (a) doesn't use undefined attribute groups, or (b) defines the attribute groups it uses, for example by using the output from https://reviews.llvm.org/P8178. The reason I'd prefer this is because I think it makes the LLVM IR slightly more readable -- as a reader I won't go looking for a definition of `#3` that doesn't exist.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71826