[PATCH] D140166: [IR] return nullptr in Instruction::getInsertionPointAfterDef for CallBrInst

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug.ll:196-198
+; CHECK-NEXT: %1 = load i8*, i8** %coro_hdl.reload.addr
+; CHECK-NEXT: call void @free(i8* %1)
 ; CHECK-NEXT: call void @llvm.dbg.declare(metadata i32 %[[CALLBR_RES]]
----------------
nickdesaulniers wrote:
> MaskRay wrote:
> > ChuanqiXu wrote:
> > > nickdesaulniers wrote:
> > > > ChuanqiXu wrote:
> > > > > We don't care about the inserted checks in the test. It should be fine to check the `llvm.dbg.declare` is in the basic block of `DEFAULT_DEST`. So maybe we can check these 2 lines are not empty or we can check there is no new BB declaration before  `llvm.dbg.declare`.
> > > > I'm not sure how best to express that to FileCheck.
> > > > 
> > > > `; CHECK-NEXT-NOT: {{.*}}:`
> > > > 
> > > > ?
> > > I feel it is a good way to check there is no new BB declaration before `llvm.dbg.declare`
> > Actually I think `utils/update_test_checks.py` may not be a bad choice for this large test file.
> > 
> > If it is not time to migrate the test, I think the patch as-is using `; CHECK-NEXT: %1 = load i8*, i8** ; ...` looks good.
> > It doesn't appear that there is more maintenance burden than `; CHECK-NEXT-NOT: {{.*}}:`
> Curiously, I tried removing existing CHECK lines from this test, then running `./llvm/utils/update_test_checks.py` on this. The result doesn't pass `llvm-lit -vv llvm/test/Transforms/Coroutines/coro-debug.ll` which may be why this test did not use `./llvm/utils/update_test_checks.py` in the first place.
Personally I prefer to not use `utils/update_test_checks.py` in coroutine's tests. Since CoroSplit pass will generate many codes and it is pretty hard to read. Currently when I see the coroutine's tests, I can know the pattern of the interesting part easily instead of reading tons of `CHECK` (that's what I feel when I read the test generated by `utils/update_test_checks.py`).

And for this patch, it is true that `; CHECK-NEXT: %1 = load i8*, i8** ; ... ` wouldn't matter a lot. But I still feel better to make the tests focus on the things it want to test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140166