Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ubsan] Improve lowering of @llvm.allow.ubsan.check #119013

Conversation

vitalybuka
Copy link
Collaborator

This fix the case, when single hot inlined callsite, prevent
checks for all other. This helps to reduce number of removed checks up to 50% (deppedes on cutoff-hot value) .

ScalarOptimizerLateEPCallback was happening during
CGSCC walk, after each inlining, but this is effectively
after inlining.

Example, order in comments:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  // 3. LowerAllowCheckPass
  set(get() + get());
}

void test() {
  // 4. Inline
  // 5. Nothing for LowerAllowCheckPass
  overflow();
}

With this patch it will look like:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  set(get() + get());
}

void test() {
  // 3. Inline
  // 4. Simplify
  overflow();
}

// Later, after inliner CGSCC walk complete:
// 5. LowerAllowCheckPass for `overflow`
// 6. LowerAllowCheckPass for `test`

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Dec 6, 2024
@vitalybuka vitalybuka requested a review from kstoimenov December 6, 2024 19:18
@llvmbot
Copy link
Member

llvmbot commented Dec 6, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

This fix the case, when single hot inlined callsite, prevent
checks for all other. This helps to reduce number of removed checks up to 50% (deppedes on cutoff-hot value) .

ScalarOptimizerLateEPCallback was happening during
CGSCC walk, after each inlining, but this is effectively
after inlining.

Example, order in comments:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  // 3. LowerAllowCheckPass
  set(get() + get());
}

void test() {
  // 4. Inline
  // 5. Nothing for LowerAllowCheckPass
  overflow();
}

With this patch it will look like:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  set(get() + get());
}

void test() {
  // 3. Inline
  // 4. Simplify
  overflow();
}

// Later, after inliner CGSCC walk complete:
// 5. LowerAllowCheckPass for `overflow`
// 6. LowerAllowCheckPass for `test`

Full diff: https://github.com/llvm/llvm-project/pull/119013.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+6-7)
  • (modified) clang/test/CodeGen/allow-ubsan-check-inline.c (+2-2)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index bf9b04f02e9f44..1f0e760e8ebd29 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -789,13 +789,12 @@ static void addSanitizers(const Triple &TargetTriple,
   }
 
   if (LowerAllowCheckPass::IsRequested()) {
-    // We can optimize after inliner, and PGO profile matching. The hook below
-    // is called at the end `buildFunctionSimplificationPipeline`, which called
-    // from `buildInlinerPipeline`, which called after profile matching.
-    PB.registerScalarOptimizerLateEPCallback(
-        [](FunctionPassManager &FPM, OptimizationLevel Level) {
-          FPM.addPass(LowerAllowCheckPass());
-        });
+    // We want to call it after inline, which is about OptimizerEarlyEPCallback.
+    PB.registerOptimizerEarlyEPCallback([](ModulePassManager &MPM,
+                                           OptimizationLevel Level,
+                                           ThinOrFullLTOPhase Phase) {
+      MPM.addPass(createModuleToFunctionPassAdaptor(LowerAllowCheckPass()));
+    });
   }
 }
 
diff --git a/clang/test/CodeGen/allow-ubsan-check-inline.c b/clang/test/CodeGen/allow-ubsan-check-inline.c
index cabe76d8034d77..1de24ab90dac0e 100644
--- a/clang/test/CodeGen/allow-ubsan-check-inline.c
+++ b/clang/test/CodeGen/allow-ubsan-check-inline.c
@@ -7,8 +7,8 @@ void set(int x);
 // We will only make decision in the `overflow` function.
 // NOINL-COUNT-1: remark: Allowed check:
 
-// FIXME: We will make decision on every inline.
-// INLINE-COUNT-1: remark: Allowed check:
+// We will make decision on every inline.
+// INLINE-COUNT-5: remark: Allowed check:
 
 static void overflow() {
   set(get() + get());

@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.ubsan-improve-lowering-of-llvmallowubsancheck to main December 7, 2024 22:50
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 7787328 into main Dec 8, 2024
8 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/ubsan-improve-lowering-of-llvmallowubsancheck branch December 8, 2024 00:12
broxigarchen pushed a commit to broxigarchen/llvm-project that referenced this pull request Dec 10, 2024
This fix the case, when single hot inlined callsite, prevent
checks for all other. This helps to reduce number of removed checks up
to 50% (deppedes on `cutoff-hot` value) .

`ScalarOptimizerLateEPCallback` was happening during
CGSCC walk, after each inlining, but this is effectively
after inlining.

Example, order in comments:

```
static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  // 3. LowerAllowCheckPass
  set(get() + get());
}

void test() {
  // 4. Inline
  // 5. Nothing for LowerAllowCheckPass
  overflow();
}
```

With this patch it will look like:
```
static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  set(get() + get());
}

void test() {
  // 3. Inline
  // 4. Simplify
  overflow();
}

// Later, after inliner CGSCC walk complete:
// 5. LowerAllowCheckPass for `overflow`
// 6. LowerAllowCheckPass for `test`
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants