This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Add a test case to make sure !prof is preserved when one instruction CSE'ed another.
ClosedPublic

Authored by mingmingl on Apr 27 2023, 5:49 PM.

Details

Summary
  • This should have been be a part of D148877. Before that patch, !prof is not added to known-id-set, and turns out unknown types of metadata are dropped in the implementation.
    • This test is mainly added to make sure there won't be regressions for this kind of pattern. The pattern is observed it in application code; looks like the result of indirect call is used as function arguments initially; after the function is inlined load-after-store CSE opportunity is exposed.

Diff Detail

Event Timeline

mingmingl created this revision.Apr 27 2023, 5:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2023, 5:49 PM
mingmingl requested review of this revision.Apr 27 2023, 5:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2023, 5:49 PM
mingmingl edited the summary of this revision. (Show Details)Apr 27 2023, 5:50 PM
mingmingl added reviewers: davidxl, tejohnson.
mingmingl retitled this revision from [PGO] Preserve the profile/llvm-unstable/no-key-annotate-fdo/fdo_compile.sh 2>/tmp/verbose.txt metadata if one instructions folds the other away to [PGO] Preserve the !prof metadata if one instructions folds the other away.
mingmingl updated this revision to Diff 517763.Apr 27 2023, 5:57 PM
mingmingl retitled this revision from [PGO] Preserve the !prof metadata if one instructions folds the other away to [NFC] Add a test case to make sure EarlyCSE preserves !prof when `DoesKMmove` is false..
mingmingl edited the summary of this revision. (Show Details)

Can you expand the summary to explain why it is being dropped, and conceptually what the fix is doing? I'm not familiar with DoesKMove and what the 2 cases are here.

llvm/test/Transforms/EarlyCSE/metadata.ll
2

Add a comment at the top about what this test is testing.

22

What was being CSE'd here that caused it to get dropped?

Confused - there was a code change here previously - is it no longer needed?

looks like the wrong test case is uploaded? Should it be CSE of direct calls with "branch_weight" profile instead of indirect calls (indirect calls are unlikely to be CSEed without points to information).

mingmingl retitled this revision from [NFC] Add a test case to make sure EarlyCSE preserves !prof when `DoesKMmove` is false. to [NFC] Add a test case to make sure !prof is preserved when one instruction CSE'ed another..Apr 28 2023, 9:53 AM
mingmingl edited the summary of this revision. (Show Details)
mingmingl updated this revision to Diff 517970.Apr 28 2023, 9:57 AM
mingmingl marked 2 inline comments as done.

add a comment about what's being tested.

mingmingl added a comment.EditedApr 28 2023, 9:57 AM

Confused - there was a code change here previously - is it no longer needed?

Yes the code change is no longer needed. I initially thought K's metadata needs to be explicitly reset to be preserved in combineMetadata when KDominatesJ is false (or DoesKMove is false), but realized it shouldn't be the case, so reverted the code change (to explicitly reset metadata again)

looks like the wrong test case is uploaded? Should it be CSE of direct calls with "branch_weight" profile instead of indirect calls (indirect calls are unlikely to be CSEed without points to information).

Here the indirect call result CSE'ed a load-after-store instruction, rather than two indirect call instructions are CSE'ed to one

llvm/test/Transforms/EarlyCSE/metadata.ll
22

Here the optimizer finds %foo should have the same value as %call1 so it CSE'ed the load operation, and combines metadata of %call1 = tail call i32 %0(ptr %call, i32 %a, i32 %b), !prof !0 and %foo = load i32, ptr %x, align 4.

Before D148877, !prof is not added to known-id-set, and turns out unknown types of metadata are dropped in the implementation.

This test is mainly added to make sure there won't be regressions for this kind of pattern. The pattern is observed it in application code; looks like the result of indirect call is used as function arguments initially; after the function is inlined load-after-store is exposed (I didn't try to get a c++ repro since the IR is a little complex)

With 'opt -passes=early-cse -debug metadata.ll' debug log gives
EarlyCSE CSE LOAD: %foo = load i32, ptr %x, align 4 to: store i32 %call1, ptr %x, align 4 (related source code)

Confused - there was a code change here previously - is it no longer needed?

'llvm/lib/Transforms/Utils/Local.cpp' in https://reviews.llvm.org/D149387 is the original code change (that is not needed)

makes sense.

davidxl accepted this revision.Apr 28 2023, 11:04 AM
This revision is now accepted and ready to land.Apr 28 2023, 11:04 AM