[PATCH] D31247: [Polly][DeLICM] Known knowledge.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 4 inline comments as done.
Meinersbur added a comment.

In https://reviews.llvm.org/D31247#708641, @grosser wrote:

> thank you for pushing this forward. This is again a pretty large patch, even though the kind of different changes are now a lot smaller. Still, for proper and timely review, it would really help to get these patches smaller and upstream as much as possible independently. I did a first round over this patch, and trying to understand which parts are needed to get the tests working that you contributed. It seems indeed that e.g. VirtualUse is not really needed


See inline comment.

> and to my surprise also the computeKnownFromWrite / computeKnownFromRead functions do not seem to be needed.

That was my laziness by making minimal changes to the reduction.ll test case to make it work. By doing this, only a particular kind of conflict occurs, namely conflicts with previously mapped values. This is not typical for GVE PRE-optimized code.  I'll come up with better ones with the next update.

Michael



================
Comment at: include/polly/Support/VirtualInstruction.h:37
+  enum UseType {
+    // An llvm::Constant.
+    Constant,
----------------
grosser wrote:
> An -> A
It is pronounced "El El Vee Em", hence the article is correct.

Also compare
https://www.google.com/search?q="An+LLVM"
vs
https://www.google.com/search?q="A+LLVM"


================
Comment at: lib/Transform/DeLICM.cpp:1236
+    switch (VUse.getType()) {
+    case VirtualUse::Constant:
+    case VirtualUse::ReadOnly: {
----------------
grosser wrote:
> The only case currently tested seems to be VirtualUse::Constant. If I add a "break" at the beginning of each other case, all tests still pass as expected. Maybe we can first introduce the constant case, without virtual use and then look again at the virtual use?
You may have missed that the "VirtualUse::InterValue" case breaks as well because it is the longest and "main" case, to avoid handling it in a switch case. All other cases return, that is, for the two test cases we need to distinguish at least these two and if its none of them (to return "value unknown"), makes three cases.

The other cases are not that interesting, so no test case for those yet. It doesn't come up because the test cases were crafted that way. For instance, VirtualUse::IntraValue never occurs because there are never two instructions depending on each other in a single basic block. The same computational pattern is employed with only one instruction, so the test case uses the smaller one.


https://reviews.llvm.org/D31247