This is an archive of the discontinued LLVM Phabricator instance.

InstCombine: Preserve tbaa metadata when merging loads that are phi arguments
ClosedPublic

Authored by ahatanak on Sep 8 2015, 3:26 PM.

Details

Summary

This patch attaches the merged metadata to the load instruction that is newly created in InstCombiner::FoldPHIArgLoadIntoPHI.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 34272.Sep 8 2015, 3:26 PM
ahatanak retitled this revision from to InstCombine: Preserve tbaa metadata when merging loads that are phi arguments.
ahatanak updated this object.
ahatanak added a subscriber: llvm-commits.
reames added a subscriber: reames.Sep 17 2015, 9:55 AM
reames added inline comments.
lib/Transforms/InstCombine/InstCombinePHI.cpp
357

On first look, this seems like it's the wrong approach. You're mutating the metadata on an existing load to accumulate the intersection of all of the incoming loads. It's not clear to me that we want to be killing the metadata on the input load.

ahatanak updated this revision to Diff 35029.Sep 17 2015, 1:31 PM

I was counting on the fact that the incoming loads will eventually get eliminated as they all have one use, so mutating the metadata is safe. But it's probably better to take an approach that doesn't mutate the metadata of an existing load. In the new patch, the metadata is attached to the newly created load.

hfinkel added inline comments.
lib/Transforms/InstCombine/InstCombinePHI.cpp
355

Why are you preserving only TBAA? I think that you could also preserve here:

MD_alias_scope
MD_noalias
MD_invariant_load
MD_nonnull

using combineMetadata.

ahatanak added inline comments.Sep 18 2015, 1:04 PM
lib/Transforms/InstCombine/InstCombinePHI.cpp
355

Yes, I think you are right. I didn't look at the other metadata since tbaa was the only one that I had to fix for my use case.

Is there a reason you left out the other two (range and fpmath) that are combined in combineMetadata? It seems to me that they can be attached to load instructions and should be preserved too.

hfinkel added inline comments.Sep 18 2015, 3:37 PM
lib/Transforms/InstCombine/InstCombinePHI.cpp
355

I don't think that fpmath applies to loads, but range does, and yes, you should include that too.

ahatanak updated this revision to Diff 35316.Sep 21 2015, 3:37 PM

Preserve the following metadata in addition to tbaa:

MD_alias_scope
MD_noalias
MD_invariant_load
MD_nonnull
MD_range

hfinkel accepted this revision.Sep 23 2015, 10:33 AM
hfinkel added a reviewer: hfinkel.

LGTM.

This revision is now accepted and ready to land.Sep 23 2015, 10:33 AM
This revision was automatically updated to reflect the committed changes.