This is an archive of the discontinued LLVM Phabricator instance.

[StackColoring] Don't merge slots with differing StackIDs
ClosedPublic

Authored by frasercrmck on May 16 2022, 8:56 AM.

Details

Summary

The documentation for this specifically mentions that this should not
happen. We could think about adding target hooks to permit it (and how
to merge IDs) in the future if that is desirable.

This specific test case was merging a scalable-vector slot into a
non-scalable one and dropping the notion of scalability, meaning we
failed to allocate enough stack space for the object.

Diff Detail

Event Timeline

frasercrmck created this revision.May 16 2022, 8:56 AM
frasercrmck requested review of this revision.May 16 2022, 8:56 AM
sdesmalen accepted this revision.May 16 2022, 1:33 PM
sdesmalen added a subscriber: sdesmalen.

We have a similar fix downstream and I actually thought we already upstreamed this. In any case, good fix!

llvm/lib/CodeGen/StackColoring.cpp
1322

minor nit: if you move this code two lines below, you can use FirstSlot and SecondSlot.

This revision is now accepted and ready to land.May 16 2022, 1:33 PM
arsenm accepted this revision.May 16 2022, 1:47 PM

I feel like I fixed this one before

MaskRay accepted this revision.May 16 2022, 3:54 PM

With sdesmalen's suggestion applied.

apply suggestion

This revision was landed with ongoing or failed builds.May 17 2022, 12:41 AM
This revision was automatically updated to reflect the committed changes.

I feel like I fixed this one before

I think that may have been in StackSlotColoring as opposed the StackColoring pass changed here :)

frasercrmck added inline comments.May 17 2022, 12:51 AM
llvm/lib/CodeGen/StackColoring.cpp
1322

Ach of course, thank you. And I'm glad you got in touch as I woke up this morning feeling guilty I hadn't notified any AArch64 people when this might (and indeed does) affect them :)

sdesmalen added inline comments.May 17 2022, 12:57 AM
llvm/lib/CodeGen/StackColoring.cpp
1322

No worries, I've got a Herald rule set up for patches related to scalable vectors, hence why I spotted it. I appreciate the (after)thought though :)