Page MenuHomePhabricator

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

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

Details

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

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.

jsji updated this revision to Diff 215423.Aug 15 2019, 10:04 AM

Rebased to latest ToT.

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.

@arsenm Is this MIR test OK?

@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

@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.

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
This revision was automatically updated to reflect the committed changes.