[PATCH] D123848: [RS4GC] Don't clone BDV if its inputs are not derived
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 18 00:57:09 PDT 2022
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
More information about the llvm-commits
mailing list