[PATCH] D123848: [RS4GC] Don't clone BDV if its inputs are not derived

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:720
+  explicit BDVState(Value *OriginalValue, StatusTy Status, bool IsDerived,
+                    Value *BaseValue = nullptr)
+      : OriginalValue(OriginalValue), Status(Status), IsDerived(IsDerived),
----------------
What worries me is that we never check the consistency of state of `IsDerived` and `BaseValue`. There should be some invariant like "if `BaseValue` is defined and not equal to `OriginalValue` then `IsDerived` should be true", or whatsoever. Do you want to add some sanity checks to make sure these states are not self-contradictory?

Ideally I'd completely give up a separate boolean flag for `IsDerived` and answer the method through other fields, but not sure if it's possible.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1033
 
+  ToRemove.clear();
+  for (auto Pair : States) {
----------------
1) If I'm reading this code correctly, `ToRemove` should already be empty and you can assert that instead.
2) I don't feel comfortable using the same `ToRemove` vector for 2 different unrelated activities. Limit scope or declare a new one?


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1034
+  ToRemove.clear();
+  for (auto Pair : States) {
+    Value *BDV = Pair.first;
----------------
Would that work here?
```
remove_if(States, [](It) { return !It.second.isDerived() ;}
```
?


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1080
       // conflicts is handled in the next loop which traverses States.
-      States[I] = BDVState(I, BDVState::Conflict);
+      States[I] = BDVState(I, BDVState::Conflict, true);
     }
----------------
Please name the boolean parameter `/* IsDerived*/ true`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123848