[PATCH] D24716: [Polly] DeLICM/DePRE (WIP)

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 29 inline comments as done.
Meinersbur added inline comments.


================
Comment at: include/polly/ScopInfo.h:1037
+  /// @param Oneline Print a more dense representation without line-breaks.
+  void print(raw_ostream &OS, bool Oneline = false) const;
 
----------------
grosser wrote:
> This seems to be unused. Can we exclude it for the moment?
It is used, by `polly::operator<<`


================
Comment at: include/polly/ScopInfo.h:1051
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const MemoryAccess *MA);
+
----------------
grosser wrote:
> This seems unused. Can we exclude it for the moment?
It is used, by many `DEBUG(dbgs() << ...)` lines.

If you delete it, `<<` will use the `void*` overload, ie. printing the object's address.

I considered `llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const MemoryAccess &MA)`, but it risks undefined behaviour if trying to print a `NULL` object. With a pointer, we can just print `null`.


================
Comment at: lib/Analysis/ScopInfo.cpp:915
 
+llvm::raw_ostream &polly::operator<<(llvm::raw_ostream &OS,
+                                     const MemoryAccess &MA) {
----------------
grosser wrote:
> Meinersbur wrote:
> > The function returns a one-line description of a MemoryAccess. Useful eg. for
> > ```
> > DEBUG(dbgs() << "Read access: " << *RA);
> > DEBUG(dbgs() << "Write access: " << *WA);
> > ```
> Alright. I would suggest to make the actual formatting/printing of the MemoryAccess class and then only call this functionality from the operator
> overloading. Also, if we could find a way to introduce / test this as part of normal Polly, this would be great. Can we e.g. use this to print new access function when importing from jscop? As such, we could introduce this code as separate patch.
The intention was to have printing functions for helping debugging `DEBUG(...)` and explicitly not to use them for unit-testing so they can be changed to show the internal state without making unit tests fail. An example why I think it is not a good idea to unittest these is the `[Scalar: 1]` tag printed by MemoryAccess::print. Changing it to something else (eg. `[MK_Value]` required changing many unut tests).

Seeing that I was not consistent with this by myself, I am going to remove this part from the patch.


================
Comment at: lib/Analysis/ScopInfo.cpp:314
+    case MK_ExitPHI:
+      OS << " MK_Array";
+      return;
----------------
grosser wrote:
> Why do you call MK_ExitPHI "MK_Array"? This seems counterintuitive.
> 
> Also, the very information you print is already available as suffix in the array name. Adding this to the printing seems redundant. I am aware that we currently do not add _exitphi to the naming, but by adding this to the array name as well we would remove this inconsistency without printing redundant information.
Printing "MK_Array" was more like a bug.

Variable names can already a contain `__phi` suffix, creating some ambiguity. Ie. the info is not strictly redundant.

There is currently no suffix for `MK_Value` either. I would not change the names as this creates compatibility issues when importing jscop and all the unit tests. The internal categorization should not be externally visible.

If you disagree, could you propose a patch which adds the suffixes?


================
Comment at: lib/Support/GICHelper.cpp:282
+}
+
 void polly::foreachElt(NonowningIslPtr<isl_union_pw_aff> UPwAff,
----------------
grosser wrote:
> These functions seem to be independent of the patch set. With the new unit-test infrastructure you added, it seems easy to test them separately. E.g. we could just iterate over the object, add the individual elements to a vector, combine them again with 'union', and check if the resulting objects are identical?
> 
> If we already add unit tests, these changes could easily be committed independently.
> 
> [Adding code like this to Polly feels generally a little bit unsatisfying, as this could be expressed a lot better with C++ bindings. This is one of the reasons I really want to get the bindings soon, as writing code/tests for code that is soon to be replaced is somehow unsatisfying. As the bindings still will take some time, it is probably OK to still upstream this.]
For the C++ bindings, I'd prefer iterators over callback functions. This would required adding a equivalent to iterators (something that remembers a position in an ISL hashtable/list).


================
Comment at: lib/Transform/DeLICM.cpp:1791
+      }
+    }
+
----------------
grosser wrote:
> Any reason you do not use scop->getReads(), scop->getWrites(), scop->getMayWrites()?
> 
> One might claim that your code prevents us from scanning the scop four times for different kinds of accesses, but this is certainly not a large performance difference (if measurable at all
> given the later more costly computations). If we want to add such a specialization, I would prefer to add this after the initial delicm implementation as a well-benchmarked and consequently justified performance optimization.
`getReads()` etc. do also return scalar accesses, ie. would need to iterate over the result again to filter those out. `addArrayWriteAccess` also computes the ValInsts for which we'd also need to iterate over the accesses.

I could remove the `addArrayReadAccess()` part if `scop->getAccessesOfType()` was public.


================
Comment at: unittests/CMakeLists.txt:19
   endif()
-  target_link_libraries(${test_name} Polly LLVMCore LLVMSupport)
+  target_link_libraries(${test_name} Polly LLVMCore LLVMSupport LLVMipo)
 endfunction()
----------------
grosser wrote:
> Why is this change needed? It works for me even without.
When linking Polly, it includes the CodegenCleanup pass, which in turn references a lot of passes from LLVMipo.

Since the linker is allowed to remove unused symbols, apparently CodegenCleanup was not linked into the unittests. I get a link error for DeLICMTests (Release+Asserts build).

Another solution is to make Polly directly depend on LLVMipo (and LLVMCore and LLVMSupport) which is currently does only for `BUILD_SHARED_LIBS` builds.


https://reviews.llvm.org/D24716