This is an archive of the discontinued LLVM Phabricator instance.

[PR52475] Ensure a correct chain in copies to/from hidden sret parameter
ClosedPublic

Authored by frasercrmck on Nov 30 2021, 6:12 AM.

Details

Summary

This patch fixes an issue during SelectionDAG construction. When the
target is unable to lower the function's return value, a hidden sret
parameter is created. It is initialized and copied to a stored variable
(DemoteRegister) with CopyToReg and is later fetched with
CopyFromReg. The bug is that the chains used for each copy are
inconsistent, and thus in rare cases the scheduler may issue them out of
order.

The fix is to ensure that the CopyFromReg uses the DAG root which is set
as the chain corresponding to the initial CopyToReg.

Fixes https://llvm.org/PR52475

Diff Detail

Event Timeline

frasercrmck created this revision.Nov 30 2021, 6:12 AM
frasercrmck requested review of this revision.Nov 30 2021, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 6:12 AM
  • adjust formatting
frasercrmck edited the summary of this revision. (Show Details)Nov 30 2021, 6:13 AM
frasercrmck added a reviewer: tlively.
frasercrmck added a subscriber: tlively.

@tlively I would appreciate your thoughts on the WebAssembly test changes

jrtc27 added a comment.EditedNov 30 2021, 6:16 AM

Presumably this only affects IR with wacky return types, because Clang inserts an explicit sret parameter in the IR it generates when the ABI needs it?

jrtc27 added inline comments.Nov 30 2021, 6:17 AM
llvm/test/CodeGen/RISCV/rvv/pr52475.ll
3

RV32?

  • add RV32 RUN line
frasercrmck marked an inline comment as done.Nov 30 2021, 6:25 AM

Presumably this only affects IR with wacky return types, because Clang inserts an explicit sret parameter in the IR it generates when the ABI needs it?

I wouldn't want to say definitively but yeah that's roughly how I see things. I don't know if that's a requirement (documented or otherwise) of clang or not, though.

llvm/test/CodeGen/RISCV/rvv/pr52475.ll
3

Good catch, thanks.

RKSimon edited the summary of this revision. (Show Details)Nov 30 2021, 8:33 AM

WebAssembly test changes lgtm 👍

craig.topper added inline comments.Nov 30 2021, 6:27 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1923–1924

Is there any difference if we use the local Chain variable here. I think getControlRoot should ensure DAG.getRoot is in sync with the Chain returned.

frasercrmck marked an inline comment as done.
  • Use existing Chain over Root. No test changes.
frasercrmck marked an inline comment as done.Dec 1 2021, 12:46 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1923–1924

Doesn't seem like there's any difference. getControlRoot looks right, as you say, and it feels better to me to use the local Chain variable. Thanks!

frasercrmck marked an inline comment as done.Dec 2 2021, 11:33 PM

I think this is ready to merge: there's approvals for the WebAssembly test changes, and I presume both RISC-V and X86 as well? Is anyone willing to give the final acceptance?

This revision is now accepted and ready to land.Dec 11 2021, 8:19 PM
This revision was landed with ongoing or failed builds.Dec 13 2021, 2:56 AM
This revision was automatically updated to reflect the committed changes.