[PATCH] D154205: [MachineLICM] Handle subloops

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:762
+      if (!Hoist(&MI, Preheader)) {
+        // We have failed to hoist MI to outmost loop's preheader. If MI is in
+        // subloop, try to hoist it to subloop's preheader.
----------------
dmgreen wrote:
> outmost -> outermost
> is in subloop -> is in a subloop
Yep, let me update them.


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:788
+
       // If we have hoisted an instruction that may store, it can only be a
       // constant store.
----------------
dmgreen wrote:
> Do you know what this refers to? I'm not sure I understand what it means. It might be worth just removing it.
Let me check it.


================
Comment at: llvm/test/CodeGen/AMDGPU/optimize-negated-cond.ll:1
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --tool llc
 ; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
----------------
dmgreen wrote:
> This doesn't really look autogenerated to me.
Sorry... it looks it did not use the script first time...
For `negated_cond` function, `SIOptimizeExecMaskingPreRA` pass fails to fold mask operations `V_CNDMASK_B32_e64` and `V_CMP_NE_U32` because they are hoisted.
Let me update it manually.


================
Comment at: llvm/test/CodeGen/AMDGPU/optimize-negated-cond.ll:41
 ; GCN:   s_cmp_lg_u32
-; GCN:   s_cselect_b64  [[CC1:[^,]+]], -1, 0
-; GCN:   s_branch [[BB1:.LBB[0-9]+_[0-9]+]]
----------------
dmgreen wrote:
> Should all these lines be removed, or should they be updated for the new codegen?
It looks these test lines are correct.
Let me keep these lines in this patch.


================
Comment at: llvm/test/CodeGen/AMDGPU/optimize-negated-cond.ll:75
+;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+; GCN: {{.*}}
----------------
dmgreen wrote:
> This shouldn't be needed.
Yep, let me remove it.


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

https://reviews.llvm.org/D154205