[PATCH] D87488: [ImplicitNullCheck] Hoisting multiple dependencies

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin added inline comments.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:465
+  // all instructions that were independent of the FaultingMI).
+  enum DependenceState { DS_Unknown, DS_None, DS_Safe };
+
----------------
Would be great to have a brief comment about what's 'safe' dependence is.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:473
+    for (auto &MO : MI->uses()) {
+      if (MO.isReg() && MO.isUse())
+        TrackedRegisterUses.insert(MO.getReg());
----------------
You say about explicit uses, but do not have corresponding check.
`MI->uses()` includes everything except explicit defs. Do you want to use `MI->explicit_uses()` here?



================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:504
+    if (CurrMI->mayLoadOrStore())
+      return false;
 
----------------
Move this check to the top of loop? (Before `canReorder()` check)


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:525
+                                TrackedRegisterUses.count(MO.getSubReg()));
+                      }))
+      return false;
----------------
Er, are you checking defs of CurrMI? Would `CurrMI->defs()` work here (if you're checking explicit defs only)?
Or loop over TrackedRegisterUSes using `CurrMI->modifiesRegister()` as predicate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87488