Page MenuHomePhabricator

[RISCV] Do not outline CFI instructions when they are needed in EH
ClosedPublic

Authored by pcwang-thead on Mar 28 2022, 10:18 PM.

Details

Summary

We saw a failure caused by unwinding with incomplete CFIs, so we
can't outline CFI instructions when they are needed in EH.

Diff Detail

Unit TestsFailed

TimeTest
60,070 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics::vloxseg_mask_mf.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -triple riscv64 -target-feature +f -target-feature +d -target-feature +zfh -target-feature +experimental-zvfh -target-feature +v -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vloxseg_mask_mf.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vloxseg_mask_mf.c
60,160 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics::vlsegff_mask.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -triple riscv32 -target-feature +f -target-feature +d -target-feature +v -target-feature +zfh -target-feature +experimental-zvfh -disable-O0-optnone -fallow-half-arguments-and-returns -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vlsegff_mask.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV32 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vlsegff_mask.c
60,340 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics::vluxseg_mask.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -triple riscv64 -target-feature +f -target-feature +d -target-feature +zfh -target-feature +experimental-zvfh -target-feature +v -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vluxseg_mask.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vluxseg_mask.c
60,120 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics::vluxseg_mask_mf.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -triple riscv64 -target-feature +f -target-feature +d -target-feature +zfh -target-feature +experimental-zvfh -target-feature +v -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vluxseg_mask_mf.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics/vluxseg_mask_mf.c
60,340 msx64 debian > Clang.Driver::hip-sanitize-options.hip
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ -fsanitize=address -nogpuinc --rocm-path=/var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/Inputs/rocm /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/hip-sanitize-options.hip 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -check-prefixes=NORDC /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/hip-sanitize-options.hip
View Full Test Results (8 Failed)

Event Timeline

pcwang-thead created this revision.Mar 28 2022, 10:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 10:18 PM
pcwang-thead requested review of this revision.Mar 28 2022, 10:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 10:18 PM

Rebase on precommit test D123364.

We this change we now don't have any tests that show a difference compared with just if (MI.isPosition()) return outliner::InstrType::Illegal;
Please add a test that showcases that new, more fined grained, outlining criteria for position instructions.

We this change we now don't have any tests that show a difference compared with just if (MI.isPosition()) return outliner::InstrType::Illegal;
Please add a test that showcases that new, more fined grained, outlining criteria for position instructions.

Thanks for your advice, but what I changed in this patch is just the way to handle CFIs when unwinding is needed.
The original code is:

if (MI.isPosition()) {
    // We can manually strip out CFI instructions later.
    if (MI.isCFIInstruction())
      return outliner::InstrType::Invisible;
    return outliner::InstrType::Illegal;
}

These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.

These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.

It's been a few days since my original comment so I may be misremembering the details but here's my perspective:

  • Before your patch we had test coverage that showed the difference between a plain if (MI.isPosition()) return outliner::InstrType::Illegal; and the then-current implementation.
  • With this patch we no longer have.
  • With this patch you are asserting that we want something more fine-grained than just "don't outline positions". Then let's have test coverage for that more fine-grained condition checking.

Also, in general, I'm in favor of the position that when you touch things you should take the opportunity to address closely related issues, within reason. That's always a tricky balance, and I don't know what the LLVM project policy is for that fine line, but it seems like something that we should strive for :)

Rebase on new test.

These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.

It's been a few days since my original comment so I may be misremembering the details but here's my perspective:

  • Before your patch we had test coverage that showed the difference between a plain if (MI.isPosition()) return outliner::InstrType::Illegal; and the then-current implementation.
  • With this patch we no longer have.
  • With this patch you are asserting that we want something more fine-grained than just "don't outline positions". Then let's have test coverage for that more fine-grained condition checking.

Also, in general, I'm in favor of the position that when you touch things you should take the opportunity to address closely related issues, within reason. That's always a tricky balance, and I don't know what the LLVM project policy is for that fine line, but it seems like something that we should strive for :)

Thank you for your explanation, I misunderstood what you meant :)

So I added some more fine-grained tests in D123364:

  • machine-outliner-cfi.mir shows how we treat CFIs if there is no EH.
  • machine-outliner-throw.ll shows how we treat CFIs if unwinding is needed.
  • machine-outliner-position.mir shows that position instructions(which are labels) are illegal to outline.

Are there some other things that I should pay attention to?

luismarques accepted this revision.Apr 20 2022, 3:23 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Apr 20 2022, 3:23 AM
This revision was landed with ongoing or failed builds.Apr 21 2022, 1:14 AM
This revision was automatically updated to reflect the committed changes.
pcwang-thead reopened this revision.Apr 21 2022, 6:16 AM
This revision is now accepted and ready to land.Apr 21 2022, 6:16 AM

Rebase on fixed tests.

pcwang-thead updated this revision to Diff 424184.EditedApr 21 2022, 6:47 AM

Trigger buildbot.

This revision was landed with ongoing or failed builds.Apr 21 2022, 9:30 PM
This revision was automatically updated to reflect the committed changes.