[PATCH] D116327: [Coroutines] Enhance symmetric transfer for constant CmpInst

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:1247
+      if (!CondConst)
+        return false;
+
----------------
rjmccall wrote:
> Should we do the same lookup in `ResolvedValues` here?
> 
> I would suggest having a general helper that you can put at the top of this function:
> 
> ```
> auto tryResolveConstant = [&](Value *V) -> Constant* {
>   auto it = ResolvedValues.find(V));
>   if (it != ResolvedValues.end())
>    V = it->second;
>   return dyn_cast<Constant>(V);
> };
> ```
> 
> You don't need to check specifically for `ConstantInt`; LLVM can constant-fold some conditions on other kinds of constants.
> Should we do the same lookup in ResolvedValues here?

According to the comment and transformation detail, the second operand should be literal constant, so we shouldn't call TryResolveConstant for it. I've added this to comment. 

> I would suggest having a general helper that you can put at the top of this function:

Done. Thanks for suggestion!

> You don't need to check specifically for ConstantInt; LLVM can constant-fold some conditions on other kinds of constants.

If we check for `Constant` instead of `ConstantInt`, then we must write `auto *Cond = dyn_cast<ConstantInt>(TryResolveConstant(...));` on line 1277 since the argument type of `findCaseValue` is `ConstantInt`, it looks odd to me. And we wouldn't lose anything if we check for `ConstantInt` only, so I chose to check specifically for ConstantInt in this revision still.




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

https://reviews.llvm.org/D116327