This is an archive of the discontinued LLVM Phabricator instance.

Differential D65542

[PeepholeOptimizer] Don't assume bitcast def always has input
ClosedPublic

Authored by jsji on Jul 31 2019, 2:42 PM.

Details

Reviewers
qcolombet
MatzeB
hfinkel
arsenm
Commits
rG0776da5236e0: [PeepholeOptimizer] Don't assume bitcast def always has input
rL369261: [PeepholeOptimizer] Don't assume bitcast def always has input
Summary

If we have a MI marked with bitcast bits, but without input operands,
PeepholeOptimizer might crash with assert.

eg:
If we apply the changes in PPCInstrVSX.td as in this patch:

[(set v4i32:$XT, (bitconvert (v16i8 immAllOnesV)))]>;

We will get assert in PeepholeOptimizer.

llvm-lit llvm-project/llvm/test/CodeGen/PowerPC/build-vector-tests.ll -v

llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:417: const
llvm::MachineOperand &llvm::MachineInstr::getOperand(unsigned int)
const: Assertion `i < getNumOperands() && "getOperand() out of range!"'
failed.

The fix is to abort if we found out of bound access.

Diff Detail

Repository
rL LLVM

Event Timeline

jsji created this revision.Jul 31 2019, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 2:42 PM
Herald added subscribers: llvm-commits, MaskRay, kbarton and 2 others. · View Herald Transcript
Harbormaster completed remote builds in B35919: Diff 212670.Jul 31 2019, 2:46 PM
Herald added a subscriber: • wuzish. · View Herald TranscriptJul 31 2019, 2:46 PM
jsji added a comment.Aug 12 2019, 8:37 AM

Ping..

qcolombet added a comment.Aug 13 2019, 10:00 AM

LGTM with a test case.

llvm/lib/CodeGen/PeepholeOptimizer.cpp
1858 ↗(On Diff #212670)

Nit: Def has no input.

(remove "the" and the t from "not")

jsji updated this revision to Diff 214917.Aug 13 2019, 2:12 PM

Fix comments and add a testcase.

Harbormaster completed remote builds in B36695: Diff 214917.Aug 13 2019, 2:13 PM
jsji added a parent revision: D65529: [PowerPC] Use xxleqv to set all one vector IMM(-1)..Aug 13 2019, 2:14 PM
jsji updated this revision to Diff 215423.Aug 15 2019, 10:04 AM

Rebased to latest ToT.

Herald added a subscriber: steven.zhang. · View Herald TranscriptAug 15 2019, 10:04 AM
Harbormaster completed remote builds in B36835: Diff 215423.Aug 15 2019, 10:05 AM
jsji added a comment.Aug 15 2019, 10:05 AM

@qcolombet Can you have a quick look at the testcase to see whether this is OK? Thanks.

arsenm added a subscriber: arsenm.Aug 15 2019, 10:07 AM
arsenm added inline comments.
llvm/test/CodeGen/PowerPC/bitcast-peelhole.ll
1 ↗(On Diff #215423)

Typo in testname: peelhole

13 ↗(On Diff #215423)

I would expect this to go in a pre-existing test, and to add a MIR test for this specifically

jsji updated this revision to Diff 215434.Aug 15 2019, 10:37 AM

Using MIR test instead.

Harbormaster completed remote builds in B36841: Diff 215434.Aug 15 2019, 10:37 AM
jsji added a reviewer: arsenm.Aug 15 2019, 10:38 AM

@arsenm Is this MIR test OK?

Herald added a subscriber: wdng. · View Herald TranscriptAug 15 2019, 10:38 AM
arsenm added a comment.Aug 15 2019, 10:46 AM
In D65542#1631857, @jsji wrote:

@arsenm Is this MIR test OK?

I think you should also keep the IR test, as it will still test the pattern change

jsji added a comment.Aug 15 2019, 10:51 AM
In D65542#1631884, @arsenm wrote:
In D65542#1631857, @jsji wrote:

@arsenm Is this MIR test OK?

I think you should also keep the IR test, as it will still test the pattern change

Sorry, I should have explained, the IR test has been included in llvm/test/CodeGen/PowerPC/build-vector-allones.ll.

qcolombet added inline comments.Aug 16 2019, 10:45 AM
llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
2 ↗(On Diff #215434)

Instead of requiring assertion, I would just explicitly check that the number of operands is correct.
You can use update_mir_test_check.py to generate the check lines for this.

qcolombet added inline comments.Aug 16 2019, 10:47 AM
llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
2 ↗(On Diff #215434)

Scratch my last comment on the number of operands!
Still would generating the CHECK lines with the update script would have caught the bug without having to rely on asserts?

jsji marked an inline comment as done.Aug 16 2019, 11:27 AM
jsji added inline comments.
llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
2 ↗(On Diff #215434)

I gave it a try. Unfotunately, no, we can't caught this without assert.

[~/build-nonassert] $ cat ../llvm-project/llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=powerpc64le-linux-gnu -run-pass=peephole-opt -verify-machineinstrs -o - %s | FileCheck %s

---
name:            bitCast
tracksRegLiveness: true
body:             |
  bb.0.entry:
    ; CHECK-LABEL: name: bitCast
    ; CHECK: [[XXLEQVOnes:%[0-9]+]]:vsrc = XXLEQVOnes
    ; CHECK: $v2 = COPY [[XXLEQVOnes]]
    ; CHECK: BLR8 implicit $lr8, implicit $rm, implicit $v2
    %0:vsrc = XXLEQVOnes
    $v2 = COPY %0
    BLR8 implicit $lr8, implicit $rm, implicit $v2

...

# This used to hit an assertion:
#   llvm/include/llvm/CodeGen/MachineInstr.h:417: const
#   llvm::MachineOperand &llvm::MachineInstr::getOperand(unsigned int)
#   const: Assertion `i < getNumOperands() && "getOperand() out of range!"' failed.
#
[~/build-nonassert] $ bin/llvm-lit ../llvm-project/llvm/test/CodeGen/PowerPC/bitcast-peephole.mir -v
-- Testing: 1 tests, single process --
PASS: LLVM :: CodeGen/PowerPC/bitcast-peephole.mir (1 of 1)
Testing Time: 0.04s
  Expected Passes    : 1

