Page MenuHomePhabricator

[llvm] Change DSOLocalEquivalent type if the underlying global value type changes
ClosedPublic

Authored by leonardchan on Mar 4 2021, 1:50 PM.

Details

Summary

We encountered an issue where LTO running on IR that used the DSOLocalEquivalent constant would result in bad codegen. The underlying issue was ValueMapper wasn't properly handling DSOLocalEquivalent, so this just adds the machinery for handling it. This code path is triggered by a fix to DSOLocalEquivalent::handleOperandChangeImpl where DSOLocalEquivalent could potentially not have the same type as its underlying GV.

This updates DSOLocalEquivalent::handleOperandChangeImpl to change the type if the GV type changes and handles this constant in ValueMapper.

Diff Detail

Event Timeline

leonardchan created this revision.Mar 4 2021, 1:50 PM
leonardchan requested review of this revision.Mar 4 2021, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 1:50 PM
phosek added a comment.Mar 5 2021, 2:39 PM

Is it feasible to cover this by a test?

llvm/lib/Transforms/Utils/ValueMapper.cpp
435

Super minor nit, I think that E would be more idiomatic here.

Is it feasible to cover this by a test?

Was going to add those and some more reviewers. Attempting to make a minimal reproducer for a test.

leonardchan marked an inline comment as done.
leonardchan edited the summary of this revision. (Show Details)
leonardchan added a subscriber: mehdi_amini.

@dexonsmith @mehdi_amini Would you be the right folks to add for reviews on ValueMapper?

@dexonsmith @mehdi_amini Would you be the right folks to add for reviews on ValueMapper?

@dexonsmith is the expert :)

phosek accepted this revision.Mar 8 2021, 5:32 PM

I'm not very familiar with this part of the codebase, but looking through the change, this looks reasonable to me.

This revision is now accepted and ready to land.Mar 8 2021, 5:32 PM