[PATCH] D32027: [Polly][DeLICM] Use Known information when comparing Existing.Written and Proposed.Written. NFC.

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

The definition of NFC is a little blurry in general, I assume.

In the most strict sense, it is really just comment, formatting changes, and renamings. But yes, we use it sometimes also for functional changes, where the output is not expected to change. However, as this is a rather large change and even unit tests are effected, maybe it helps to clarify to add a small sentence:

"This change only affects unit tests, but no functional changes are expected on LLVM-IR, as no Known information is yet extracted and consequently this functionality is only triggered through unit tests."

> It is already tested by e.g.
> 
> EXPECT_TRUE(
> 
>   checkIsConflicting({"{ Dom[i] }", nullptr, "{}"}, {nullptr, "{}", "{}"}));

OK, I missed this one.

In general, this patch is clearly correct. I added some comments to give you some options, but please go ahead and choose what seems best for you. Most of my remarks are personal style comments only.



================
Comment at: lib/Transform/DeLICM.cpp:425
+  return Result;
+}
+
----------------
Meinersbur wrote:
> grosser wrote:
> > This could possibly be written simpler as
> > 
> > UMap = UMap.subtract_range(makeUnknown());
> > 
> > assuming we introduce a makeUnknown() function.
> This would turn a linear-time function into an exponential-worst-time function due to its use of the intLP solver. Also, `isl_map_subtract` unconditionally calls normalization functions such as `isl_basic_map_simplify` and `isl_map_compute_divs`.
We have so many exponential worst-case functions, one more likely does not matter. The question is if we expect problems in practice. I commonly avoid 'subtract' as it indeed can become expensive, but I would assume subtracting universe sets should be fast. If it is not, we should probably fix isl.

I would prefer to keep the code simpler, and then add optimizations where needed. Again, the code is not super complex either. So I don't feel strong regarding what you choose.


================
Comment at: lib/Transform/DeLICM.cpp:788
+                             .intersect(filterKnownValInst(Proposed.Written))
+                             .domain();
+
----------------
Meinersbur wrote:
> grosser wrote:
> > If you intersect before filtering, you just need to apply the filter once.
> The idea of calling `filterKnownValInst` early is that the unknown spaces do not need to be computed just to throw them away afterwards.
I generally prefer simplicity over performance optimizations, as long as we don't do anything clearly inefficient.
Now, the difference is not very large. I will trust here your choice.


https://reviews.llvm.org/D32027