[PATCH] D157689: [GuardWidening] Refactor to work with the list of checks to widen/hoist

[Aleksandr Popov via Phabricator via llvm-commits]

aleksandr.popov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GuardWidening.cpp:697
+                                    SmallVectorImpl<Value *> &ChecksToWiden,
+                                    Instruction *InsertPt, Value *&Result) {
   using namespace llvm::PatternMatch;
----------------
anna wrote:
> Can we return the result instead of returning a boolean and storing the result in a separate argument?
> i.e.
> 
> ```
> std::optional<Value *> GuardWideningImpl::mergeChecks(SmallVectorImpl<Value *> &ChecksToHoist,
>                                     SmallVectorImpl<Value *> &ChecksToWiden,
>                                     Instruction *InsertPt)
> ```
> 
> If this works, pls land this separately before this patch (don't need explicit review) and then rebase on this revision. Makes it easier for review. Thanks. 
> 
> In places where only the boolean is required, you can just check the optional has value (In `isWideningCondProfitable` for example).
> 
I've extracted widenCondCommon splitting in a separate patch, PTAL: https://reviews.llvm.org/D159009


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

https://reviews.llvm.org/D157689