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

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

Details

Reviewers
 paulwalker-arm peterwaller-arm DavidTruby sdesmalen kmclaughlin efriedma
Commits
rGc19c0ad6813d: [AArch64][SVE] Fix bug in lowering of fixed-length integer vector divides
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. Apr 13 2021, 1:56 AM

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

Gentle ping. 🙂

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.

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

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