[PATCH] D113289: LICM: Hoist LOAD without STORE

Fri Nov 26 02:45:00 PST 2021

djtodoro added a comment.

In D113289#3152152 <https://reviews.llvm.org/D113289#3152152>, @reames wrote:

> Heading in the right direction.
>
> In addition to the inline comments, please update and simplify the submission comment.  The comment should not explain the motivating workload (except maybe as a link to a bug), and should instead focus on explaining the effect of the transform.  i.e. use a more minimal example, and explain *why* we're implementing the transform.

It makes sense to me.

================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1955
+
+  bool shouldDelete(Instruction *I) const override {
+    if (const auto *SI = dyn_cast<StoreInst>(I)) {
----------------
reames wrote:
> The whole preserveduses mechanism seems like overkill.  The preserveuses set should simply be every store in the use set when CanInstertStoreInExistingBlocks is false.  If it's not, I'm missing something and we probably need some comments/tests.
Yes... I've had some tests failing due to some other reason, but this shouldn't have been included. Thanks.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113289/new/

https://reviews.llvm.org/D113289