[PATCH] D129230: [IR][coroutine] make final and non-final llvm.coro.saves unique
Yuanfang Chen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 6 15:00:00 PDT 2022
ychen created this revision.
ychen added reviewers: rjmccall, ChuanqiXu.
Herald added subscribers: jdoerfert, hiraditya.
Herald added a project: All.
ychen requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
An alternative to D129025 <https://reviews.llvm.org/D129025>.
For
define void @f(i32 %x) {
entry:
%cmp = icmp slt i32 %x, 0
br i1 %cmp, label %await.suspend, label %final.suspend
await.suspend:
%0 = call token @llvm.coro.save(ptr null)
%1 = call i8 @llvm.coro.suspend(token %0, i1 false)
br label %coro.ret
final.suspend:
%2 = call token @llvm.coro.save(ptr null)
%3 = call i8 @llvm.coro.suspend(token %2, i1 true)
br label %coro.ret
coro.ret:
ret void
}
SimplifyCFG would hoist `llvm.coro.save` into the entry block. However,
since one is for the final suspend and the other is not, the transformation
is not desired.
I proposed D129025 <https://reviews.llvm.org/D129025>, which requires a update to the token type definition
in LangRef to make merging two token-returning instructions illegal
(which I didn't do yet and I think it may affect other users of the token
type like WinEH, GC intrinsics). D129025 <https://reviews.llvm.org/D129025> also excludes some valid
transformation: for this test case, if both branches of the if statement
contains non-final suspend, it is ok to hoisting `llvm.coro.save`.
So this patch, I propose to directly change the IR equality between two
`llvm.coro.save` to disable the invalid hoisting. I don't like the
special casing of `llvm.coro.save` in the `llvm/IR` space however, the
only other choice would be to change `llvm.coro.save` in some way like:
1. add new flag parameter to `llvm.coro.save` 2. add
`llvm.coro.save.final`, both of which, I'm not a fan of.
Any thoughts?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D129230
Files:
llvm/docs/LangRef.rst
llvm/lib/IR/Instruction.cpp
llvm/lib/Transforms/Utils/FunctionComparator.cpp
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1505,10 +1505,6 @@
if (I1->isTerminator())
goto HoistTerminator;
- // Hoisting token-returning instructions would obscure the origin.
- if (I1->getType()->isTokenTy())
- return Changed;
-
// If we're going to hoist a call, make sure that the two instructions we're
// commoning/hoisting are both marked with musttail, or neither of them is
// marked as such. Otherwise, we might end up in a situation where we hoist
Index: llvm/lib/Transforms/Utils/FunctionComparator.cpp
===================================================================
--- llvm/lib/Transforms/Utils/FunctionComparator.cpp
+++ llvm/lib/Transforms/Utils/FunctionComparator.cpp
@@ -28,6 +28,7 @@
#include "llvm/IR/GlobalValue.h"
#include "llvm/IR/InlineAsm.h"
#include "llvm/IR/InstrTypes.h"
+#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/LLVMContext.h"
@@ -596,6 +597,21 @@
if (const CmpInst *CI = dyn_cast<CmpInst>(L))
return cmpNumbers(CI->getPredicate(), cast<CmpInst>(R)->getPredicate());
if (auto *CBL = dyn_cast<CallBase>(L)) {
+ // Must have the same "Final Suspend" flag.
+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(L)) {
+ if (II->getIntrinsicID() == Intrinsic::coro_save && L->getNumUses() &&
+ R->getNumUses()) {
+ auto *CS1 = cast<IntrinsicInst>(L->user_back());
+ auto *CS2 = cast<IntrinsicInst>(R->user_back());
+ assert(CS1->getIntrinsicID() == Intrinsic::coro_suspend &&
+ CS2->getIntrinsicID() == Intrinsic::coro_suspend);
+ bool IsFinal1 = cast<Constant>(CS1->getArgOperand(1))->isOneValue();
+ bool IsFinal2 = cast<Constant>(CS2->getArgOperand(1))->isOneValue();
+ if (int Res = cmpNumbers(IsFinal1, IsFinal2))
+ return Res;
+ }
+ }
+
auto *CBR = cast<CallBase>(R);
if (int Res = cmpNumbers(CBL->getCallingConv(), CBR->getCallingConv()))
return Res;
Index: llvm/lib/IR/Instruction.cpp
===================================================================
--- llvm/lib/IR/Instruction.cpp
+++ llvm/lib/IR/Instruction.cpp
@@ -455,11 +455,24 @@
SI->getSyncScopeID() == cast<StoreInst>(I2)->getSyncScopeID();
if (const CmpInst *CI = dyn_cast<CmpInst>(I1))
return CI->getPredicate() == cast<CmpInst>(I2)->getPredicate();
- if (const CallInst *CI = dyn_cast<CallInst>(I1))
+ if (const CallInst *CI = dyn_cast<CallInst>(I1)) {
+ // Must have the same "Final Suspend" flag.
+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I1)) {
+ if (II->getIntrinsicID() == Intrinsic::coro_save && I1->getNumUses() &&
+ I2->getNumUses()) {
+ auto *CS1 = cast<IntrinsicInst>(I1->user_back());
+ auto *CS2 = cast<IntrinsicInst>(I2->user_back());
+ assert(CS1->getIntrinsicID() == Intrinsic::coro_suspend &&
+ CS2->getIntrinsicID() == Intrinsic::coro_suspend);
+ return cast<Constant>(CS1->getArgOperand(1))->isOneValue() ==
+ cast<Constant>(CS2->getArgOperand(1))->isOneValue();
+ }
+ }
return CI->isTailCall() == cast<CallInst>(I2)->isTailCall() &&
CI->getCallingConv() == cast<CallInst>(I2)->getCallingConv() &&
CI->getAttributes() == cast<CallInst>(I2)->getAttributes() &&
CI->hasIdenticalOperandBundleSchema(*cast<CallInst>(I2));
+ }
if (const InvokeInst *CI = dyn_cast<InvokeInst>(I1))
return CI->getCallingConv() == cast<InvokeInst>(I2)->getCallingConv() &&
CI->getAttributes() == cast<InvokeInst>(I2)->getAttributes() &&
Index: llvm/docs/LangRef.rst
===================================================================
--- llvm/docs/LangRef.rst
+++ llvm/docs/LangRef.rst
@@ -3714,8 +3714,9 @@
The token type is used when a value is associated with an instruction
but all uses of the value must not attempt to introspect or obscure it.
-As such, it is not appropriate to have a :ref:`phi <i_phi>` or
-:ref:`select <i_select>` of type token.
+As such, it is illegal to have a :ref:`phi <i_phi>` or
+:ref:`select <i_select>` of type token. It is also illegal to merge two
+token returning instructions that belongs to different basic blocks unless their respective users could also be merged.
:Syntax:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129230.442694.patch
Type: text/x-patch
Size: 4516 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220706/0629b636/attachment.bin>
More information about the llvm-commits
mailing list