[PATCH] D65542: [PeepholeOptimizer] Don't assume bitcast def always has input

[Jinsong Ji via Phabricator via llvm-commits]

jsji marked an inline comment as done.
jsji added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/bitcast-peephole.mir:2
+# RUN: llc -mtriple=powerpc64le-linux-gnu -run-pass=peephole-opt -verify-machineinstrs -o - %s | FileCheck %s
+# REQUIRES: asserts
+
----------------
qcolombet wrote:
> jsji wrote:
> > qcolombet wrote:
> > > qcolombet wrote:
> > > > 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.
> > > 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?
> > 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
> > 
> > ```
> > 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?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65542/new/

https://reviews.llvm.org/D65542