[InstCombine] Remove transformation on call instruction where return value need void to non-void conversion #98536

yozhu · 2024-07-11T20:22:39Z

Skip simplification on call instruction where a non-void return value is
expected but the callee returns void, which is undefined behavior and can
lead to non-determinism runtime behaviors or crashes.

… conversion on return value

llvmbot · 2024-07-11T20:23:11Z

@llvm/pr-subscribers-llvm-transforms

Author: None (yozhu)

Changes

Add an option to skip simplification on call instruction where a non-void
return value is expected but the callee returns void, which is undefined
behavior and can lead to non-determinism runtime behaviors or crashes.

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

1 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+6-1)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index 0ca56f08c333d..880932159f570 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -89,6 +89,11 @@ static cl::opt<unsigned> GuardWideningWindow(
     cl::desc("How wide an instruction window to bypass looking for "
              "another guard"));
 
+static cl::opt<bool> SkipRetTypeVoidToNonVoidCallInst(
+    "instcombine-skip-call-ret-type-void-to-nonvoid", cl::init(false),
+    cl::desc("skip simplifying call instruction which expects a non-void "
+             "return value but the callee returns void"));
+
 /// Return the specified type promoted as it would be to pass though a va_arg
 /// area.
 static Type *getPromotedType(Type *Ty) {
@@ -4073,7 +4078,7 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
 
       if (!Caller->use_empty() &&
           // void -> non-void is handled specially
-          !NewRetTy->isVoidTy())
+          (SkipRetTypeVoidToNonVoidCallInst || !NewRetTy->isVoidTy()))
         return false;   // Cannot transform this return value.
     }

yozhu · 2024-07-11T20:24:55Z

Sorry - I messed up my local clone and branches and had to recreate this PR. It was the same code change as in #96574, where the conclusion is that adding an option to disable this optimization is fine but emitting a fatal error is not allowed.

goldsteinn · 2024-07-12T08:18:04Z

missing tests?

yozhu · 2024-07-26T18:26:17Z

Ping ~~ @nikic @goldsteinn

nikic · 2024-07-27T09:38:03Z

I think we should remove the void/non-void transform entirely, not just add an option. I don't really see a good reason to perform this transform. Looking back at the history, I think this is just a leftover of an overly liberal initial implementation.

yozhu · 2024-07-29T16:48:42Z

I think we should remove the void/non-void transform entirely, not just add an option. I don't really see a good reason to perform this transform. Looking back at the history, I think this is just a leftover of an overly liberal initial implementation.

Happy to remove this transformation entirely.

Given this, should we also emit an error when seeing this void to non-void conversion at call sites? It is known to be undefined behavior that could result in non-determinism or crash at runtime; and If it is non-LTO compilation, clang would output an error unconditionally.

…need void to non-void conversion

nikic · 2024-08-01T08:15:53Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

-      if (!Caller->use_empty() &&
-          // void -> non-void is handled specially
-          !NewRetTy->isVoidTy())
+      if (!Caller->use_empty() || NewRetTy->isVoidTy())


Suggested change

if (!Caller->use_empty() || NewRetTy->isVoidTy())

if (!Caller->use_empty())

A more conservative change would be this. In that case we'd still allow changing the return type to void as long as the return value is not used. That may be more consistent with the overall handling?

Yes this makes sense.

nikic · 2024-08-01T08:20:00Z

llvm/test/Transforms/InstCombine/skip-opt-void-to-non-void-conversion.ll

+; RUN: opt --passes=instcombine %t.bc -S | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-none-linux-android21"


Drop data layout and triple.

nikic · 2024-08-01T08:20:13Z

llvm/test/Transforms/InstCombine/skip-opt-void-to-non-void-conversion.ll

+; RUN: llvm-as %s -o %t.bc
+; RUN: opt --passes=instcombine %t.bc -S | FileCheck %s


Suggested change

; RUN: llvm-as %s -o %t.bc

; RUN: opt --passes=instcombine %t.bc -S | FileCheck %s

; RUN: opt --passes=instcombine -S < %s | FileCheck %s

nikic · 2024-08-01T08:20:51Z

llvm/test/Transforms/InstCombine/skip-opt-void-to-non-void-conversion.ll

+; CHECK-NEXT:    ret i32 [[TMP1]]
+;
+entry:
+  %_rc_ = alloca i32, align 4


This test is more complicated than it needs to be. You shouldn't need much more here than a call + return.

nikic · 2024-08-01T19:21:59Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

