This is an archive of the discontinued LLVM Phabricator instance.

Differential D68004

[InstCombine] Fix call guard difference with dbg
ClosedPublic

Authored by yechunliang on Sep 24 2019, 6:44 PM.

Details

Reviewers
davide
reames
vsk
apilipenko
Commits
rGc32f0ff92f02: [InstCombine] Fix call guard difference with dbg
Summary

GuardWideningWindow() in InstCombine pass is used to bypass looking for another guard, the initial value is "3" which does not consider the case with dbg.value, if dbg instructions exist, it will exceed the guard widening window and the another guard will be missed.

The purpose of this patch is to skip debug instructions while looking for the guard in the window, use getNextNonDebugInstruction instead of previous getNextNode.

Fix for https://bugs.llvm.org/show_bug.cgi?id=37714

Diff Detail

Event Timeline

yechunliang created this revision.Sep 24 2019, 6:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 6:44 PM
Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript
apilipenko added a comment.Sep 24 2019, 7:08 PM

We should just skip debug instructions while looking for the guard in the window. Just use getNextNonDebugInstruction instead of current getNextNode.

yechunliang updated this revision to Diff 221659.Sep 24 2019, 7:44 PM

[InstCombine] Fix call guard difference with dbg

While using "opt -debugify-each", the dbg instructions will exceed GuardWideningWindow, so that the instructions will be different with and without "-debugify-each" after optimization. We should just skip debug instructions while looking for the guard in the window. Just use getNextNonDebugInstruction instead of current getNextNode.
lebedev.ri added a subscriber: lebedev.ri.Sep 25 2019, 1:13 AM
In D68004#1681802, @apilipenko wrote:

We should just skip debug instructions while looking for the guard in the window. Just use getNextNonDebugInstruction instead of current getNextNode.

+1. Also needs a test?

apilipenko added a comment.Sep 25 2019, 3:57 PM

+1 to a test.

davide requested changes to this revision.Sep 25 2019, 4:18 PM

yes, this needs a test.

This revision now requires changes to proceed.Sep 25 2019, 4:18 PM
yechunliang updated this revision to Diff 221878.Sep 25 2019, 7:03 PM

thanks for review. add test case with --debugify-each based on previous call-guard.ll

yechunliang updated this revision to Diff 223828.Oct 8 2019, 5:24 AM

Added one more RUN command line with "-debugify-each" in existed call-guard.ll, needn't duplicate another same test case file.

yechunliang added a comment.Oct 8 2019, 5:25 AM

just added updated test, could some one help to review again? thanks.

apilipenko accepted this revision.Oct 28 2019, 3:37 PM
yechunliang added a comment.Oct 29 2019, 6:35 PM

@apilipenko thanks for accepting, I have no submit permission, would you please also help me to submit this patch? thanks.

yechunliang edited the summary of this revision. (Show Details)Oct 29 2019, 6:46 PM
yechunliang added a comment.Nov 22 2019, 5:23 AM

ping

davide added a comment.Nov 22 2019, 10:58 AM

I'm committing this for you.

