This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Support memory intrinsics
ClosedPublic

Authored by jdoerfert on Sep 6 2014, 3:31 AM.

Details

Summary
+ Allow and handle memcpy, memset and memmove intrinsics.
+ Allow to add lower/upper bounds to the access range of non-affine
  memory accesses (useful with a range analysis)

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert updated this revision to Diff 13365.Sep 6 2014, 3:31 AM
jdoerfert retitled this revision from to [Polly] Support memory intrinsics.
jdoerfert updated this object.
jdoerfert added reviewers: grosser, sebpop, simbuerg.
jdoerfert added subscribers: grosser, sebpop, simbuerg.
jdoerfert added a subscriber: Unknown Object (MLST).Sep 6 2014, 3:34 AM
jdoerfert updated this revision to Diff 13485.Sep 9 2014, 11:43 AM

Added early pointer check & adjusted based on the change of the misc intrinsics commit

grosser edited edge metadata.Jan 26 2016, 10:16 AM

Hi Johannes,

sorry I never got to review this patch. It looks small, self-contained and seems to be going into the right direction. Hence, I could most likely review this quickly if there is still interest. Does this patch still apply or does it require a rebase?

Again, 1.5 years old... I guess a rebase is needed.

jdoerfert updated this revision to Diff 47933.Feb 14 2016, 2:40 PM
jdoerfert edited edge metadata.

Rebase

etherzhhb added inline comments.
include/polly/Support/ScopHelper.h
169 ↗(On Diff #47933)

This doesn't make sense to me.

lib/Analysis/ScopDetection.cpp
492 ↗(On Diff #47933)

should it be getArgOperand (which is equivalent at the moment, but safer)?

500 ↗(On Diff #47933)

How about use getRawDest or getDest (this one strip the pointer-cast)?

lib/Analysis/ScopInfo.cpp
765 ↗(On Diff #47933)

simply isa<MemIntrinsic>(getAccessInstruction())?

2576 ↗(On Diff #47933)

why we relax this assertion?

etherzhhb added inline comments.Feb 16 2016, 1:03 AM
include/polly/Support/ScopHelper.h
169 ↗(On Diff #47933)

We may simply return nullptr here.

jdoerfert marked 5 inline comments as done.Feb 18 2016, 9:07 AM
jdoerfert added inline comments.
lib/Analysis/ScopInfo.cpp
2576 ↗(On Diff #47933)

Because the alias sets in the alias set tracker look differen when we also add memory intrinsics. This does not change the semantics of the algorithm though, it just refrlects that the alias groups can be small now such that we do not care.

jdoerfert updated this revision to Diff 48337.Feb 18 2016, 9:08 AM

Changed according to the comments. Thanks!

I don't see that the copied' size gets evaluated apart from checking that it's affine.

Does the following work as expected?

float A[128][128], B[128];
for (int i = 0; i < 128; i+=1)
  memcpy(A[i], B, 128*sizeof(float));
include/polly/Support/ScopHelper.h
224 ↗(On Diff #48337)

Maybe add a comment explaining this?

I guess this is taken from LoadInst/StoreInst implementation of isUnordered() with getOrdering() returning NotAtomic.

lib/Analysis/ScopDetection.cpp
38 ↗(On Diff #48337)

Can you be more specific: memset, memcpy, memmove are allowed?

lib/Analysis/ScopInfo.cpp
306 ↗(On Diff #48337)

LLVM coding style suggests to use auto only in specific cases. Eg. where the type already appears on the right side or the type is already abstracted away.

If you keep auto, the asterisk is superfluous.

test/ScopInfo/memcpy.ll
16 ↗(On Diff #48337)

Shouldn't this be a ReadAccess? This look like memcpy write two locations instead of reading one, writing the other.

test/ScopInfo/memmove.ll
16 ↗(On Diff #48337)

dito

jdoerfert updated this revision to Diff 48621.Feb 21 2016, 7:58 AM
jdoerfert marked 4 inline comments as done.

Updated according to comment. Thx.

I don't see that the copied' size gets evaluated apart from checking that it's affine.

Does the following work as expected?

float A[128][128], B[128];
for (int i = 0; i < 128; i+=1)
  memcpy(A[i], B, 128*sizeof(float));

The examples contain length values already. I think they work. The evaluation can be found in MemoryAccess::buildMemIntrinsicAccessRelation.

lib/Analysis/ScopInfo.cpp
306 ↗(On Diff #48337)

LLVM coding style suggests to use auto only in specific cases. Eg. where the type already appears on the right side or the type is already abstracted away.

Since the variable is called BB I guess the type is kinda obvious.

If you keep auto, the asterisk is superfluous.

But it tells you that this is a pointer. We had this discussion before and while ppl started to ignore it and use plain auto I still think it is a good idea to add * and & if appropriate.

This passes all LNT tests with an unprofitable build.

@etherzhhb, @Meinersbur: Are there still open problems/comments or can I commit this?

LGTM and I think this patch will be very useful.

include/polly/Support/ScopHelper.h
56 ↗(On Diff #48621)

load, store or memintrinsic ...

58 ↗(On Diff #48621)

isLoad(), isStore(), isMemIntrinsic() ...

lib/CodeGen/BlockGenerators.cpp
321–323 ↗(On Diff #48621)

Unrelated change?

This revision was automatically updated to reflect the committed changes.
jdoerfert marked 3 inline comments as done.