This is an archive of the discontinued LLVM Phabricator instance.

[mips][msa] Mask vectors holding shift amounts
ClosedPublic

Authored by smaksimovic on Mar 24 2017, 7:36 AM.

Details

Summary

Masked vectors which hold shift amounts when creating following nodes: ISD::SHL, ISD::SRL or ISD::SRA.
Instructions that use said nodes, which have had their arguments altered are sll, srl, sra, bneg, bclr and bset.

For said instructions, the shift amount or the bit position that is specified in the corresponding vector elements
will be interpreted as the shift amount/bit position modulo the size of the element in bits.

The problem lies in compiling with -O2 enabled, where the instructions for formats .w and .d are not generated,
but are instead optimized away.
In this case, having shift amounts that are either negative or greater than the element bit size results in
generation of incorrect results when constant folding.

We remedy this by masking the operands for the nodes mentioned above before actually creating them, so that the
final result is correct before placed into the constant pool.

Diff Detail

Repository
rL LLVM

Event Timeline

smaksimovic created this revision.Mar 24 2017, 7:36 AM
efriedma requested changes to this revision.Mar 24 2017, 11:48 AM
efriedma added a subscriber: efriedma.

This is obviously not the correct fix: you need to mask the shift amount whether or not you can prove the shift amount is constant in lowerINTRINSIC_WO_CHAIN.

This revision now requires changes to proceed.Mar 24 2017, 11:48 AM
smaksimovic edited edge metadata.

Hello, thanks for the comment.

Made a change introducing an ISD::AND node which is now used to truncate the vector elements which hold shift amounts regardless of the vector having constants or not.

One downside we're experiencing now is that although we produce the code that executes correctly, and.v instruction is generated in addition to sll, where and.v is superfluous since sll will handle both negative and those shift amounts that are larger by value than the vector element size correctly.

Ideally, we'd like the sll and and.v to be optimized away in case all of the operands are constant values.
Additionally, this could be restricted to -O2, since on lower optimization levels shift instructions are generated anyway, hence and.v will be redundant in that situation as well.

Any tips on handling these two issues?

You basically need to pattern-match the ISD::AND+ISD::SHL to the appropriate native instruction. Given that the AND mask is going to get lowered to a constant-pool load (I think?), it's tricky to match with TableGen patterns; probably easiest to add a target-specific MIPSISD::VSHL etc. which masks the shift amount. The x86 backend does something similar.

sdardis requested changes to this revision.Apr 3 2017, 4:43 AM

This latest diff is too general. For the constant case we can perform the bit masking at compile time to reformulate the constant into something that LLVM's IR will treat as legal and that matches what the hardware will do.

For the general case we can elide the '(and operand, (build_vector <ElementSizeInBits -1>...))' with the following:

def immi32Cst31 : ImmLeaf<i32, [{return isUInt<32>(Imm) && Imm == 31;}]>;

def : MSAPat<
  (v4i32 (shl v4i32:$ws, (v4i32 (and v4i32:$wt,
                                     (build_vector immi32Cst31, immi32Cst31,
                                                   immi32Cst31, immi32Cst31))))),
  (v4i32 (SLL_W v4i32:$ws, v4i32:$wt))>;

(That example may need some reformatting.)

That example which I've briefly tested removes the and SDNode during ISel. You can modify that pattern into a multiclass and then instantiate it for the relevant instructions.

In principle we should cover the .b and .h cases as well.

lib/Target/Mips/MipsSEISelLowering.cpp
1556–1560 ↗(On Diff #93624)

This hunk only covers the non-constant case. When the operand to a logical ISD vector node--which implicitly masks the lower bits--is a ConstantSDnode, we should instead reformulate the constant so that it only contains bits that the MSA instructions will look at.

This revision now requires changes to proceed.Apr 3 2017, 4:43 AM
efriedma added inline comments.Apr 3 2017, 1:28 PM
lib/Target/Mips/MipsSEISelLowering.cpp
1556–1560 ↗(On Diff #93624)

Doesn't constant folding cover the constant case?

@efriedma was right about constant folding taking care of the constant case, so you can ignore my comments about that.

Some more comments inlined, mostly on the tests.

lib/Target/Mips/MipsSEISelLowering.cpp
1556–1560 ↗(On Diff #93624)

You right, it does. I assumed it wouldn't.

test/CodeGen/Mips/msa/3r-s.ll
134 ↗(On Diff #93624)

For the test cases you've changed, can you update the CHECK: <function name>: to CHECK-LABEL: ?

143 ↗(On Diff #93624)

These extra ; can be removed in the changed cases as well.

643 ↗(On Diff #93624)

This can be removed.

test/CodeGen/Mips/msa/shift_constant_pool.ll
4–5 ↗(On Diff #93624)

Rather than using CHECK-ALL, you can just use ALL for brevity. The multiple -check-prefix s can be combined into --check-prefixes=ALL,MIPS64 / --check-prefixes=ALL,MIPS32

24 ↗(On Diff #93624)

This should ALL-LABEL: llvm_mips_sll_w_test_const_vec:

33–34 ↗(On Diff #93624)

These two lines are unnecessary.

52 ↗(On Diff #93624)

See my comments on llvm_mips_sll_w_test_const_vec about ALL-LABEL and the last two lines which can be dropped.

80 ↗(On Diff #93624)

See my comments on llvm_mips_sll_w_test_const_vec about ALL-LABEL and the last two lines which can be dropped.

smaksimovic edited edge metadata.
  • Added classes covering patterns which remove and nodes for sll, srl and sra instructions in MipsMSAInstrInfo.td
  • Modified the existing patterns for bclr, bneg, bset instructions to match the additional and node which we now create
  • Dropped changes in test/CodeGen/Mips/msa/3r-s.ll, since we do not generate and instructions now, so no changes required
  • Added a test which specifically checks for said and instructions not being present in the resulting code
sdardis requested changes to this revision.Apr 13 2017, 8:56 AM

Testing this shows that test/CodeGen/Mips/msa/bitwise.ll is now failing as the bitwise instructions are no longer selected.

lib/Target/Mips/MipsMSAInstrInfo.td
355 ↗(On Diff #94800)

This predicate is unused.

371–410 ↗(On Diff #94800)

These changes are regressing the general bitwise ISel logic for these instructions, as now their patterns are matching what the intrinsics produce when those intrinsics are lowered.

You'll probably need to duplicate the patterns instead of changing them.

This revision now requires changes to proceed.Apr 13 2017, 8:56 AM
smaksimovic edited edge metadata.

Addressed review comments.

sdardis accepted this revision.Apr 19 2017, 3:11 AM

LGTM, you'll need @efriedma to accept this as well before this revision can be closed.

lib/Target/Mips/MipsMSAInstrInfo.td
3843–3861 ↗(On Diff #95552)

Nit: These can be redone as a multiclass pattern parametrized over the SDNode and instruction, similar to what you did with the MSAShiftPats.

Redefined bset and bneg patterns via multiclass as suggested.

This revision is now accepted and ready to land.Apr 19 2017, 11:44 AM
smaksimovic retitled this revision from [mips][msa] Truncation of vector elements for instructions creating ISD::SHL, ISD::SRL or ISD::SRA nodes to [mips][msa] Mask vectors holding shift amounts.Apr 20 2017, 5:50 AM
smaksimovic edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.