This is an archive of the discontinued LLVM Phabricator instance.

Keep track of ScopArrayInfo objects that model PHI node storage
ClosedPublic

Authored by grosser on Jul 27 2015, 11:53 PM.

Details

Summary

When translating PHI nodes into memory dependences during code generation we
require two kinds of memory. 'Normal memory' as for all scalar dependences and
'PHI node memory' to store the incoming values of the PHI node. With this
patch we now mark and track these two kinds of memories, which we previously
incorrectly marked as a single memory object.

Being aware of PHI node storage makes code generation easier, as we do not need
to guess what kind of storage a scalar reference requires. This simplifies the
code nicely.

Diff Detail

Repository
rL LLVM

Event Timeline

grosser updated this revision to Diff 30789.Jul 27 2015, 11:53 PM
grosser retitled this revision from to Keep track of ScopArrayInfo objects that model PHI node storage.
grosser updated this object.
grosser added a reviewer: jdoerfert.
grosser added subscribers: llvm-commits, pollydev.
grosser updated this revision to Diff 30804.Jul 28 2015, 5:04 AM

Also simplify some of the RegionInfo code

jdoerfert edited edge metadata.Jul 28 2015, 6:52 AM

Some comments but overall LGTM.

include/polly/ScopInfo.h
1053 ↗(On Diff #30789)

typo.

lib/Analysis/ScopInfo.cpp
357 ↗(On Diff #30789)

I would have used the suffix (last argument) of this call to encode the __phi but you don't have to.

std::string BasePtrName = getIslCompatibleName("MemRef_", BasePtr, IsPHI ? "__phi" : "");
1713 ↗(On Diff #30789)

Why not use a bool in the pair but introduce this 1/0 integer?

1721 ↗(On Diff #30789)

auto *

This revision was automatically updated to reflect the committed changes.

I addressed all your remarks, except:

Why not use a bool in the pair but introduce this 1/0 integer?

Because there is currently no DenseMapInfo available for bool and it is not 100% clear for me how to add this. However, I realized that I can even with char write just use IsPHI instead of IsPHI ? 1: 0. It would good to add such an implementation (which is super small), but I am not sure how to add thumbstones, empty keys and two boolean values, if only a bool can
be returned as key. As an implementation based on 'char' has almost the same properties, I did not spend more time to look into this.

Thanks again for your review,
Best Tobias