This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Propagate scoped AA metadata when lowering mem intrinsics.
ClosedPublic

Authored by hliao on May 10 2021, 8:34 PM.

Details

Summary
  • When memory intrinsics, such as memcpy, the attached scoped AA metadata is not passed down to the backend. As a result, the backend cannot schedule relevant memory operations around them following that hint. In this patch, SelectionDAG is enhanced to propagate that metadata (scoped AA only) when they are lowered into loads and stores.

Diff Detail

Event Timeline

hliao created this revision.May 10 2021, 8:34 PM
hliao requested review of this revision.May 10 2021, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2021, 8:34 PM
xbolva00 added inline comments.
llvm/test/CodeGen/AArch64/memcpy-scoped-aa.ll
1

Precommit new tests?

gchatelet added inline comments.May 11 2021, 2:48 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5838–5841

[nit] Maybe create a function to avoid repetition?

llvm/test/CodeGen/AArch64/memcpy-scoped-aa.ll
1

I agree that would be nice to have them pre-commited and reflecting the old behavior so we can see the effect of this patch.

gchatelet added inline comments.May 11 2021, 2:50 AM
llvm/test/CodeGen/AArch64/memcpy-scoped-aa.ll
61

You may want to add a test for @llvm.memcpy.inline.p1i8.p1i8.i64 as well.

hliao updated this revision to Diff 344406.May 11 2021, 8:00 AM

Add more tests and revise following the comment.

hliao updated this revision to Diff 346485.May 19 2021, 9:24 AM

Include tests examining the MIR to ensure scoped AA metadata are attached on loads/stores lowered from mem ops.

hliao updated this revision to Diff 346533.May 19 2021, 12:18 PM

Revise the AA metadata matching.

hliao added a comment.May 25 2021, 5:37 AM

PING for review

I added some remarks.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6436

IMHO, the expansion of the mem intrinsic should be responsible for only using the metadata that makes sense for it ?
(If not, we should have an assertion to check that the AAInfo is sane).

!alias.scope and !noalias make sense, with the meaning: this memory instruction might alias with '!alias.scope` scopes, and will never alias with !noalias scopes.
!tbaa does not make sense (I think), but !tbaa.struct does. The expansion might make use of it, or leave away.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5758

See above: the expansion itself should be responsible for taking the elements that it gets.
At this level, we might check that the tbaa is sane, but IMHO, that should then only check that !tbaa is null.

llvm/test/CodeGen/AArch64/memcpy-scoped-aa.ll
79

Can we also have the check for the other instructions (with scope info) at MIR level ?

hliao updated this revision to Diff 347705.May 25 2021, 9:22 AM

Move the AAInfo preparation into the code lowering memcpy/memmove/memset.

hliao added inline comments.May 25 2021, 9:25 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6436

That's reasonable. I just moved the preparation of that AAInfo into the lowering part from the builder. So far, scoped AA metadata are retained. But, TBAA ones are cleared so far. We may take advantage of tbaa.struct in the future to refine loads/stores generated.

This revision is now accepted and ready to land.May 25 2021, 11:27 AM
hliao updated this revision to Diff 347743.May 25 2021, 11:35 AM

Rebase on the pre-committed test D103106.

This revision was landed with ongoing or failed builds.May 25 2021, 11:43 AM
This revision was automatically updated to reflect the committed changes.