Page MenuHomePhabricator

[AMDGPU] Fix clamp bit DAG operand
ClosedPublic

Authored by hliao on Mar 20 2019, 12:39 PM.

Details

Summary
  • Should use targetconstant instead of constant operand for clamp bit, which is expected as an immediate operand. Under certain conditions, such as a common i1 false constant is used in other place and selected before the instruction with clamp bit, register operand may be added instead of immediate one. Use targetcosntant to enforce that.

Diff Detail

Repository
rL LLVM

Event Timeline

hliao created this revision.Mar 20 2019, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2019, 12:39 PM
hliao added a comment.Mar 20 2019, 1:08 PM

Ah, yes, but this patch covers more cases. Shall we merge them together.

tpr accepted this revision.Mar 20 2019, 1:14 PM

Thanks Michael; that is much better than my fix in D59556. Let's go with this one and I'll abandon my one.

This revision is now accepted and ready to land.Mar 20 2019, 1:14 PM
arsenm requested changes to this revision.Mar 20 2019, 1:16 PM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1124–1125 ↗(On Diff #191561)

This is missing a test case

llvm/test/CodeGen/AMDGPU/uaddo.ll
176 ↗(On Diff #191561)

This should really use VGPR inputs

178 ↗(On Diff #191561)

Should also cover sub version

This revision now requires changes to proceed.Mar 20 2019, 1:16 PM
hliao added a comment.Mar 20 2019, 1:17 PM

Thanks. Done!

hliao marked 2 inline comments as done.Mar 20 2019, 1:19 PM

sorry, saw this too late, will address in another patch

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1124–1125 ↗(On Diff #191561)

Sorry, saw this too late, I will address this in a later patch. OK?

llvm/test/CodeGen/AMDGPU/uaddo.ll
178 ↗(On Diff #191561)

sure, will address that in a later patch

This revision was not accepted when it landed; it landed in state Needs Revision.Mar 20 2019, 1:20 PM
Closed by commit rL356608: [AMDGPU] Fix clamp bit DAG operand (authored by hliao, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
arsenm added a comment.Apr 1 2019, 8:21 AM

sorry, saw this too late, will address in another patch

ping

hliao added a comment.Apr 1 2019, 9:11 AM

sorry, saw this too late, will address in another patch

ping

sorry, forget posting them, here's the link, https://reviews.llvm.org/D60071