This is an archive of the discontinued LLVM Phabricator instance.

Differential D104142

[IROutliner] Ensure instructions at end of candidate are excluded
ClosedPublic

Authored by AndrewLitteken on Jun 11 2021, 12:00 PM.

Details

Reviewers
jroelofs
paquette
Commits
rGf564299fe94b: [IROutliner] Ensure instructions at end of candidate are excluded
Summary

Occasionally instructions are between the last instruction in a region, and the following instruction as identified by the Candidate. This adds an extra check right before splitting a candidate that excludes the region from being split/checked for outlining to remove errors.

Tests Changed:
Tranforms/IROutliner/outlining-extra-bitcasts.ll

Diff Detail

Event Timeline

AndrewLitteken created this revision.Jun 11 2021, 12:00 PM
Herald added subscribers: ormris, hiraditya. · View Herald TranscriptJun 11 2021, 12:00 PM
AndrewLitteken requested review of this revision.Jun 11 2021, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 12:00 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
AndrewLitteken added a reviewer: paquette.Jun 11 2021, 12:11 PM
Harbormaster completed remote builds in B108868: Diff 351522.Jun 11 2021, 12:36 PM
ormris removed a subscriber: ormris.Jun 11 2021, 2:49 PM
hiraditya added inline comments.Jun 13 2021, 12:47 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
1583

Do we need to split when the region is going to be ignored?

AndrewLitteken added inline comments.Jun 13 2021, 3:01 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
1583

There's a chance that when the region is split, extra instructions are added changing the possibility of outlining, and it is no longer viable to be split. It might be better to have this function return a boolean if it has been split, or has not been, then continue based on that result.

AndrewLitteken updated this revision to Diff 351899.Jun 14 2021, 9:41 AM

Update splitCandidates to escape early if the region cannot be extracted, and checking the boolean CandidateSplit for the region rather than using IgnoreRegion,

Harbormaster completed remote builds in B109123: Diff 351899.Jun 14 2021, 11:03 AM
paquette added a comment.Jun 30 2021, 5:10 PM

Can you add context to the diff?

AndrewLitteken updated this revision to Diff 355907.Jul 1 2021, 9:20 AM

Adding context to the diff.

Harbormaster completed remote builds in B112008: Diff 355907.Jul 1 2021, 10:29 AM
paquette added inline comments.Jul 1 2021, 12:51 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
132

Move EndInst above and use it here?

Also, this could use a comment.

1661

Could use a comment explaining why this wouldn't happen.

llvm/test/Transforms/IROutliner/outlining-bitcasts.ll
40

Why did these lines disappear?

53
53

Sounds like "Outlining only occurs in this case due to the lack of a cost model" should be a FIXME?

AndrewLitteken added inline comments.Jul 2 2021, 7:27 AM
llvm/test/Transforms/IROutliner/outlining-bitcasts.ll
40

They move down to the bottom of the test, I think this was a function of the test generation script.

53

It's not really a fix me, the no cost command line argument is here to test these sorts of cases, so that it's not dependent on the way that the cost of outlining is calculated, rather how we define similarity. I can make this more clear.

AndrewLitteken updated this revision to Diff 356177.Jul 2 2021, 8:23 AM

Adding comments and fixes.

Harbormaster completed remote builds in B112200: Diff 356177.Jul 2 2021, 8:57 AM
AndrewLitteken added a child revision: D104143: [IRSim][IROutliner] Canonicalizing commutative value numbering between similarity sections..Jul 28 2021, 1:45 PM
AndrewLitteken removed a child revision: D104143: [IRSim][IROutliner] Canonicalizing commutative value numbering between similarity sections..Jul 28 2021, 2:13 PM
AndrewLitteken added a parent revision: D104143: [IRSim][IROutliner] Canonicalizing commutative value numbering between similarity sections..
AndrewLitteken added a child revision: D106440: [IROutliner] Change Prioritization of Outlining to honor cost model.Jul 28 2021, 2:15 PM
AndrewLitteken updated this revision to Diff 362547.Jul 28 2021, 2:52 PM
Harbormaster completed remote builds in B116810: Diff 362547.Jul 28 2021, 5:30 PM
AndrewLitteken updated this revision to Diff 367854.Aug 20 2021, 12:03 PM
AndrewLitteken edited the summary of this revision. (Show Details)

Updating based on feedback, and adding extra comments.

Harbormaster completed remote builds in B120599: Diff 367854.Aug 20 2021, 12:57 PM
paquette accepted this revision.Aug 25 2021, 1:36 PM

LGTM with nits

llvm/lib/Transforms/IPO/IROutliner.cpp
139

run-on sentence?

147

message should reflect the check

1700

run-on sentence?

llvm/test/Transforms/IROutliner/outlining-bitcasts.ll
53
This revision is now accepted and ready to land.Aug 25 2021, 1:36 PM
AndrewLitteken updated this revision to Diff 368729.Aug 25 2021, 2:21 PM

updating for nits.

Harbormaster completed remote builds in B121232: Diff 368729.Aug 25 2021, 3:08 PM
This revision was landed with ongoing or failed builds.Aug 30 2021, 9:31 AM
Closed by commit rGf564299fe94b: [IROutliner] Ensure instructions at end of candidate are excluded (authored by AndrewLitteken). · Explain Why
This revision was automatically updated to reflect the committed changes.
AndrewLitteken added a commit: rGf564299fe94b: [IROutliner] Ensure instructions at end of candidate are excluded.

Revision Contents

PathSize
llvm/
lib/
Transforms/
IPO/
7 lines
test/
Transforms/
IROutliner/
126 lines

Diff 351899

llvm/lib/Transforms/IPO/IROutliner.cpp

