Function has token parameter but isn't an intrinsic

[compnerd]

Bugzilla Link	40710
Version	unspecified
OS	Windows NT
CC	@DougGregor,@hiraditya,@pcc,@zygoloid,@rnk,@smeenai,@vedantk

Extended Description

Reduced from ICU 61:

// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc19.0.0 -emit-obj -o /dev/null -O1 -fcxx-exceptions -fexceptions %s
struct T {
  virtual ~T();
};

void f();

struct S {
  virtual ~S();
  T t_[1];
};

S::~S() { f(); }

[compnerd]

assigned to @hiraditya

[compnerd]

The IR verifier might be too stringent on the token checking. The outlined filter funclet is given a token to tie it to the parent clause but is a "Function" in IR land. This triggers a verification failure.

[compnerd]

This is caused by the hot/cold splitting code path promoting the token to a formal argument.

[rnk]

(pasting in stuff I said in IRC)

1. outlining WinEH code is fine, but unlikely to be profitable or save size, as EH funclets are already "outlined", so it's a bit redundant
2. if you do outline code with funclet bundles, just delete all "funclet" bundle operands and make a "funclet" operand bundle on the call to the outlined code

And, I think the verifier is working as intended here. You are not supposed to be able to pass a token as a regular parameter, it can't be passed through registers/memory/phis, etc, and I think arguments fit in there as well. It just so happens that you can look at the bundle type ("funclet" in this case) and reason about it, so it is possible to do what you want, but in the general case, I don't think tokens can be outlined.

[vedantk]

(pasting in stuff I said in IRC)

1. outlining WinEH code is fine, but unlikely to be profitable or save size,
   as EH funclets are already "outlined", so it's a bit redundant

I only have a vague understanding of WinEH, so apologies for the naive question: is the funclet really the thing being split here?

It looks like two blocks dominated by a cleanuppad are split, and afaik they aren't normally outlined?

2. if you do outline code with funclet bundles, just delete all "funclet"
   bundle operands and make a "funclet" operand bundle on the call to the
   outlined code

Should splitting even be allowed on functions with WinEH personality? I don't think any run-time testing has been done with this. And as you point out, it may not be profitable.

And, I think the verifier is working as intended here. You are not supposed
to be able to pass a token as a regular parameter, it can't be passed
through registers/memory/phis, etc, and I think arguments fit in there as
well. It just so happens that you can look at the bundle type ("funclet" in
this case) and reason about it, so it is possible to do what you want, but
in the general case, I don't think tokens can be outlined.

This points to a separate bug. CodeExtractor shouldn't permit this.

[vedantk]

Proposed patch (disable splitting within functions with scoped EH personalities)

[compnerd]

Created attachment 21477 [details]
Proposed patch (disable splitting within functions with scoped EH
personalities)

I think that we should still teach the Hot/Cold splitting to deal with the tokens. But, yes the funclets should not be split into the cold path since they are already outlined.

[rnk]

I only have a vague understanding of WinEH, so apologies for the naive
question: is the funclet really the thing being split here?

It looks like two blocks dominated by a cleanuppad are split, and afaik they
aren't normally outlined?

When I compile this code with -O2 and run the hotcoldsplit pass on it, I see this block being outlined:

Found a cold block:

ehcleanup:                                        ; preds = %entry
  %1 = cleanuppad within none []
  %arraydestroy.element7 = getelementptr inbounds %struct.S, %struct.S* %this, i64 0, i32 1, i64 0
  call void @"??1T@@UEAA@XZ"(%struct.T* nonnull %arraydestroy.element7) #​4 [ "funclet"(token %1) ]
  call void @​__std_terminate() #​5 [ "funclet"(token %1) ]
  unreachable

Outlining in ??_GS@@UEAAPEAXI@Z

The blocks in the cleanuppad region constitute the funclet. They share a stack frame with the parent function, but the code is discontiguous from the parent function and has its own prologue/epilogue.

Should splitting even be allowed on functions with WinEH personality? I
don't think any run-time testing has been done with this. And as you point
out, it may not be profitable.

Up to you. I think if we update CodeExtractor to be token aware, this may sort itself out naturally. Keep in mind that checking isScopedEHPersonality also affects web assembly, which I think uses the same representation.

And, I think the verifier is working as intended here. You are not supposed
to be able to pass a token as a regular parameter, it can't be passed
through registers/memory/phis, etc, and I think arguments fit in there as
well. It just so happens that you can look at the bundle type ("funclet" in
this case) and reason about it, so it is possible to do what you want, but
in the general case, I don't think tokens can be outlined.

This points to a separate bug. CodeExtractor shouldn't permit this.

Makes sense.

[vedantk]

I only have a vague understanding of WinEH, so apologies for the naive
question: is the funclet really the thing being split here?

It looks like two blocks dominated by a cleanuppad are split, and afaik they
aren't normally outlined?

When I compile this code with -O2 and run the hotcoldsplit pass on it, I see
this block being outlined:

'''
Found a cold block:

ehcleanup: ; preds = %entry
%1 = cleanuppad within none []
%arraydestroy.element7 = getelementptr inbounds %struct.S, %struct.S*
%this, i64 0, i32 1, i64 0
call void @"??1T@@UEAA@XZ"(%struct.T* nonnull %arraydestroy.element7) #​4 [
"funclet"(token %1) ]
call void @​__std_terminate() #​5 [ "funclet"(token %1) ]
unreachable

Outlining in ??_GS@@UEAAPEAXI@Z
'''

The blocks in the cleanuppad region constitute the funclet. They share a
stack frame with the parent function, but the code is discontiguous from the
parent function and has its own prologue/epilogue.

The splitting pass will refuse to extract EH pads (see mayExtractBlock). It does still consider EH pads to be cold, though, and will propagate that information.

By 'cleanuppad region', can I assume you mean blocks which use a cleannuppad token?

Should splitting even be allowed on functions with WinEH personality? I
don't think any run-time testing has been done with this. And as you point
out, it may not be profitable.

Up to you. I think if we update CodeExtractor to be token aware, this may
sort itself out naturally. Keep in mind that checking isScopedEHPersonality
also affects web assembly, which I think uses the same representation.

I'll take a cut at this, but might need some help with the run-time testing bit.

And, I think the verifier is working as intended here. You are not supposed
to be able to pass a token as a regular parameter, it can't be passed
through registers/memory/phis, etc, and I think arguments fit in there as
well. It just so happens that you can look at the bundle type ("funclet" in
this case) and reason about it, so it is possible to do what you want, but
in the general case, I don't think tokens can be outlined.

This points to a separate bug. CodeExtractor shouldn't permit this.

Makes sense.

[compnerd]

BTW, I ended up with this reduction:

; RUN: opt -hotcoldsplit %s -S -o /dev/null

define void @"??1S@@UEAA@XZ"() personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
entry:
  invoke void @"?f@@YAXXZ"()
          to label %invoke.cont unwind label %ehcleanup

invoke.cont:
  ret void

ehcleanup:
  %0 = cleanuppad within none []
  br label %ehcleanup.resume

ehcleanup.resume:
  call void @"?f@@YAXXZ"() [ "funclet"(token %0) ]
  br label %terminate

terminmate:
  call void @"?terminate@@YAXXZ"() [ "funclet"(token %0) ]
  unreachable
}

declare void @"?f@@YAXXZ"()

declare i32 @__CxxFrameHandler3(...)

declare void @"?terminate@@YAXXZ"()

[hiraditya]

Thanks for providing the repro @Saleem. There's a small typo so the fixed up repro is here:

; RUN: opt --hot-cold-split %s -S -o /dev/null

define void @"??1S@@UEAA@XZ"() personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
entry:
  invoke void @"?f@@YAXXZ"()
          to label %invoke.cont unwind label %ehcleanup

invoke.cont:
  ret void

ehcleanup:
  %0 = cleanuppad within none []
  br label %ehcleanup.resume

ehcleanup.resume:
  call void @"?f@@YAXXZ"() [ "funclet"(token %0) ]
  br label %terminate

terminate:
  call void @"?terminate@@YAXXZ"() [ "funclet"(token %0) ]
  unreachable
}

declare void @"?f@@YAXXZ"()

declare i32 @__CxxFrameHandler3(...)

declare void @"?terminate@@YAXXZ"()

the compilation aborts after hotcoldsplit with the following IR.

*** IR Dump After Hot Cold Splitting ***
; ModuleID = '<stdin>'
source_filename = "<stdin>"

define void @"??1S@@UEAA@XZ"() personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
entry:
  invoke void @"?f@@YAXXZ"()
          to label %invoke.cont unwind label %ehcleanup

invoke.cont:                                      ; preds = %entry
  ret void

ehcleanup:                                        ; preds = %entry
  %0 = cleanuppad within none []
  br label %codeRepl

codeRepl:                                         ; preds = %ehcleanup
  call void @"??1S@@UEAA@XZ.cold.1"(token %0) #1
  ret void
}

declare void @"?f@@YAXXZ"()

declare i32 @__CxxFrameHandler3(...)

declare void @"?terminate@@YAXXZ"()

; Function Attrs: cold minsize noreturn
define internal void @"??1S@@UEAA@XZ.cold.1"(token %0) #0 personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
newFuncRoot:
  br label %ehcleanup.resume

ehcleanup.resume:                                 ; preds = %newFuncRoot
  call void @"?f@@YAXXZ"() [ "funclet"(token %0) ]
  br label %terminate

terminate:                                        ; preds = %ehcleanup.resume
  call void @"?terminate@@YAXXZ"() [ "funclet"(token %0) ]
  unreachable
}

attributes #0 = { cold minsize noreturn }
attributes #1 = { noinline }
Function has token parameter but isn't an intrinsic
  call void @"??1S@@UEAA@XZ.cold.1"(token %0) #1
in function ??1S@@UEAA@XZ
LLVM ERROR: Broken function found, compilation aborted!

[hiraditya]

Bug fix: D68192