Page MenuHomePhabricator

[AArch64] Fix performPostLD1Combine to check for constant lane index.
ClosedPublic

Authored by gberry on May 8 2018, 10:59 AM.

Details

Summary

performPostLD1Combine in AArch64ISelLowering looks for vector
insert_vector_elt of a loaded value which it can optimize into a single
LD1LANE instruction. The code checking for the pattern was not checking
if the lane index was a constant which could cause two problems:

  • an assert when lowering the LD1LANE ISD node since it assumes an constant operand
  • an assert in isel if the lane index value depends on the post-incremented base register

Both of these issues are avoided by simply checking that the lane index
is a constant.

Fixes bug 35822.

Diff Detail

Repository
rL LLVM

Event Timeline

gberry created this revision.May 8 2018, 10:59 AM
samparker added inline comments.
lib/Target/AArch64/AArch64ISelLowering.cpp
9938 ↗(On Diff #145730)

Should the value of Lane also be checked for legality?

javed.absar added inline comments.May 9 2018, 4:42 AM
lib/Target/AArch64/AArch64ISelLowering.cpp
9938 ↗(On Diff #145730)

Maybe simpler to write it as :

// Check lane is a ...
if (IsLaneOp && !isa<ConstantSDNode>(N->getOperand(2))

return ..
gberry updated this revision to Diff 145999.May 9 2018, 1:46 PM

Add lane index range check

gberry added inline comments.May 9 2018, 1:49 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
9938 ↗(On Diff #145730)

I added a range check of the lane index, even though I don't think it is possible to get an INSERT_VECTOR_ELT with an out-of-range constant index at this point in isel. Better safe than sorry though.

@javed.absar I don't think your suggestion makes sense after the addition of the range check. I also like that the later use of 'Lane' is a little clearer, but I don't feel strongly about it.

samparker accepted this revision.May 10 2018, 12:49 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 10 2018, 12:49 AM
This revision was automatically updated to reflect the committed changes.