This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Re-calculate scoped AA metadata when merging stores.
ClosedPublic

Authored by hliao on May 19 2021, 7:02 PM.

Diff Detail

Event Timeline

hliao created this revision.May 19 2021, 7:02 PM
hliao requested review of this revision.May 19 2021, 7:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 7:02 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16973

(And move the merge logic to IR/Metadata.h)

Also note that I am not sure if it is safe to merge the TBAA part in this way. (Especially when using new struct path tbaa)

hliao updated this revision to Diff 357323.Jul 8 2021, 12:35 PM

Move the AAMDNodes merging into a common place.

hliao marked an inline comment as done.Jul 8 2021, 1:05 PM

Maybe also a testcase using new struct path tbaa would be handy ?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16986

You are merging aa information of adjacent stores here. For the pure 'struct path tbaa' this should be fine. For the 'new' struct path tbaa, there is a problem, as that one also tracks the offset/size of the the tbaa tag information. For that reason, I think that a (new) 'mergeAdjacent' is better, as that one can throw away the tbaa info (or adapt the offset/size when compatible) when in 'new struct path tbaa' mode.

hliao updated this revision to Diff 373680.Sep 20 2021, 11:57 AM

Rebase after metadata interfaces are already revised in D109852

hliao updated this revision to Diff 373695.Sep 20 2021, 12:43 PM

Add concat to replace merge, which overlapping locations are expected.

hliao marked an inline comment as done.Sep 20 2021, 12:44 PM

LGTM. Just some small nits.

llvm/test/CodeGen/AArch64/merge-scoped-aa-store.ll
15–16
  • Rename this to 'blam0'. That makes it impossible for the MIR-LABEL to match 'blam1'.
  • also check for the 'LDRdui (including the metadata) ? (It's not the explicit goal of the test as such, but imho it makes sense to ensure that the scopes are still matching.)
36

Also here, I think it makes sense to add a check for LDRdui ?

This revision is now accepted and ready to land.Sep 21 2021, 1:26 AM
This revision was landed with ongoing or failed builds.Sep 21 2021, 8:50 AM
This revision was automatically updated to reflect the committed changes.
hliao marked 2 inline comments as done.Sep 21 2021, 8:50 AM