Page MenuHomePhabricator

[coroutines] Fix spills of static array allocas
ClosedPublic

Authored by ben-clayton on May 1 2019, 4:26 AM.

Details

Summary

CoroFrame was not considering static array allocas, and was only ever reserving a single element in the coroutine frame.
This meant that stores to the non-zero'th element would corrupt later frame data.

Store static array allocas as field arrays in the coroutine frame.

Added test.

Diff Detail

Repository
rL LLVM

Event Timeline

ben-clayton created this revision.May 1 2019, 4:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 4:26 AM
capn awarded a token.May 1 2019, 6:42 PM
capn added a subscriber: capn.
Orlando added a subscriber: Orlando.May 2 2019, 3:18 AM

Hi, I added a few inline comments, mostly just nits on style.

lib/Transforms/Coroutines/CoroFrame.cpp
405 ↗(On Diff #197527)

count -> Count

507 ↗(On Diff #197527)

indices -> Indices

514 ↗(On Diff #197527)

count -> Count

test/Transforms/Coroutines/coro-frame-arrayalloca.ll
3 ↗(On Diff #197527)

Is this second RUN redundant?

Fixed capitalization of locals 'Count' and 'Indices', removed duplicate '; RUN:' line in test.

ben-clayton marked 4 inline comments as done.May 3 2019, 12:12 AM
ben-clayton added inline comments.
test/Transforms/Coroutines/coro-frame-arrayalloca.ll
3 ↗(On Diff #197527)

Good spot! Yes, removed.

GorNishanov accepted this revision.May 3 2019, 10:16 AM

LGTM! Thank you for doing the fix.
What is the original source that synthesized allocas with count?

I tried to reproduce it from c++ with char a[5], but it got mapped to llvm array types.

This revision is now accepted and ready to land.May 3 2019, 10:16 AM

Thank you GorNishanov.

What is the original source that synthesized allocas with count?

SwiftShader (a CPU implementation of OpenGL ES and Vulkan) which uses Reactor for JIT code generation of shaders and compute / vertex / fragment pipelines.
I'm adding coroutine support for handling SPIR-V memory barriers which bring all shader invocations up to the same barrier before continuing.
The change is here if you're curious to see the coroutine implementation.

I do not believe I have permissions to submit this. Please can you do the honours?

Thanks again,
Ben

I do not believe I have permissions to submit this. Please can you do the honours?

Gentle ping!

Another friendly ping.

This revision was automatically updated to reflect the committed changes.
lewissbaker added inline comments.
llvm/trunk/lib/Transforms/Coroutines/CoroFrame.cpp
429

Does this call need to be changed to Padder.addType(Types.back())?

I can imagine a case where Ty is char and we have a local variable of type char[3] with the next local variable having type int.

Couldn't this result in the Padding object thinking it needs to pad to the next int value by adding char[3] whereas it actually only needs to pad with char?