@@ -4233,9 +4231,6 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {

  AttributeSet FnAttrs = CallerPAL.getFnAttrs();

-  if (NewRetTy->isVoidTy())
-    Caller->setName("");   // Void type should not have a name.


I think this bit might still be needed? Can you please add another function to your test that does %res = call i32 @foo() but does not use the result? Just want to make sure this doesn't hit some void name assertion.

Yes this would necessary now after we have tightened the condition above to skip transformation only when return value has uses.

nikic · 2024-08-01T19:22:07Z

llvm/test/Transforms/InstCombine/skip-opt-void-to-non-void-conversion.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt --passes=instcombine -S < %s | FileCheck %s
+
+define dso_local void @foo() {


Drop dso_local.

nikic · 2024-08-01T19:22:25Z

llvm/test/Transforms/InstCombine/skip-opt-void-to-non-void-conversion.ll

+;
+entry:
+  %1 = tail call i32 @foo()
+  %2 = add i32 %1, 1


Can drop the add too :)

nikic

LGTM

AreaZR · 2024-08-02T19:25:57Z

Would it be wise to backport this as this is the removal of something undesirable from the codebase? Or are we okay with 19.x retaining this behavior?

yozhu · 2024-08-02T22:45:09Z

Would it be wise to backport this as this is the removal of something undesirable from the codebase? Or are we okay with 19.x retaining this behavior?

I'm fine with either way. This transformation is not desirable, but it had been in the codebase for quite a long time.

…value need void to non-void conversion (llvm#98536) Skip simplification on call instruction where a non-void return value is expected but the callee returns void, which is undefined behavior and could lead to non-determinism or crashes.

[InstCombine] Add an option to not take advantage of void to non-void…

03bb6f3

… conversion on return value

yozhu requested a review from nikic as a code owner July 11, 2024 20:22

llvmbot added the llvm:transforms label Jul 11, 2024

Add a test case

0ac5dd8

yozhu requested a review from goldsteinn July 15, 2024 21:45

[instcombine] Remove transformation involving return value from call …

2a4d481

…need void to non-void conversion

yozhu changed the title ~~[InstCombine] Add an option to skip simplification on call instruction where a non-void return value is expected while the callee returns void~~ [InstCombine] Remove transformation on call instruction where return value need void to non-void conversion Aug 1, 2024

nikic reviewed Aug 1, 2024

View reviewed changes

address review feedback

e61904e

yozhu requested a review from nikic August 1, 2024 16:24

nikic reviewed Aug 1, 2024

View reviewed changes

address review feedback 2

0863610

yozhu requested a review from nikic August 2, 2024 01:04

Forgot to remove the unnecessary add instr from the new test case

94d4332

nikic approved these changes Aug 2, 2024

View reviewed changes

yozhu merged commit 6c36716 into llvm:main Aug 2, 2024
5 of 7 checks passed

yozhu deleted the instcombinecall branch August 2, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Remove transformation on call instruction where return value need void to non-void conversion #98536

[InstCombine] Remove transformation on call instruction where return value need void to non-void conversion #98536

yozhu commented Jul 11, 2024 •

edited

Loading

llvmbot commented Jul 11, 2024

yozhu commented Jul 11, 2024

goldsteinn commented Jul 12, 2024

yozhu commented Jul 26, 2024 •

edited

Loading

nikic commented Jul 27, 2024

yozhu commented Jul 29, 2024

nikic Aug 1, 2024

yozhu Aug 1, 2024

nikic Aug 1, 2024

nikic Aug 1, 2024

nikic Aug 1, 2024

nikic Aug 1, 2024

yozhu Aug 2, 2024

nikic Aug 1, 2024

nikic Aug 1, 2024

nikic left a comment

AreaZR commented Aug 2, 2024

yozhu commented Aug 2, 2024

	if (!Caller->use_empty() \|\| NewRetTy->isVoidTy())
	if (!Caller->use_empty())

		; RUN: llvm-as %s -o %t.bc
		; RUN: opt --passes=instcombine %t.bc -S \| FileCheck %s

	; RUN: llvm-as %s -o %t.bc
	; RUN: opt --passes=instcombine %t.bc -S \| FileCheck %s
	; RUN: opt --passes=instcombine -S < %s \| FileCheck %s

[InstCombine] Remove transformation on call instruction where return value need void to non-void conversion #98536

[InstCombine] Remove transformation on call instruction where return value need void to non-void conversion #98536

Conversation

yozhu commented Jul 11, 2024 • edited Loading

llvmbot commented Jul 11, 2024

yozhu commented Jul 11, 2024

goldsteinn commented Jul 12, 2024

yozhu commented Jul 26, 2024 • edited Loading

nikic commented Jul 27, 2024

yozhu commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

AreaZR commented Aug 2, 2024

yozhu commented Aug 2, 2024

yozhu commented Jul 11, 2024 •

edited

Loading

yozhu commented Jul 26, 2024 •

edited

Loading