sgraenitz marked an inline comment as done. sgraenitz added a comment. My follow-up got delayed, because I hit another bug, which appears to be a regression in release 14. This is why I wrote the tests for release/13.x and I still have to port them back to mainline, so this is *not yet ready to land*. As I don't expect it to cause big changes, however, I though it might be good to update the review anyway.
Rebased on release/13.x I am running the tests like this and both are passing: > ninja llvm-config llvm-readobj llc FileCheck count not > bin\llvm-lit.py --filter=win64-funclet-preisel-intrinsics test\CodeGen\X86 > ninja clang > bin\llvm-lit.py --filter=arc-exceptions-seh tools\clang\test @rnk What do you think about implementation, naming, etc. Is that ok? In D124762#3486288 <https://reviews.llvm.org/D124762#3486288>, @rnk wrote: > Lastly, I think the inliner needs to be taught that these intrinsics need to > carry operand bundles, otherwise this problem will arise after optimization. > The inliner has the same logic here: > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2311 Thanks for the note. Yes, I can reproduce that. I am not yet sure what's the best way to fix it though. Actually, I'd prefer to split it out into a separate review, so the two discussions can remain their individual focus. What do you think? ================ Comment at: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm:1 +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s + ---------------- Note: There is another test without the `-seh` postfix that checks very similar situations for CodeGen with the macOS runtime. ================ Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:966 - if (FuncletBundleOperand == FuncletPad) + if (!FuncletPad || FuncletBundleOperand == FuncletPad) continue; ---------------- sgraenitz wrote: > rnk wrote: > > Is this change necessary? It seems like it shouldn't be after the clang and > > preisel pass changes. > Yes, apparently it is necessary. There are cases, where > `CodeGenFunction::getBundlesForFunclet()` registers a "funclet" bundle > operand, but the `FuncletPad` here is null. My guess was it might be due to > optimizations? Might that be due to the inliner issue then? I'd address it in the follow-up review if it's ok? ================ Comment at: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll:53 +; At this point the code used to be truncated: +; CHECK-NOT: int3 +; ---------------- Another side-note: Often times Clang just didn't emit anything here, so execution would run into whatever code comes next. The `int3` I sometimes got might as well have been "accidental" padding. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124762/new/ https://reviews.llvm.org/D124762 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