[~/build] $ bin/llvm-lit ../llvm-project/llvm/test/CodeGen/PowerPC/bitcast-peephole.mir 
-- Testing: 1 tests, single process --
FAIL: LLVM :: CodeGen/PowerPC/bitcast-peephole.mir (1 of 1)
Testing Time: 0.32s
********************
Failing Tests (1):
    LLVM :: CodeGen/PowerPC/bitcast-peephole.mir

  Unexpected Failures: 1
jsji updated this revision to Diff 215647.Aug 16 2019, 11:35 AM

Update the MIR test using script.

Harbormaster completed remote builds in B36892: Diff 215647.Aug 16 2019, 11:36 AM
qcolombet added inline comments.Aug 16 2019, 12:24 PM
llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
2 ↗(On Diff #215434)

I gave it a try. Unfotunately, no, we can't caught this without assert.

So we end up generating correct code by luck for this case?

jsji marked an inline comment as done.Aug 16 2019, 1:37 PM
jsji added inline comments.
llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
2 ↗(On Diff #215434)

Yes.

When we get getOperand(SrcIdx), getOperand(1), we are accessing out of bound memory,
but we are lucky to not triggering core dump, instead we get a garbage value,
then Src.isUndef() test become true, then we return ValueTrackerResult().
so we ended up generating correct code without problem in this case.

ASAN build can catch the problem though.

SUMMARY: AddressSanitizer: use-after-poison .../llvm-project/llvm/include/llvm/CodeGen/MachineOperand.h:390:12 in isUndef
Shadow bytes around the buggy address:
  0x1fe0fee91480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1fe0fee91490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1fe0fee914a0: 00 00 00 00 00 00 00 00 f7 f7 00 00 00 00 00 00
  0x1fe0fee914b0: 00 00 f7 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1fe0fee914c0: 00 00 00 00 00 00 00 00 00 00 00 00 f7 00 00 00
=>0x1fe0fee914d0: 00 00 00 00 00 00 f7 00 00 00 00[f7]00 00 00 00
  0x1fe0fee914e0: 00 00 00 00 00 f7 00 00 00 00 00 00 00 00 f7 00
  0x1fe0fee914f0: 00 00 00 00 00 00 00 00 f7 00 00 00 00 00 00 00
  0x1fe0fee91500: 00 f7 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1fe0fee91510: 00 00 f7 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1fe0fee91520: 00 00 00 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7

Do you think we can leave it to be caught by ASAN build instead of assert build?

qcolombet added inline comments.Aug 16 2019, 4:19 PM
llvm/test/CodeGen/PowerPC/bitcast-peephole.mir
2 ↗(On Diff #215434)

Let's leave the REQUIRE: assert then.

Thanks for double checking.

qcolombet accepted this revision.Aug 16 2019, 4:20 PM
This revision is now accepted and ready to land.Aug 16 2019, 4:20 PM
Closed by commit rL369261: [PeepholeOptimizer] Don't assume bitcast def always has input (authored by jsji). · Explain WhyAug 19 2019, 7:17 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
CodeGen/
5 lines
Target/
PowerPC/
4 lines
test/
CodeGen/
PowerPC/
23 lines

Diff 215889

llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp

Show First 20 Lines • Show All 1,847 Lines • ▼ Show 20 Lines if (MO.isImplicit() && MO.isDead())
continue; continue;
assert(!MO.isDef() && "We should have skipped all the definitions by now"); assert(!MO.isDef() && "We should have skipped all the definitions by now");
if (SrcIdx != EndOpIdx) if (SrcIdx != EndOpIdx)
// Multiple sources? // Multiple sources?
return ValueTrackerResult(); return ValueTrackerResult();
SrcIdx = OpIdx; SrcIdx = OpIdx;
} }
// In some rare case, Def has no input, SrcIdx is out of bound,
// getOperand(SrcIdx) will fail below.
if (SrcIdx >= Def->getNumOperands())
return ValueTrackerResult();
// Stop when any user of the bitcast is a SUBREG_TO_REG, replacing with a COPY // Stop when any user of the bitcast is a SUBREG_TO_REG, replacing with a COPY
// will break the assumed guarantees for the upper bits. // will break the assumed guarantees for the upper bits.
for (const MachineInstr &UseMI : MRI.use_nodbg_instructions(DefOp.getReg())) { for (const MachineInstr &UseMI : MRI.use_nodbg_instructions(DefOp.getReg())) {
if (UseMI.isSubregToReg()) if (UseMI.isSubregToReg())
return ValueTrackerResult(); return ValueTrackerResult();
} }
const MachineOperand &Src = Def->getOperand(SrcIdx); const MachineOperand &Src = Def->getOperand(SrcIdx);
▲ Show 20 Lines • Show All 242 Lines • Show Last 20 Lines

llvm/trunk/lib/Target/PowerPC/PPCInstrVSX.td

Show First 20 Lines • Show All 1,308 Lines • ▼ Show 20 Lineslet AddedComplexity = 400 in { // Prefer VSX patterns over non-VSX patterns.
def : Pat<(int_ppc_vsx_xxleqv v4i32:$A, v4i32:$B), def : Pat<(int_ppc_vsx_xxleqv v4i32:$A, v4i32:$B),
(XXLEQV $A, $B)>; (XXLEQV $A, $B)>;
let isCodeGenOnly = 1, isMoveImm = 1, isAsCheapAsAMove = 1, let isCodeGenOnly = 1, isMoveImm = 1, isAsCheapAsAMove = 1,
isReMaterializable = 1 in { isReMaterializable = 1 in {
def XXLEQVOnes : XX3Form_SameOp<60, 186, (outs vsrc:$XT), (ins), def XXLEQVOnes : XX3Form_SameOp<60, 186, (outs vsrc:$XT), (ins),
"xxleqv $XT, $XT, $XT", IIC_VecGeneral, "xxleqv $XT, $XT, $XT", IIC_VecGeneral,
[(set v4i32:$XT, (v4i32 immAllOnesV))]>; [(set v4i32:$XT, (bitconvert (v16i8 immAllOnesV)))]>;
} }
def XXLORC : XX3Form<60, 170, def XXLORC : XX3Form<60, 170,
(outs vsrc:$XT), (ins vsrc:$XA, vsrc:$XB), (outs vsrc:$XT), (ins vsrc:$XA, vsrc:$XB),
"xxlorc $XT, $XA, $XB", IIC_VecGeneral, "xxlorc $XT, $XA, $XB", IIC_VecGeneral,
[(set v4i32:$XT, (or v4i32:$XA, (vnot_ppc v4i32:$XB)))]>; [(set v4i32:$XT, (or v4i32:$XA, (vnot_ppc v4i32:$XB)))]>;
// VSX scalar loads introduced in ISA 2.07 // VSX scalar loads introduced in ISA 2.07
▲ Show 20 Lines • Show All 2,772 Lines • ▼ Show 20 Lines def : Pat<(v4i32 (build_vector i32:$A, i32:$B, i32:$C, i32:$D)),
(MTVSRD (RLDIMI AnyExts.C, AnyExts.D, 32, 0)), VSRC), (MTVSRD (RLDIMI AnyExts.C, AnyExts.D, 32, 0)), VSRC),
(COPY_TO_REGCLASS (COPY_TO_REGCLASS
(MTVSRD (RLDIMI AnyExts.A, AnyExts.B, 32, 0)), VSRC), 0)>; (MTVSRD (RLDIMI AnyExts.A, AnyExts.B, 32, 0)), VSRC), 0)>;
def : Pat<(v4i32 (build_vector i32:$A, i32:$A, i32:$A, i32:$A)), def : Pat<(v4i32 (build_vector i32:$A, i32:$A, i32:$A, i32:$A)),
(XXSPLTW (COPY_TO_REGCLASS (MTVSRWZ $A), VSRC), 1)>; (XXSPLTW (COPY_TO_REGCLASS (MTVSRWZ $A), VSRC), 1)>;
} }
let Predicates = [HasP8Vector] in { let Predicates = [HasP8Vector] in {
def : Pat<(v4i32 (bitconvert (v16i8 immAllOnesV))),
(XXLEQVOnes)>;
def : Pat<(v1i128 (bitconvert (v16i8 immAllOnesV))), def : Pat<(v1i128 (bitconvert (v16i8 immAllOnesV))),
(v1i128 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>; (v1i128 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>;
def : Pat<(v2i64 (bitconvert (v16i8 immAllOnesV))), def : Pat<(v2i64 (bitconvert (v16i8 immAllOnesV))),
(v2i64 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>; (v2i64 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>;
def : Pat<(v8i16 (bitconvert (v16i8 immAllOnesV))), def : Pat<(v8i16 (bitconvert (v16i8 immAllOnesV))),
(v8i16 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>; (v8i16 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>;
def : Pat<(v16i8 (bitconvert (v16i8 immAllOnesV))), def : Pat<(v16i8 (bitconvert (v16i8 immAllOnesV))),
(v16i8 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>; (v16i8 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>;
▲ Show 20 Lines • Show All 125 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/PowerPC/bitcast-peephole.mir

# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=powerpc64le-linux-gnu -run-pass=peephole-opt -verify-machineinstrs -o - %s | FileCheck %s
---
name: bitCast
tracksRegLiveness: true
body: |
bb.0.entry:
; CHECK-LABEL: name: bitCast
; CHECK: [[XXLEQVOnes:%[0-9]+]]:vsrc = XXLEQVOnes
; CHECK: $v2 = COPY [[XXLEQVOnes]]
; CHECK: BLR8 implicit $lr8, implicit $rm, implicit $v2
%0:vsrc = XXLEQVOnes
$v2 = COPY %0
BLR8 implicit $lr8, implicit $rm, implicit $v2
...
# This used to hit an assertion:
# llvm/include/llvm/CodeGen/MachineInstr.h:417: const
# llvm::MachineOperand &llvm::MachineInstr::getOperand(unsigned int)
# const: Assertion `i < getNumOperands() && "getOperand() out of range!"' failed.
#