davide added a comment.Nov 22 2019, 1:36 PM
commit c32f0ff92f024a8af438fc8d85906e441b5a2682 (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
Date:   Fri Nov 22 13:02:18 2019 -0800

    [InstCombine] Fix call guard difference with dbg
    
    Patch by Chris Ye!
    
    Differential Revision: https://reviews.llvm.org/D68004
This revision was not accepted when it landed; it landed in state Needs Review.Nov 22 2019, 1:39 PM
Closed by commit rGc32f0ff92f02: [InstCombine] Fix call guard difference with dbg (authored by davide). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
Transforms/
InstCombine/
8 lines
test/
Transforms/
InstCombine/
1 line

Diff 230717

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Show First 20 Lines • Show All 4,054 Lines • ▼ Show 20 Lines case Intrinsic::experimental_gc_relocate: {
// TODO: relocate((gep p, C, C2, ...)) -> gep(relocate(p), C, C2, ...) // TODO: relocate((gep p, C, C2, ...)) -> gep(relocate(p), C, C2, ...)
break; break;
} }
case Intrinsic::experimental_guard: { case Intrinsic::experimental_guard: {
// Is this guard followed by another guard? We scan forward over a small // Is this guard followed by another guard? We scan forward over a small
// fixed window of instructions to handle common cases with conditions // fixed window of instructions to handle common cases with conditions
// computed between guards. // computed between guards.
Instruction *NextInst = II->getNextNode(); Instruction *NextInst = II->getNextNonDebugInstruction();
for (unsigned i = 0; i < GuardWideningWindow; i++) { for (unsigned i = 0; i < GuardWideningWindow; i++) {
// Note: Using context-free form to avoid compile time blow up // Note: Using context-free form to avoid compile time blow up
if (!isSafeToSpeculativelyExecute(NextInst)) if (!isSafeToSpeculativelyExecute(NextInst))
break; break;
NextInst = NextInst->getNextNode(); NextInst = NextInst->getNextNonDebugInstruction();
} }
Value *NextCond = nullptr; Value *NextCond = nullptr;
if (match(NextInst, if (match(NextInst,
m_Intrinsic<Intrinsic::experimental_guard>(m_Value(NextCond)))) { m_Intrinsic<Intrinsic::experimental_guard>(m_Value(NextCond)))) {
Value *CurrCond = II->getArgOperand(0); Value *CurrCond = II->getArgOperand(0);
// Remove a guard that it is immediately preceded by an identical guard. // Remove a guard that it is immediately preceded by an identical guard.
if (CurrCond == NextCond) if (CurrCond == NextCond)
return eraseInstFromFunction(*NextInst); return eraseInstFromFunction(*NextInst);
// Otherwise canonicalize guard(a); guard(b) -> guard(a & b). // Otherwise canonicalize guard(a); guard(b) -> guard(a & b).
Instruction* MoveI = II->getNextNode(); Instruction *MoveI = II->getNextNonDebugInstruction();
while (MoveI != NextInst) { while (MoveI != NextInst) {
auto *Temp = MoveI; auto *Temp = MoveI;
MoveI = MoveI->getNextNode(); MoveI = MoveI->getNextNonDebugInstruction();
Temp->moveBefore(II); Temp->moveBefore(II);
} }
II->setArgOperand(0, Builder.CreateAnd(CurrCond, NextCond)); II->setArgOperand(0, Builder.CreateAnd(CurrCond, NextCond));
return eraseInstFromFunction(*NextInst); return eraseInstFromFunction(*NextInst);
} }
break; break;
} }
} }
▲ Show 20 Lines • Show All 776 Lines • Show Last 20 Lines

llvm/test/Transforms/InstCombine/call-guard.ll

; RUN: opt < %s -instcombine -S | FileCheck %s ; RUN: opt < %s -instcombine -S | FileCheck %s
; RUN: opt < %s -instcombine -S -debugify-each | FileCheck %s
declare void @llvm.experimental.guard(i1, ...) declare void @llvm.experimental.guard(i1, ...)
define void @test_guard_adjacent_same_cond(i1 %A) { define void @test_guard_adjacent_same_cond(i1 %A) {
; CHECK-LABEL: @test_guard_adjacent_same_cond( ; CHECK-LABEL: @test_guard_adjacent_same_cond(
; CHECK-NEXT: call void (i1, ...) @llvm.experimental.guard(i1 %A) [ "deopt"() ] ; CHECK-NEXT: call void (i1, ...) @llvm.experimental.guard(i1 %A) [ "deopt"() ]
; CHECK-NEXT: ret void ; CHECK-NEXT: ret void
call void(i1, ...) @llvm.experimental.guard( i1 %A )[ "deopt"() ] call void(i1, ...) @llvm.experimental.guard( i1 %A )[ "deopt"() ]
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines