This is an archive of the discontinued LLVM Phabricator instance.

Differential D79217

Process gep (select ptr1, ptr2) in SROA
ClosedPublic

Authored by rampitec on Apr 30 2020, 4:55 PM.

Details

Reviewers
arsenm
chandlerc
jingyue
Commits
rG42725aeed8cb: Process gep (select ptr1, ptr2) in SROA

Diff Detail

Event Timeline

rampitec created this revision.Apr 30 2020, 4:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 4:55 PM
Herald added subscribers: hiraditya, wdng. · View Herald Transcript
rampitec mentioned this in D79145: Allow gep (select) combine with non-constant operands.Apr 30 2020, 5:06 PM
rampitec added a reviewer: chandlerc.May 4 2020, 8:40 AM
arsenm added inline comments.May 5 2020, 12:41 PM
llvm/lib/Transforms/Scalar/SROA.cpp
3512

Needs a newline after?

llvm/test/Transforms/SROA/select-gep.ll
16

Store something for non-undef test output?

18

This increases the instruction count if the load won't be eliminated. Can you add a test with a volatile load or something else to prevent it?

rampitec updated this revision to Diff 262227.May 5 2020, 2:26 PM
rampitec marked 4 inline comments as done.

Addressed review comments.

llvm/test/Transforms/SROA/select-gep.ll
18

It may increase instruction count, but if it will remain after SROA there is a pattern in InstCombine to revert it back. That is actually the reason why D79145 was over complicated and I have switched to processing it right in SROA.

rampitec added a comment.May 6 2020, 3:36 PM

PSDB passed.

rampitec updated this revision to Diff 262726.May 7 2020, 12:15 PM

Added one more test for select gep (select gep, gep), gep (select gep, gep).

rampitec added a comment.May 11 2020, 8:31 AM

Ping.

rampitec added a reviewer: jingyue.May 14 2020, 2:56 PM
arsenm added inline comments.May 14 2020, 3:27 PM
llvm/lib/Transforms/Scalar/SROA.cpp
3509

Early exit if not select and reduce indentation

rampitec marked an inline comment as done.May 14 2020, 3:53 PM
rampitec added inline comments.
llvm/lib/Transforms/Scalar/SROA.cpp
3509

I will have to return this if anyway after D79218. There is one if for Select and one for PHI.

rampitec marked an inline comment as done.May 14 2020, 4:01 PM
rampitec added inline comments.
llvm/lib/Transforms/Scalar/SROA.cpp
3509

I will extract it into a separate function, here and in another patch.

rampitec updated this revision to Diff 264124.May 14 2020, 4:11 PM
rampitec marked an inline comment as done.

Extracted folding into a separate function.

arsenm added inline comments.May 14 2020, 5:37 PM
llvm/lib/Transforms/Scalar/SROA.cpp
3480

The inbounds vs. noninbounds paths aren't stressed in the test

rampitec updated this revision to Diff 264267.May 15 2020, 9:49 AM
rampitec marked an inline comment as done.

Added a non-inbound GEP test.

arsenm added inline comments.May 18 2020, 5:55 PM
llvm/lib/Transforms/Scalar/SROA.cpp
3476

Probably should check there are only constant indexes?

llvm/test/Transforms/SROA/select-gep.ll
7

Should have a test with non-constant GEP indices. This probably doesn't help if they aren't constants

rampitec updated this revision to Diff 264953.May 19 2020, 10:14 AM
rampitec marked 2 inline comments as done.

Added check for constant indexes and test.

rampitec added a comment.May 21 2020, 10:23 AM

Ping! @chandlerc do you mind to take a look? Thanks in advance!

arsenm accepted this revision.May 26 2020, 12:32 PM

lgtm

This revision is now accepted and ready to land.May 26 2020, 12:32 PM
Closed by commit rG42725aeed8cb: Process gep (select ptr1, ptr2) in SROA (authored by rampitec). · Explain WhyMay 26 2020, 1:06 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
Transforms/
Scalar/
47 lines
test/
Transforms/
SROA/
103 lines

Diff 264124

llvm/lib/Transforms/Scalar/SROA.cpp

Show First 20 Lines • Show All 3,456 Lines • ▼ Show 20 Lines bool visitBitCastInst(BitCastInst &BC) {
return false; return false;
} }
bool visitAddrSpaceCastInst(AddrSpaceCastInst &ASC) { bool visitAddrSpaceCastInst(AddrSpaceCastInst &ASC) {
enqueueUsers(ASC); enqueueUsers(ASC);
return false; return false;
} }
// Fold gep (select cond, ptr1, ptr2) => select cond, gep(ptr1), gep(ptr2)
void foldGEPSelect(GetElementPtrInst &GEPI) {
SelectInst *Sel = cast<SelectInst>(GEPI.getPointerOperand());
LLVM_DEBUG(dbgs() << " Rewriting gep(select) -> select(gep):"
<< "\n original: " << *Sel
<< "\n " << GEPI);
IRBuilderTy Builder(&GEPI);
SmallVector<Value *, 4> Index(GEPI.idx_begin(), GEPI.idx_end());
bool IsInBounds = GEPI.isInBounds();
arsenmUnsubmitted
Done
ReplyInline Actions

Probably should check there are only constant indexes?

arsenm: Probably should check there are only constant indexes?
Value *True = Sel->getTrueValue();
Value *NTrue =
IsInBounds
? Builder.CreateInBoundsGEP(True, Index,
arsenmUnsubmitted
Done
ReplyInline Actions

The inbounds vs. noninbounds paths aren't stressed in the test

arsenm: The inbounds vs. noninbounds paths aren't stressed in the test
True->getName() + ".sroa.gep")
: Builder.CreateGEP(True, Index, True->getName() + ".sroa.gep");
Value *False = Sel->getFalseValue();
Value *NFalse =
IsInBounds
? Builder.CreateInBoundsGEP(False, Index,
False->getName() + ".sroa.gep")
: Builder.CreateGEP(False, Index, False->getName() + ".sroa.gep");
Value *NSel = Builder.CreateSelect(Sel->getCondition(), NTrue, NFalse,
Sel->getName() + ".sroa.sel");
GEPI.replaceAllUsesWith(NSel);
GEPI.eraseFromParent();
LLVM_DEBUG(dbgs() << "\n to: " << *NTrue
<< "\n " << *NFalse
<< "\n " << *NSel << '\n');
if (isa<Instruction>(NTrue))
visit(cast<Instruction>(NTrue));
if (isa<Instruction>(NFalse))
visit(cast<Instruction>(NFalse));
}
bool visitGetElementPtrInst(GetElementPtrInst &GEPI) { bool visitGetElementPtrInst(GetElementPtrInst &GEPI) {
if (isa<SelectInst>(GEPI.getPointerOperand())) {
foldGEPSelect(GEPI);
arsenmUnsubmitted
Done
ReplyInline Actions

Early exit if not select and reduce indentation

arsenm: Early exit if not select and reduce indentation
rampitecAuthorUnsubmitted
Done
ReplyInline Actions

I will have to return this if anyway after D79218. There is one if for Select and one for PHI.

rampitec: I will have to return this if anyway after D79218. There is one if for Select and one for PHI.
rampitecAuthorUnsubmitted
Done
ReplyInline Actions

I will extract it into a separate function, here and in another patch.

rampitec: I will extract it into a separate function, here and in another patch.
return true;
}
arsenmUnsubmitted
Done
ReplyInline Actions

Needs a newline after?

arsenm: Needs a newline after?
enqueueUsers(GEPI); enqueueUsers(GEPI);
return false; return false;
} }
bool visitPHINode(PHINode &PN) { bool visitPHINode(PHINode &PN) {
enqueueUsers(PN); enqueueUsers(PN);
return false; return false;
} }
▲ Show 20 Lines • Show All 1,228 Lines • Show Last 20 Lines

