[PATCH] D104445: [SimplifyCFGPass] Tail-merging function-terminating blocks

Mon Jun 21 12:54:44 PDT 2021

rnk added a comment.

In D104445#2827627 <https://reviews.llvm.org/D104445#2827627>, @lebedev.ri wrote:

> Thank you for taking a look.
> Thinking about it, this change should be split up into NFC refactor,
> removal of "block must be empty" check,
> and several patches to enable each terminator opcode.

Sounds good, I am supportive of the long term direction.

> In D104445#2827485 <https://reviews.llvm.org/D104445#2827485>, @rnk wrote:
>
>> - it removes debug info
>
> This change certainly doesn't remove debuginfo, and i'm not sure if the generic sinking code is debuginfo-lossy, i

I don't see how it is possible for this transform to preserve source location information. See this example, where the call to `foo` ends up with a "line 0" location, presumably because of simplifycfg:
https://gcc.godbolt.org/z/foqfT7aP4
We don't have a way to represent a "merged" source location, despite the name of `DILocation::getMergedLocation`.

>> - it makes it harder to form regions later in the backend
>>
>> GPU targets, wasm, and Windows EH preparation may need to undo this type of region-breaking transform late in the backend, and if they do, the only thing this transform is doing is throwing away debug info.
>
> Yeah, no idea what any of this means, so i can't really comment on this point.

I think the easiest and most relevant example is WebAssembly (wasm). This virtual target does not have anything like `goto`, so all control flow must be restructured into standard high level control flow constructs. I believe this is mostly implemented in this pass:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp
Maybe the paper abstract describes it better:
https://dl.acm.org/doi/10.1145/2048147.2048224

GPU targets have similar requirements, but GPU programs don't usually contain assertions, so this is less interesting.

Windows exception handling (WinEH) has similar requirements. This is the code that clones blocks to ensure that every block is only reachable from one function or funclet entry block:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/WinEHPrepare.cpp#L744

I think what I'm saying can be summarized as "the middle end shouldn't perform transforms which the backend must heroically undo", and there needs to be some kind of TTI hook to control this.

>> If you limit this change to return blocks, that should completely address most regional concerns.
>
> By "return blocks", you literally mean blocks that end with `ret`, but neither `resume` nor `unreachable`, correct?

Yes.

>> I would also like to see this change for noreturn / unreachable blocks, but I think it's a longer path from here to there.
>
> Could you please be more specific about that path? I think i'm most interested in `resume`, actually.

I think we could extend this to `resume` without much issue. The DwarfEHPrepare pass already rewrites them into branches to a single block that calls `_Unwind_Resume`.

In D104445#2829076 <https://reviews.llvm.org/D104445#2829076>, @lebedev.ri wrote:

> That being said, let's try this in smaller steps, D104598 <https://reviews.llvm.org/D104598>/D104597 <https://reviews.llvm.org/D104597> being the first one, handling only the `ret`.

Sounds good.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104445