Page MenuHomePhabricator

[ARM] Teach ComputeKnownBitsTarget to handle extract vectors
ClosedPublic

Authored by dnsampaio on Dec 27 2018, 4:19 AM.

Details

Summary

Teaching the ARM backend to set upper zero bits when performing a sign or zero extend allows to remove redundant masking.

Diff Detail

Repository
rL LLVM

Event Timeline

dnsampaio created this revision.Dec 27 2018, 4:19 AM
sbaranga added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
13639 ↗(On Diff #179545)

IIUC we're computing here the known bits on the input vector operand. However, when computing the result of the VGETLANE operation we're not taking into account the lane operand, which seems wrong to me.

We should have additional tests to cover both the sext and zext cases and cases where the lane operand is not zero.

13656 ↗(On Diff #179545)

Is this assert unreachable?

dnsampaio marked 3 inline comments as done.Dec 28 2018, 4:21 AM
dnsampaio added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
13639 ↗(On Diff #179545)

Perhaps I've got it wrong, but from what I've seen is that computeKnownBits tells bits that are known to be zero or one in all vector elements (lanes). So it really does not matter which lane is being extracted. Even the width is the number of bits of a single element, not the entire vector.

I'll add a signed extension test, to show that the behavior is the same.

13656 ↗(On Diff #179545)

It is. Compiling fore release does not test it. Thanks.

dnsampaio updated this revision to Diff 179623.Dec 28 2018, 4:33 AM
dnsampaio marked an inline comment as done.
dnsampaio retitled this revision from [ARM] Teach ComputeKnownBits to handle extract vectors to [ARM] Teach ComputeKnownBitsTarget to handle extract vectors.
dnsampaio edited the summary of this revision. (Show Details)
RKSimon added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
13639 ↗(On Diff #179545)

computeKnownBits can takes DemandedElts mask to indicate the vector elements that you care about - see the ISD::EXTRACT_VECTOR_ELT code in SelectionDAG::computeKnownBits for an example of how it works.

dnsampaio updated this revision to Diff 179634.Dec 28 2018, 6:40 AM
dnsampaio marked 3 inline comments as done.

Track a single element if the index is a constant.

dnsampaio added inline comments.Dec 28 2018, 7:01 AM
lib/Target/ARM/ARMISelLowering.cpp
13639 ↗(On Diff #179545)

Hi, thanks for the suggestion. I just couldn't find a straightforward test to capture such behavior, as most simplifications are done before extractelement gets replaced by VGETLANE. It doesn't seems to have an intrinsic as well.

RKSimon added inline comments.Dec 28 2018, 7:23 AM
lib/Target/ARM/ARMISelLowering.cpp
13639 ↗(On Diff #179545)

Something that make use of the implicit sext/zext should be testable? IIRC we do something similar on X86 for PEXTRB/PEXTRW

sbaranga added inline comments.Dec 28 2018, 8:42 AM
lib/Target/ARM/ARMISelLowering.cpp
13643 ↗(On Diff #179634)

I think this test should always evaluate to true. Maybe it should be an assert?

dnsampaio marked an inline comment as done.Dec 29 2018, 9:36 AM
dnsampaio added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
13643 ↗(On Diff #179634)

Don't know. Can't Pos be nullptr? That is, the position is not a constant value? (vector[variableX] ?)
if (Pos >= NumSrcElts), perhaps I could just consider it a poison and return as a value zero.

dnsampaio updated this revision to Diff 180253.Jan 4 2019, 9:08 AM
dnsampaio marked 2 inline comments as done.

Removed treatment for out of bounds constant, as it is simplified to poison before being transformed to VGETLANE.
Also removed treatment for variable index, as it is not accepted by VGETLANE.
Added extra tests.

lib/Target/ARM/ARMISelLowering.cpp
13643 ↗(On Diff #179634)

Oh, I see that when creating these operands we only allow constant values. And trying llvm-mc it confirms it so. Sorry, did not get the suggestion. :)

sbaranga accepted this revision.Jan 6 2019, 3:39 PM

LGTM! (with one nit)

lib/Target/ARM/ARMISelLowering.cpp
13643 ↗(On Diff #179634)

Yeah, sorry, I should have given a justification for that.

These nodes are also used to only select instructions that have the lane encoded as an immediate.

13642 ↗(On Diff #180253)

Nit: you can use a cast<> instead of dyn_cast here and remove the assert.

This revision is now accepted and ready to land.Jan 6 2019, 3:39 PM
This revision was automatically updated to reflect the committed changes.
dnsampaio marked an inline comment as done.