Context not available.
void OutlinableRegion::splitCandidate() { void OutlinableRegion::splitCandidate() {
assert(!CandidateSplit && "Candidate already split!"); assert(!CandidateSplit && "Candidate already split!");
if (Candidate->end()->Inst !=
Candidate->backInstruction()->getNextNonDebugInstruction())
return;
paquetteUnsubmitted
Not Done
ReplyInline Actions

Move EndInst above and use it here?

Also, this could use a comment.

paquette: Move EndInst above and use it here? Also, this could use a comment.
Instruction *StartInst = (*Candidate->begin()).Inst; Instruction *StartInst = (*Candidate->begin()).Inst;
Instruction *EndInst = (*Candidate->end()).Inst; Instruction *EndInst = (*Candidate->end()).Inst;
assert(StartInst && EndInst && "Expected a start and end instruction?"); assert(StartInst && EndInst && "Expected a start and end instruction?");
paquetteUnsubmitted
Not Done
ReplyInline Actions

run-on sentence?

paquette: run-on sentence?
paquetteUnsubmitted
Not Done
ReplyInline Actions
Instruction *StartInst = (*Candidate->begin()).Inst;
- assert(StartInst && EndInst && "Expected a start instruction?");
+ assert(StartInst && EndInst && "Expected start and end instructions?");
StartBB = StartInst->getParent();

message should reflect the check

paquette: message should reflect the check
Context not available.
// Break the outlinable region out of its parent BasicBlock into its own // Break the outlinable region out of its parent BasicBlock into its own
// BasicBlocks (see function implementation). // BasicBlocks (see function implementation).
OS->splitCandidate(); OS->splitCandidate();
if (!OS->CandidateSplit)
continue;
hiradityaUnsubmitted
Not Done
ReplyInline Actions

Do we need to split when the region is going to be ignored?

hiraditya: Do we need to split when the region is going to be ignored?
AndrewLittekenAuthorUnsubmitted
Done
ReplyInline Actions

There's a chance that when the region is split, extra instructions are added changing the possibility of outlining, and it is no longer viable to be split. It might be better to have this function return a boolean if it has been split, or has not been, then continue based on that result.

AndrewLitteken: There's a chance that when the region is split, extra instructions are added changing the…
std::vector<BasicBlock *> BE = {OS->StartBB}; std::vector<BasicBlock *> BE = {OS->StartBB};
OS->CE = new (ExtractorAllocator.Allocate()) OS->CE = new (ExtractorAllocator.Allocate())
CodeExtractor(BE, nullptr, false, nullptr, nullptr, nullptr, false, CodeExtractor(BE, nullptr, false, nullptr, nullptr, nullptr, false,
Context not available.
paquetteUnsubmitted
Not Done
ReplyInline Actions

Could use a comment explaining why this wouldn't happen.

paquette: Could use a comment explaining why this wouldn't happen.
paquetteUnsubmitted
Not Done
ReplyInline Actions

run-on sentence?

paquette: run-on sentence?

llvm/test/Transforms/IROutliner/outlining-bitcasts.ll

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --include-generated-funcs
; RUN: opt -S -verify -iroutliner -ir-outlining-no-cost < %s | FileCheck %s ; RUN: opt -S -verify -iroutliner -ir-outlining-no-cost < %s | FileCheck %s
; This test ensures that an extra output is not added when there is a bitcast ; This test ensures that an extra output is not added when there is a bitcast
Context not available.
declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
define void @outline_bitcast_base() { define void @outline_bitcast_base() {
; CHECK-LABEL: @outline_bitcast_base(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[B:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[C:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[D:%.*]] = alloca i32, align 4
; CHECK-NEXT: call void @[[FUNCTION_0:.*]](i32* [[A]], i32* [[B]], i32* [[C]], i32* [[D]])
; CHECK-NEXT: ret void
;
entry: entry:
%a = alloca i32, align 4 %a = alloca i32, align 4
%b = alloca i32, align 4 %b = alloca i32, align 4
Context not available.
} }
define void @outline_bitcast_removed() { define void @outline_bitcast_removed() {
; CHECK-LABEL: @outline_bitcast_removed(
paquetteUnsubmitted
Not Done
ReplyInline Actions

Why did these lines disappear?

paquette: Why did these lines disappear?
AndrewLittekenAuthorUnsubmitted
Done
ReplyInline Actions

They move down to the bottom of the test, I think this was a function of the test generation script.

AndrewLitteken: They move down to the bottom of the test, I think this was a function of the test generation…
; CHECK-NEXT: entry:
; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[B:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[C:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[D:%.*]] = alloca i32, align 4
; CHECK-NEXT: call void @[[FUNCTION_0:.*]](i32* [[A]], i32* [[B]], i32* [[C]], i32* [[D]])
; CHECK-NEXT: [[LT_CAST1:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 -1, i8* [[LT_CAST1]])
; CHECK-NEXT: [[AM:%.*]] = load i32, i32* [[B]], align 4
; CHECK-NEXT: [[BM:%.*]] = load i32, i32* [[A]], align 4
; CHECK-NEXT: [[CM:%.*]] = load i32, i32* [[C]], align 4
; CHECK-NEXT: [[AS:%.*]] = add i32 [[AM]], [[BM]]
; CHECK-NEXT: [[BS:%.*]] = add i32 [[BM]], [[AM]]
; CHECK-NEXT: [[CS:%.*]] = add i32 [[BM]], [[CM]]
; CHECK-NEXT: [[LT_CAST:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 -1, i8* [[LT_CAST]])
; CHECK-NEXT: ret void
;
entry: entry:
%a = alloca i32, align 4 %a = alloca i32, align 4
%b = alloca i32, align 4 %b = alloca i32, align 4
Context not available.
%am = load i32, i32* %b %am = load i32, i32* %b
%bm = load i32, i32* %a %bm = load i32, i32* %a
%cm = load i32, i32* %c %cm = load i32, i32* %c
%as = add i32 %am, %bm
%bs = add i32 %bm, %am
%cs = add i32 %bm, %cm
call void @llvm.lifetime.end.p0i8(i64 -1, i8* %X) call void @llvm.lifetime.end.p0i8(i64 -1, i8* %X)
ret void ret void
} }
; The first bitcast is moved down to lifetime start, and, since the original
; endpoint does not match the new endpoint, we cannot extract and outline the
; second bitcast and set of ads. Outlining only occurs in this case due to
paquetteUnsubmitted
Not Done
ReplyInline Actions
; endpoint does not match the new endpoint, we cannot extract and outline the
- ; second bitcast and set of ads. Outlining only occurs in this case due to
+ ; second bitcast and set of adds. Outlining only occurs in this case due to
; the lack of a cost model
paquette:
paquetteUnsubmitted
Not Done
ReplyInline Actions

Sounds like "Outlining only occurs in this case due to the lack of a cost model" should be a FIXME?

paquette: Sounds like "Outlining only occurs in this case due to the lack of a cost model" should be a…
AndrewLittekenAuthorUnsubmitted
Done
ReplyInline Actions

It's not really a fix me, the no cost command line argument is here to test these sorts of cases, so that it's not dependent on the way that the cost of outlining is calculated, rather how we define similarity. I can make this more clear.

AndrewLitteken: It's not really a fix me, the no cost command line argument is here to test these sorts of…
paquetteUnsubmitted
Not Done
ReplyInline Actions
; endpoint does not match the new endpoint, we cannot extract and outline the
- ; second bitcast and set of ads. Outlining only occurs in this case due to
+ ; second bitcast and set of adds. Outlining only occurs in this case due to
; the lack of a cost model, as denoted by the debug command line argument.
paquette:
; the lack of a cost model
define void @outline_bitcast_base2(i32 %a, i32 %b, i32 %c) { define void @outline_bitcast_base2(i32 %a, i32 %b, i32 %c) {
; CHECK-LABEL: @outline_bitcast_base2(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AL:%.*]] = add i32 [[A:%.*]], [[B:%.*]]
; CHECK-NEXT: [[BL:%.*]] = add i32 [[B]], [[A]]
; CHECK-NEXT: [[CL:%.*]] = add i32 [[B]], [[C:%.*]]
; CHECK-NEXT: ret void
;
entry: entry:
%d = alloca i32, align 4
%X = bitcast i32* %d to i8*
%al = add i32 %a, %b %al = add i32 %a, %b
%bl = add i32 %b, %a %bl = add i32 %b, %a
%cl = add i32 %b, %c %cl = add i32 %b, %c
%buffer = mul i32 %a, %b
%Y = bitcast i32* %d to i8*
%am = add i32 %a, %b
%bm = add i32 %b, %a
%cm = add i32 %b, %c
call void @llvm.lifetime.start.p0i8(i64 -1, i8* %X)
call void @llvm.lifetime.end.p0i8(i64 -1, i8* %X)
ret void ret void
} }
; CHECK: define internal void @[[FUNCTION_0]](i32* [[ARG0:%.*]], i32* [[ARG1:%.*]], i32* [[ARG2:%.*]], i32* [[ARG3:%.*]]) ; CHECK-LABEL: @outline_bitcast_base(
; CHECK: entry_to_outline: ; CHECK-NEXT: entry:
; CHECK-NEXT: store i32 2, i32* [[ARG0]], align 4 ; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4
; CHECK-NEXT: store i32 3, i32* [[ARG1]], align 4 ; CHECK-NEXT: [[B:%.*]] = alloca i32, align 4
; CHECK-NEXT: store i32 4, i32* [[ARG2]], align 4 ; CHECK-NEXT: [[C:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[X:%.*]] = bitcast i32* [[ARG3]] to i8* ; CHECK-NEXT: [[D:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[AL:%.*]] = load i32, i32* [[ARG0]], align 4 ; CHECK-NEXT: call void @outlined_ir_func_0(i32* [[A]], i32* [[B]], i32* [[C]], i32* [[D]])
; CHECK-NEXT: [[BL:%.*]] = load i32, i32* [[ARG1]], align 4 ; CHECK-NEXT: ret void
; CHECK-NEXT: [[CL:%.*]] = load i32, i32* [[ARG2]], align 4 ;
;
; CHECK-LABEL: @outline_bitcast_removed(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[B:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[C:%.*]] = alloca i32, align 4
; CHECK-NEXT: [[D:%.*]] = alloca i32, align 4
; CHECK-NEXT: call void @outlined_ir_func_0(i32* [[A]], i32* [[B]], i32* [[C]], i32* [[D]])
; CHECK-NEXT: [[LT_CAST1:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 -1, i8* [[LT_CAST1]])
; CHECK-NEXT: [[AM:%.*]] = load i32, i32* [[B]], align 4
; CHECK-NEXT: [[BM:%.*]] = load i32, i32* [[A]], align 4
; CHECK-NEXT: [[CM:%.*]] = load i32, i32* [[C]], align 4
; CHECK-NEXT: [[LT_CAST:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 -1, i8* [[LT_CAST]])
; CHECK-NEXT: ret void
;
;
; CHECK-LABEL: @outline_bitcast_base2(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[D:%.*]] = alloca i32, align 4
; CHECK-NEXT: call void @outlined_ir_func_1(i32* [[D]], i32 [[A:%.*]], i32 [[B:%.*]], i32 [[C:%.*]])
; CHECK-NEXT: [[BUFFER:%.*]] = mul i32 [[A]], [[B]]
; CHECK-NEXT: [[Y:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: [[AM:%.*]] = add i32 [[A]], [[B]]
; CHECK-NEXT: [[BM:%.*]] = add i32 [[B]], [[A]]
; CHECK-NEXT: [[CM:%.*]] = add i32 [[B]], [[C]]
; CHECK-NEXT: [[LT_CAST1:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 -1, i8* [[LT_CAST1]])
; CHECK-NEXT: [[LT_CAST:%.*]] = bitcast i32* [[D]] to i8*
; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 -1, i8* [[LT_CAST]])
; CHECK-NEXT: ret void
;
;
; CHECK: @outlined_ir_func_0(i32* [[TMP0:%.*]], i32* [[TMP1:%.*]], i32* [[TMP2:%.*]], i32* [[TMP3:%.*]])
; CHECK: entry_to_outline:
; CHECK-NEXT: store i32 2, i32* [[TMP0:%.*]], align 4
; CHECK-NEXT: store i32 3, i32* [[TMP1:%.*]], align 4
; CHECK-NEXT: store i32 4, i32* [[TMP2:%.*]], align 4
; CHECK-NEXT: [[X:%.*]] = bitcast i32* [[TMP3:%.*]] to i8*
; CHECK-NEXT: [[AL:%.*]] = load i32, i32* [[TMP0]], align 4
; CHECK-NEXT: [[BL:%.*]] = load i32, i32* [[TMP1]], align 4
; CHECK-NEXT: [[CL:%.*]] = load i32, i32* [[TMP2]], align 4
; CHECK-NEXT: br label [[ENTRY_AFTER_OUTLINE_EXITSTUB:%.*]]
;
;
; CHECK: @outlined_ir_func_1(i32* [[TMP0:%.*]], i32 [[TMP1:%.*]], i32 [[TMP2:%.*]], i32 [[TMP3:%.*]])
; CHECK: entry_to_outline:
; CHECK-NEXT: [[X:%.*]] = bitcast i32* [[TMP0]] to i8*
; CHECK-NEXT: [[AL:%.*]] = add i32 [[TMP1]], [[TMP2]]
; CHECK-NEXT: [[BL:%.*]] = add i32 [[TMP2]], [[TMP1]]
; CHECK-NEXT: [[CL:%.*]] = add i32 [[TMP2]], [[TMP3]]
; CHECK-NEXT: br label [[ENTRY_AFTER_OUTLINE_EXITSTUB:%.*]]
;
Context not available.