Page MenuHomePhabricator

[AArch64][SVE] Fix bug in lowering of fixed-length integer vector divides
ClosedPublic

Authored by joechrisellis on Apr 13 2021, 1:56 AM.

Details

Summary

The function AArch64TargetLowering::LowerFixedLengthVectorIntDivideToSVE
previously assumed the operands were full vectors, but this is not
always true. This function would produce bogus if the division operands
are not full vectors, resulting in miscompiles when dividing 8-bit or
16-bit vectors.

The fix is to perform an extend + div + truncate for non-full vectors,
instead of the usual unpacking and unzipping logic. This is an additive
change which reduces the non-full integer vector divisions to a pattern
recognised by the existing lowering logic.

For future reference, an example of code that would miscompile before
this patch is below:

1  int8_t foo(unsigned N, int8_t *a, int8_t *b, int8_t *c) {
2      int8_t result = 0;
3      for (int i = 0; i < N; ++i) {
4          result += (a[i] / b[i]) / c[i];
5      }
6      return result;
7  }

Diff Detail

Event Timeline

joechrisellis created this revision.Apr 13 2021, 1:56 AM
joechrisellis requested review of this revision.Apr 13 2021, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 1:56 AM

Fix failing llvm/test/CodeGen/AArch64/sve-fixed-length-int-rem.ll test.

Gentle ping. 🙂

paulwalker-arm added inline comments.Mon, Apr 19, 2:55 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-div.ll
0–1

OCD perhaps but can you move this and VBITS_EQ_1024 to the end of the list so the GE parts remain aligned.

0–1

Given the updates I wondered if this is still necessary?

74

To be precise this should be QUARTER VECTOR OR SMALLER. Same goes for the other instances that follow.

251

What is different here from the previous tests? I would have thought HALF VECTOR would carry the same meaning.

llvm/test/CodeGen/AArch64/sve-fixed-length-int-rem.ll
0–1

As above. Is this still used? I imagine most of my div.ll comments will apply to this file.

joechrisellis marked 3 inline comments as done.

Address review comments.

  • @paulwalker-arm:
    • more accurate comments describing the fullness of vectors.
    • more consistent use of labels.
    • move EQ labels to the end of the prefix list in invocations of FileCheck.
    • removed unused VBYTES for -aarch64-sve-vector-bits-min=128.
joechrisellis marked 2 inline comments as done.Mon, Apr 19, 7:37 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-div.ll
0–1

Sorry I should have been clearer. I meant to say that I didn't think any of the RUN lines require setting VBYTES because with your change the predicate patterns should be fixed based on the VBITS prefix in use.

Address review comments:

joechrisellis marked an inline comment as done.Thu, Apr 22, 8:10 AM
paulwalker-arm accepted this revision.Thu, Apr 22, 9:59 AM

The non-power-of-two RUN lines should be extended to allow maximum testing (see comments) but otherwise looks good.

llvm/test/CodeGen/AArch64/sve-fixed-length-int-div.ll
3

This is missing -check-prefixes=CHECK,VBITS_EQ_256. Actually this is a universal comment, in that all the RUN lines with vector-bits-min=512-896 should include VBITS_EQ_512, vector-bits-min=1024-1920 should include VBITS_EQ_1024.

llvm/test/CodeGen/AArch64/sve-fixed-length-int-rem.ll
3

Same missing VBITS_EQ_### comment as with div tests.

This revision is now accepted and ready to land.Thu, Apr 22, 9:59 AM