[PATCH] D78326: [PredicateInfo] Factor out PredicateInfoBuilder (NFC)

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: lib/Transforms/Utils/PredicateInfo.cpp:493
   // as assume statements.
-  SmallVector<Value *, 8> OpsToRename;
   for (auto DTN : depth_first(DT.getRootNode())) {
----------------
fhahn wrote:
> nikic wrote:
> > fhahn wrote:
> > > I think it would be slightly better to keep passing OpsToRename to various places, as it makes things a bit more explicit; that's the main list op operations to rename.
> > Hm, why do you think that OpsToRename should be handled differently than other state like ValueInfo?
> > 
> > In fact, now that I look a bit more closely at this, I think there's some duplication going on here, in that the values stored in OpsToRename should be the same as the keys of ValueInfoNums. I guess this is stored separate to preserve order?
> > Hm, why do you think that OpsToRename should be handled differently than other state like ValueInfo?
> 
> I think passing it to renameUses makes it clearer at the call site & function signature *what* uses are renamed, although it's true that it currently queries some other information as well. I have a few patches somewhere, which expose renameUses for general SSA renaming and there it would be convenient to just pass in the list of uses to rename.
> 
> > In fact, now that I look a bit more closely at this, I think there's some duplication going on here, in that the values stored in OpsToRename should be the same as the keys of ValueInfoNums. I guess this is stored separate to preserve order?
> `ValueInfoNums` maps values to their ValueInfo entry and I think it is used to ensure that there's a single ValueInfo per value. I guess alternatively we could just use a map Value* -> ValueInfo, but then we would have to  iterate over the keys of the map. That would safe sizeof(unsigned) per value to rename, but iterating over the keys in the map should be more expensive in terms of runtime.
> I think passing it to renameUses makes it clearer at the call site & function signature *what* uses are renamed, although it's true that it currently queries some other information as well. I have a few patches somewhere, which expose renameUses for general SSA renaming and there it would be convenient to just pass in the list of uses to rename.

Fair enough, I've changed the code back to pass OpsToRename explicitly.

> ValueInfoNums maps values to their ValueInfo entry and I think it is used to ensure that there's a single ValueInfo per value. I guess alternatively we could just use a map Value* -> ValueInfo, but then we would have to iterate over the keys of the map. That would safe sizeof(unsigned) per value to rename, but iterating over the keys in the map should be more expensive in terms of runtime.

I think the split into ValueInfoNums and ValueInfos here is because ValueInfo is large, so storing it in a DenseMap is inefficient. Using a DenseMap of integers that index into a separate vector avoids that.


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

https://reviews.llvm.org/D78326