Page MenuHomePhabricator

Simplify codegen and debug info generation for block context parameters.
ClosedPublic

Authored by aprantl on Oct 25 2017, 2:24 PM.

Details

Summary

The exisiting code goes out of its way to put block parameters into an alloca at -O0 only, but then describes the function argument (instead of the alloca) with a dbg.declare and the value loaded from the alloca with a dbg.value. Describing a function argument with a dbg.declare is undocumented in the LLVM-CFE contract and broke after LLVM r642022.

This patch just generates the alloca unconditionally, the mem2reg pass will eliminate it at -O1 and up anyway and points the dbg.declare to the alloca as intended (which mem2reg will then correctly rewrite into a dbg.value).

rdar://problem/35043980

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Oct 25 2017, 2:24 PM
rjmccall edited edge metadata.Oct 26 2017, 10:52 AM

The original code doesn't make any sense; it looks like the indirection is backwards. We just built a debug variable corresponding to the block descriptor pointer parameter, so LocalAddr should be dbg.declare'd to be that variable and Arg should be dbg.value'd. It looks like the real fix in your patch is just getting that right rather than anything to do with the alloca.

I don't particularly care about eliminating the alloca, since there are plenty of other places where we emit allocas that we assume we can destroy, and a few easily-eliminated instructions in a scattered function isn't going to greatly contribute to -O build times. So I think this patch is fine.

The original code doesn't make any sense; it looks like the indirection is backwards. We just built a debug variable corresponding to the block descriptor pointer parameter, so LocalAddr should be dbg.declare'd to be that variable and Arg should be dbg.value'd. It looks like the real fix in your patch is just getting that right rather than anything to do with the alloca.

Correct. The alloca is just a simplification.

I don't particularly care about eliminating the alloca, since there are plenty of other places where we emit allocas that we assume we can destroy, and a few easily-eliminated instructions in a scattered function isn't going to greatly contribute to -O build times. So I think this patch is fine.

Thanks!

This revision was automatically updated to reflect the committed changes.