Page MenuHomePhabricator

[MemorySSA] Don't do addr phi translation if addr value is not a loop invariant
Changes PlannedPublic

Authored by StephenFan on Sep 6 2022, 7:44 PM.

Details

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > LLVM.Analysis/MemorySSA::phi-translation.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -aa-pipeline=basic-aa -passes='print<memoryssa>,verify<memoryssa>' -disable-output < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Analysis/MemorySSA/phi-translation.ll 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Analysis/MemorySSA/phi-translation.ll --check-prefixes=CHECK,NOLIMIT

Event Timeline

StephenFan created this revision.Sep 6 2022, 7:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 7:44 PM
StephenFan requested review of this revision.Sep 6 2022, 7:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 7:44 PM
nikic added a comment.Sep 7 2022, 1:11 AM

You need to update phi-translation.ll for this change. From a cursory look I think the results improve while still being correct.

llvm/include/llvm/Analysis/MemorySSA.h
1258

A slightly more accurate check would be if (Location.Size != LocationSize::beforeOrAfterPointer()). If the size is already unknown originally, then we don't need to bother with phi translation either.

nikic added a comment.Sep 7 2022, 1:24 AM

You need to update phi-translation.ll for this change. From a cursory look I think the results improve while still being correct.

This is because PerformedPhiTranslation no longer gets set. After looking a bit closer, I think this flag is a leftover from an old implementation. Submitted https://reviews.llvm.org/D133404 to remove.

Make check more accurate

This patch seems wrong. It will make the test fail if D134161 is applied.

StephenFan added inline comments.Sep 19 2022, 12:07 AM
llvm/include/llvm/Analysis/MemorySSA.h
1269

These codes here seem to make sense. But deleting them would not cause any tests to fail. I think it is necessary to add test cases to cover it.

StephenFan planned changes to this revision.Sep 19 2022, 12:07 AM