llvm/test/Transforms/SROA/select-gep.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -sroa < %s | FileCheck %s
%pair = type { i32, i32 }
define i32 @test_sroa_select_gep(i1 %cond) {
; CHECK-LABEL: @test_sroa_select_gep(
arsenmUnsubmitted
Done
ReplyInline Actions

Should have a test with non-constant GEP indices. This probably doesn't help if they aren't constants

arsenm: Should have a test with non-constant GEP indices. This probably doesn't help if they aren't…
; CHECK-NEXT: bb:
; CHECK-NEXT: [[LOAD_SROA_SPECULATED:%.*]] = select i1 [[COND:%.*]], i32 1, i32 2
; CHECK-NEXT: ret i32 [[LOAD_SROA_SPECULATED]]
;
bb:
%a = alloca %pair, align 4
%b = alloca %pair, align 4
%gep_a = getelementptr inbounds %pair, %pair* %a, i32 0, i32 1
%gep_b = getelementptr inbounds %pair, %pair* %b, i32 0, i32 1
arsenmUnsubmitted
Done
ReplyInline Actions

Store something for non-undef test output?

arsenm: Store something for non-undef test output?
store i32 1, i32* %gep_a, align 4
store i32 2, i32* %gep_b, align 4
arsenmUnsubmitted
Done
ReplyInline Actions

This increases the instruction count if the load won't be eliminated. Can you add a test with a volatile load or something else to prevent it?

arsenm: This increases the instruction count if the load won't be eliminated. Can you add a test with a…
rampitecAuthorUnsubmitted
Done
ReplyInline Actions

It may increase instruction count, but if it will remain after SROA there is a pattern in InstCombine to revert it back. That is actually the reason why D79145 was over complicated and I have switched to processing it right in SROA.

rampitec: It may increase instruction count, but if it will remain after SROA there is a pattern in…
%select = select i1 %cond, %pair* %a, %pair* %b
%gep = getelementptr inbounds %pair, %pair* %select, i32 0, i32 1
%load = load i32, i32* %gep, align 4
ret i32 %load
}
define i32 @test_sroa_select_gep_volatile_load(i1 %cond) {
; CHECK-LABEL: @test_sroa_select_gep_volatile_load(
; CHECK-NEXT: bb:
; CHECK-NEXT: [[A_SROA_0:%.*]] = alloca i32
; CHECK-NEXT: [[A_SROA_2:%.*]] = alloca i32
; CHECK-NEXT: [[B_SROA_0:%.*]] = alloca i32
; CHECK-NEXT: [[B_SROA_2:%.*]] = alloca i32
; CHECK-NEXT: store i32 11, i32* [[A_SROA_0]], align 4
; CHECK-NEXT: store i32 12, i32* [[B_SROA_0]], align 4
; CHECK-NEXT: store i32 21, i32* [[A_SROA_2]], align 4
; CHECK-NEXT: store i32 22, i32* [[B_SROA_2]], align 4
; CHECK-NEXT: [[SELECT_SROA_SEL:%.*]] = select i1 [[COND:%.*]], i32* [[A_SROA_0]], i32* [[B_SROA_0]]
; CHECK-NEXT: [[LOAD1:%.*]] = load volatile i32, i32* [[SELECT_SROA_SEL]], align 4
; CHECK-NEXT: [[SELECT_SROA_SEL3:%.*]] = select i1 [[COND]], i32* [[A_SROA_2]], i32* [[B_SROA_2]]
; CHECK-NEXT: [[LOAD2:%.*]] = load volatile i32, i32* [[SELECT_SROA_SEL3]], align 4
; CHECK-NEXT: [[ADD:%.*]] = add i32 [[LOAD1]], [[LOAD2]]
; CHECK-NEXT: ret i32 [[ADD]]
;
bb:
%a = alloca %pair, align 4
%b = alloca %pair, align 4
%gep_a0 = getelementptr inbounds %pair, %pair* %a, i32 0, i32 0
%gep_b0 = getelementptr inbounds %pair, %pair* %b, i32 0, i32 0
store i32 11, i32* %gep_a0, align 4
store i32 12, i32* %gep_b0, align 4
%gep_a1 = getelementptr inbounds %pair, %pair* %a, i32 0, i32 1
%gep_b1 = getelementptr inbounds %pair, %pair* %b, i32 0, i32 1
store i32 21, i32* %gep_a1, align 4
store i32 22, i32* %gep_b1, align 4
%select = select i1 %cond, %pair* %a, %pair* %b
%gep1 = getelementptr inbounds %pair, %pair* %select, i32 0, i32 0
%load1 = load volatile i32, i32* %gep1, align 4
%gep2 = getelementptr inbounds %pair, %pair* %select, i32 0, i32 1
%load2 = load volatile i32, i32* %gep2, align 4
%add = add i32 %load1, %load2
ret i32 %add
}
define i32 @test_sroa_select_gep_undef(i1 %cond) {
; CHECK-LABEL: @test_sroa_select_gep_undef(
; CHECK-NEXT: bb:
; CHECK-NEXT: [[A_SROA_0:%.*]] = alloca i32
; CHECK-NEXT: [[SELECT_SROA_SEL:%.*]] = select i1 [[COND:%.*]], i32* [[A_SROA_0]], i32* undef
; CHECK-NEXT: [[LOAD:%.*]] = load i32, i32* [[SELECT_SROA_SEL]], align 4
; CHECK-NEXT: ret i32 [[LOAD]]
;
bb:
%a = alloca %pair, align 4
%select = select i1 %cond, %pair* %a, %pair* undef
%gep = getelementptr inbounds %pair, %pair* %select, i32 0, i32 1
%load = load i32, i32* %gep, align 4
ret i32 %load
}
define i32 @test_sroa_gep_select_gep(i1 %cond) {
; CHECK-LABEL: @test_sroa_gep_select_gep(
; CHECK-NEXT: bb:
; CHECK-NEXT: [[A_SROA_0:%.*]] = alloca i32
; CHECK-NEXT: [[B_SROA_0:%.*]] = alloca i32
; CHECK-NEXT: store i32 1, i32* [[A_SROA_0]], align 4
; CHECK-NEXT: store i32 2, i32* [[B_SROA_0]], align 4
; CHECK-NEXT: [[SELECT_SROA_SEL:%.*]] = select i1 [[COND:%.*]], i32* [[A_SROA_0]], i32* [[B_SROA_0]]
; CHECK-NEXT: [[SELECT2:%.*]] = select i1 [[COND]], i32* [[SELECT_SROA_SEL]], i32* [[A_SROA_0]]
; CHECK-NEXT: [[LOAD:%.*]] = load i32, i32* [[SELECT2]], align 4
; CHECK-NEXT: ret i32 [[LOAD]]
;
bb:
%a = alloca %pair, align 4
%b = alloca %pair, align 4
%gep_a = getelementptr inbounds %pair, %pair* %a, i32 0, i32 1
%gep_b = getelementptr inbounds %pair, %pair* %b, i32 0, i32 1
store i32 1, i32* %gep_a, align 4
store i32 2, i32* %gep_b, align 4
%select = select i1 %cond, i32* %gep_a, i32* %gep_b
%gep = getelementptr inbounds i32, i32* %select, i32 0
%select2 = select i1 %cond, i32* %gep, i32* %gep_a
%load = load i32, i32* %select2, align 4
ret i32 %load
}