This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] truncate left shift
ClosedPublic

Authored by rampitec on Jun 27 2017, 4:15 PM.

Details

Summary

That is pretty common for clang to produce code like
(shl %x, (and %amt, 31)). In this situation we can still perform
trunc (shl) into shl (trunc) conversion given the known value
range of shift amount.

Diff Detail

Event Timeline

rampitec created this revision.Jun 27 2017, 4:15 PM
arsenm added inline comments.Jun 27 2017, 5:06 PM
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
2761–2762 ↗(On Diff #104306)

This exact combine is already done in DAGCombiner:

// trunc (shl x, K) -> shl (trunc x), K => K < VT.getScalarSizeInBits()

Why isn't it triggering? Is the other combine you added missing an AddToWorklist or something?

rampitec added inline comments.Jun 27 2017, 5:08 PM
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
2761–2762 ↗(On Diff #104306)

Ah. That one only works with constants.

rampitec added inline comments.
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
2761–2762 ↗(On Diff #104306)

Do you want me to transfer it there?

arsenm added inline comments.Jun 27 2017, 5:21 PM
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
2761–2762 ↗(On Diff #104306)

Yes, it would be better if it was all in one place

rampitec updated this revision to Diff 104329.Jun 27 2017, 6:04 PM
rampitec marked 4 inline comments as done.
rampitec edited the summary of this revision. (Show Details)

Moved implementation into DAGCombiner and replaced existing DAGCombiner's optimization which was only capable of dealing with constant shift amounts.

arsenm added inline comments.Jun 27 2017, 6:56 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8215

Will this regress the constant vector case?

rampitec added inline comments.Jun 27 2017, 7:10 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8215

No.

arsenm accepted this revision.Jun 27 2017, 7:14 PM

LGTM

This revision is now accepted and ready to land.Jun 27 2017, 7:14 PM
rampitec updated this revision to Diff 104340.Jun 27 2017, 7:25 PM
rampitec marked 2 inline comments as done.

Added vector by constant vector testcase.

The vector case won't matter for us, but maybe for other targets with legal vector operations

This revision was automatically updated to reflect the committed changes.