[PATCH] [Polly] Support ModRef function behaviour

Tobias Grosser phabricator at grosser.es
Sun Sep 7 23:48:39 PDT 2014


Hey Johannes,

it is great you are pushing the limits of Polly further. Supporting memory modifying function calls is great.

Two questions:

- What is the reason no test cases are added? This is a new feature were it seems very valuable to have test cases. Did you encounter a certain problem when creating test cases? How did you test this code without test cases?

- It is not fully clear to me what kind of functions are now supported. Your commit message sounds to me as if the newly supported functions can only read/modify the data locations they directly point to? I had the impression they are not allowed to do something like:

void arrayoffset(float *A) {
  A[10] = 10;
}

or

void arraycast(float *A) {
  *((double*) A) = 10;
}

right? But the following is allowed:

void pointerdereference(float *A) {
  *A = 10;
}

Now looking at the actual code it seems all three of them are theoretically allowed. Is this right?

It may be helpful to state this more clearly in the commit message possibly giving some examples and explaining that we model this as an access to the full and unbounded data space of this array. Similarly, a comment in the code about what is supported here may also help readability.

Also, for the arraycast test case, it would be useful to give some reasoning why your choice of WSize is still correct.

Tobias

================
Comment at: lib/Analysis/TempScopInfo.cpp:290
@@ +289,3 @@
+      WriteBasePtr = dyn_cast<SCEVUnknown>(SE->getPointerBase(WriteAF));
+      assert(WriteBasePtr && "Could not find base pointer");
+      WriteAF = SE->getMinusSCEV(WriteAF, WriteBasePtr);
----------------
Should we not verify this already in the ScopDetection?

================
Comment at: lib/Analysis/TempScopInfo.cpp:295
@@ +294,3 @@
+      Accs.push_back(std::make_pair(
+          IRAccess(IRAccess::MAY_WRITE, WriteBasePtr->getValue(), WriteAF,
+                   WSize, false, Subscripts, Sizes, WriteAF, WriteUB),
----------------
Good that you choose MAY_WRITE here.

http://reviews.llvm.org/D5227






More information about the llvm-commits mailing list