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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet 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
+
----------------
jsji wrote:
> 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?
Let's leave the `REQUIRE: assert` then.

Thanks for double checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65542