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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

thanks for the very fast update. This was exactly what I was looking for. I have a couple of minor typos and final questions on the Knowledge and Zone description. Otherwise, I think the knowledge stuff is good to go.

(I believe this was the most difficult part of the patch. The remaining part is still a lot larger, but I believe most of it is well explained and also not technically too complicated).



================
Comment at: lib/Transform/DeLICM.cpp:47
+// assumed to always take place before writes, hence we can think of reads
+// taking place and the beginning of a timepoint and writes at their end. This
+// has the following consequences:
----------------
AT the

at THE end


================
Comment at: lib/Transform/DeLICM.cpp:50
+//
+// * A read at the timepoint that starts the live-range (i.e. a write at the
+//   same timepoint) reads the previous value. Hence, exclude the timepoint
----------------
e.g., a LOAD


================
Comment at: lib/Transform/DeLICM.cpp:51
+// * A read at the timepoint that starts the live-range (i.e. a write at the
+//   same timepoint) reads the previous value. Hence, exclude the timepoint
+//   starting the zone.
----------------
WE exclude ... starting the zone FROM THE LIVE-RANGE.


================
Comment at: lib/Transform/DeLICM.cpp:57
+//   include the timepoint starting the zone to determine whether they are
+//   conflicting.
+//
----------------
It is unclear to me why a write may overwrite a variable's value. Is it because it is a may write, due to the order of the writes in the statement, or because it may write an identical value (and consequently the write has no effect)?

Also, I am not sure why you mention "undefined behavior" here. In the context of C this term has a very strong meaning. Is this really what you mean here? Can you explain what exactly is undefined  and why?


================
Comment at: lib/Transform/DeLICM.cpp:60
+// * A read at the timepoint that ends the live-range reads the same variable.
+//   Hence, include the timepoint at the end of the zone to include that read
+//   into the live-range. Doing otherwise would be a contradiction of the
----------------
Hence, WE include


================
Comment at: lib/Transform/DeLICM.cpp:62
+//   into the live-range. Doing otherwise would be a contradiction of the
+//   live-range.
+//
----------------
What is a contradiction of a live-range. You can contradict statements that state something, but what does contradicting a live-range mean?


================
Comment at: lib/Transform/DeLICM.cpp:64
+//
+// * A write at the timepoint that ends the live-range start a new live-range.
+//   It must not be included to the live-rage of the previous definition.
----------------
starts


================
Comment at: lib/Transform/DeLICM.cpp:65
+// * A write at the timepoint that ends the live-range start a new live-range.
+//   It must not be included to the live-rage of the previous definition.
+//
----------------
IN the live-range


================
Comment at: lib/Transform/DeLICM.cpp:68
+// All combinations of reads and writes at the endpoints are possible, but most
+// of the time only the write->read (e.g. live-range from definition to last
+// use) and read->write (e.g. unused range from last use to overwrite) and
----------------
e.g.,


================
Comment at: lib/Transform/DeLICM.cpp:69
+// of the time only the write->read (e.g. live-range from definition to last
+// use) and read->write (e.g. unused range from last use to overwrite) and
+// combinations are interesting (half-open ranges). write->write zones might be
----------------
e.g.,


================
Comment at: lib/Transform/DeLICM.cpp:519
+        "This function is only prepared to learn occupied elements from That");
+    assert(!Occupied && "Adapting Occupied too not implemented");
+
----------------
I don't get the grammar of the sentence in the assert. Could you look at it again?


https://reviews.llvm.org/D24716