This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Ensure SCEV does not replace aliases with their aliasees
ClosedPublic

Authored by leonardchan on Feb 14 2023, 10:48 AM.

Details

Summary

Passes in general shouldn't replace an alias with the aliasee (see https://reviews.llvm.org/D66606). This can lead to situations where a linkonce_odr symbol (which could be interposable if lowered to weak linkage) can be replaced with a local aliasee which won't be interposable. SVEC does this when the function is invoked by FunctionPass Manager -> Loop Pass Manager -> Induction Variable Users in the codegen pipeline. This was found in hwasan instrumented code where a linonce_odr alias was replaced with its private aliasee.

This fixes the bug descriped at https://github.com/llvm/llvm-project/issues/60668.

Diff Detail

Event Timeline

leonardchan created this revision.Feb 14 2023, 10:48 AM
leonardchan requested review of this revision.Feb 14 2023, 10:48 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 14 2023, 10:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Lemme know if the clang regression test isn't necessary or should be in its own commit.

pcc requested changes to this revision.Feb 21 2023, 3:55 PM

Passes shouldn't be replacing aliases with aliasees in general, see D66606. The right fix is to fix SCEV to not do that.

This revision now requires changes to proceed.Feb 21 2023, 3:55 PM
leonardchan retitled this revision from [hwasan] Ensure hwasan aliases do not have ODR linkage to [hwasan] Ensure SCEV does not replace aliases with their aliasees.
leonardchan edited the summary of this revision. (Show Details)
leonardchan retitled this revision from [hwasan] Ensure SCEV does not replace aliases with their aliasees to [llvm] Ensure SCEV does not replace aliases with their aliasees.

Passes shouldn't be replacing aliases with aliasees in general, see D66606. The right fix is to fix SCEV to not do that.

Updated

LG

clang/test/CodeGenCXX/pr60668.cpp
1 ↗(On Diff #499349)

We try avoiding .cc to assembly test. Drop this file.

MaskRay added a comment.EditedFeb 22 2023, 1:40 PM

Full context at https://github.com/llvm/llvm-project/issues/60668.

Best to leave a simplified description in the summary so that a reader doesn't have to go to the external link and summarize the discussions themselves.
And give a link to https://reviews.llvm.org/D66606

This SCEV function is invoked by FunctionPass Manager -> Loop Pass Manager -> Induction Variable Users in the codegen pipeline.

MaskRay accepted this revision.Feb 22 2023, 1:41 PM
leonardchan marked an inline comment as done.
leonardchan edited the summary of this revision. (Show Details)Feb 22 2023, 4:45 PM

Full context at https://github.com/llvm/llvm-project/issues/60668.

Best to leave a simplified description in the summary so that a reader doesn't have to go to the external link and summarize the discussions themselves.
And give a link to https://reviews.llvm.org/D66606

This SCEV function is invoked by FunctionPass Manager -> Loop Pass Manager -> Induction Variable Users in the codegen pipeline.

Updated

MaskRay accepted this revision.Feb 22 2023, 4:54 PM

[llvm]

[SCEV] is a more common tag for ScalarEvolution patches.

leonardchan retitled this revision from [llvm] Ensure SCEV does not replace aliases with their aliasees to [SCEV] Ensure SCEV does not replace aliases with their aliasees.Feb 22 2023, 5:04 PM
pcc accepted this revision.Feb 22 2023, 5:33 PM

LGTM

This revision is now accepted and ready to land.Feb 22 2023, 5:33 PM

@reames let's continue the discussion here. What specific bits with the optimizer does this break? I'm not too familiar with SVEC. Prior to this, one thing I tried was instead just doing Ops.push_back(GA) and getSCEV(GA) but those seemed to result in infinite recursions last I checked probably because there's another pass attempting to resolve aliases.