Page MenuHomePhabricator

[mips] Fix Mips MSA instrinsics
ClosedPublic

Authored by sdardis on Oct 10 2016, 9:47 AM.

Details

Summary

The usage of some MIPS MSA instrinsics that took immediates could crash LLVM
during lowering. This patch addresses that behaviour. Crucially this patch
also makes the use of intrinsics with out of range immediates as producing an
internal error.

The ld,st instrinsics would trigger an assertion failure for MIPS64 as their
lowering would attempt to add an i32 offset to a i64 pointer.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 74140.Oct 10 2016, 9:47 AM
sdardis retitled this revision from to [mips] Fix Mips MSA instrinsics.
sdardis updated this object.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
vkalintiris edited edge metadata.Oct 19 2016, 4:40 AM

Why shouldn't we "crash" when we pass invalid/bad constant immediates to our intrinsics? Do you have a case in mind that causes us problems if we do so?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
1108 ↗(On Diff #74140)

I don't think that we need this because the type will be always promoted based on the check above. Do you have any example/test case that hits this?

1134 ↗(On Diff #74140)

Similarly, given that the element type has been expanded, I don't think that we need to cover zero-extensions.

The problem with just crashing, is that reason for the crash is completely unclear. The two changed lines in SelectionDAG.cpp from zext to zextOrTrunc ensure that handling a constant that doesn't need promotion doesn't force an assertion error in APInt's zext. I went with this solution as it seemed to be the least invasive.

The problem with just crashing, is that reason for the crash is completely unclear.

We can definitely "crash" and let our users see the reason with a meaningful message displaying the correct value range for the specific intrinsic. These intrinsics are MIPS-specific and do not get generated by LLVM but by its frontends (either the source language or any other code-generation library that our users are developing for MIPS-specific optimizations). For example, C and C++ are covered by your correspondent Clang patch that catches these errors at the input source.

The two changed lines in SelectionDAG.cpp from zext to zextOrTrunc ensure that handling a constant that doesn't need promotion doesn't force an assertion error in APInt's zext. I went with this solution as it seemed to be the least invasive.

Such cases would indicate a problem/bug in the surrounding code, which as far as I can tell, looks good to me. That's why I'm wondering if you had any specific example that triggered the assertions in zext() or trunc().

From immediates-bad.ll:

define void @addvi_d(<2 x i64> * %ptr) {
entry:
; CHECK-LABEL: addvi_d:
; CHECK: addv.d
  %a = load <2 x i64>, <2 x i64> * %ptr, align 16
  %r = call <2 x i64> @llvm.mips.addvi.d(<2 x i64> %a, i32 65)
  store <2 x i64> %r, <2 x i64> * %ptr, align 16
  ret void
}

will crash LLVM. Dump from llc:

Assertion failed: (width > BitWidth && "Invalid APInt ZeroExtend request"), function zext, file /Users/simon/dev/llvm/llvm2/llvm/lib/Support/APInt.cpp, line 981.
Stack dump:
0.	Program arguments: /Users/simon/dev/llvm/llvm2/llvmgitsvnbuild/./bin/llc -march=mips -mattr=+msa,+fp64 -relocation-model=pic 
1.	Running pass 'Function Pass Manager' on module '<stdin>'.
2.	Running pass 'MIPS DAG->DAG Pattern Instruction Selection' on function '@addvi_d'

As you can seeI think there may be another solution though, which is to constrain the type of the immediate operand down to things like i5. I would have to reinvestigate.

From immediates-bad.ll:

define void @addvi_d(<2 x i64> * %ptr) {
entry:
; CHECK-LABEL: addvi_d:
; CHECK: addv.d
  %a = load <2 x i64>, <2 x i64> * %ptr, align 16
  %r = call <2 x i64> @llvm.mips.addvi.d(<2 x i64> %a, i32 65)
  store <2 x i64> %r, <2 x i64> * %ptr, align 16
  ret void
}

will crash LLVM. Dump from llc:

Assertion failed: (width > BitWidth && "Invalid APInt ZeroExtend request"), function zext, file /Users/simon/dev/llvm/llvm2/llvm/lib/Support/APInt.cpp, line 981.
Stack dump:
0.	Program arguments: /Users/simon/dev/llvm/llvm2/llvmgitsvnbuild/./bin/llc -march=mips -mattr=+msa,+fp64 -relocation-model=pic 
1.	Running pass 'Function Pass Manager' on module '<stdin>'.
2.	Running pass 'MIPS DAG->DAG Pattern Instruction Selection' on function '@addvi_d'

As you can seeI think there may be another solution though, which is to constrain the type of the immediate operand down to things like i5. I would have to reinvestigate.

That's weird, it compiles fine for me with r284832:

vk@nx9420 $ ninja -C /home/vk/build/llvm/debug/ && llc -march=mips -mattr=+msa,+fp64 -relocation-model=pic <./tmp.ll
ninja: Entering directory `/home/vk/build/llvm/debug/'
ninja: no work to do.
      .text
      .abicalls
      .section        .mdebug.abi32,"",@progbits
      .nan    legacy
      .module fp=64
      .file   "<stdin>"
      .text
      .globl  addvi_d
      .p2align        2
      .type   addvi_d,@function
      .set    nomicromips
      .set    nomips16
      .ent    addvi_d
addvi_d:                                # @addvi_d
      .cfi_startproc
      .frame  $sp,0,$ra
      .mask   0x00000000,0
      .fmask  0x00000000,0
      .set    noreorder
      .set    nomacro
      .set    noat
# BB#0:                                 # %entry
      ldi.d   $w0, 65
      shf.w   $w0, $w0, 177
      ld.d    $w1, 0($4)
      addv.d  $w0, $w1, $w0
      jr      $ra
      st.d    $w0, 0($4)
      .set    at
      .set    macro
      .set    reorder
      .end    addvi_d
$func_end0:
      .size   addvi_d, ($func_end0)-addvi_d
      .cfi_endproc
      .section        ".note.GNU-stack","",@progbits
      .text
@v16i8_a = global <16 x i8> <i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7, i8 8, i8 9, i8 10, i8 11, i8 12, i8 13, i8 14, i8 15>, align 16
@v16i8_r = global <16 x i8> <i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7, i8 8, i8 9, i8 10, i8 11, i8 12, i8 13, i8 14, i8 15>, align 16

; Function Attrs: nounwind
define void @test() #0 {
entry:
  %0 = load <16 x i8>, <16 x i8>* @v16i8_r, align 16
  %1 = load <16 x i8>, <16 x i8>* @v16i8_a, align 16
  %2 = call <16 x i8> @llvm.mips.binsri.b(<16 x i8> %0, <16 x i8> %1, i32 -1)
  store <16 x i8> %2, <16 x i8>* @v16i8_r, align 16
  ret void
}

Although that IR should not be generated, I want to be sure that we get something out rather than a bizarre crash.

sdardis updated this revision to Diff 77812.Nov 14 2016, 7:32 AM
sdardis updated this object.
sdardis removed a reviewer: zoran.jovanovic.
sdardis removed rL LLVM as the repository for this revision.

Address issue with ld, st intrinsics.

vkalintiris added inline comments.Nov 15 2016, 11:28 AM
lib/Target/Mips/MipsSEISelLowering.cpp
2289 ↗(On Diff #77812)

Is there any scenario where we might need to truncate the offset here? It seems to me that we only care for the extension of the offset type with an IDS::SIGN_EXTEND node. Similarly for the lowerMSAStoreIntr below.

sdardis updated this revision to Diff 78356.Nov 17 2016, 5:59 AM
sdardis marked an inline comment as done.

Address review comments.

lib/Target/Mips/MipsSEISelLowering.cpp
2289 ↗(On Diff #77812)

Yes, you're right, we only need an ISD::SIGN_EXTEND node here.

sdardis updated this revision to Diff 83357.Jan 6 2017, 4:50 AM
sdardis added a reviewer: slthakur.

Rebase and flag usable values as an error rather than UNDEF.

Correction, our of range values as an error.

vkalintiris accepted this revision.Jan 9 2017, 9:26 AM
vkalintiris edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jan 9 2017, 9:26 AM
sdardis updated this object.Jan 10 2017, 8:50 AM
sdardis edited edge metadata.
This revision was automatically updated to reflect the committed changes.