This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][SDAG] attempt to custom legalize uaddo/usubo for long operands
Needs RevisionPublic

Authored by vikramRH on Feb 1 2023, 8:15 PM.

Details

Reviewers
arsenm
foad
alex-t
Summary

The goal here was to eliminate the redundant compare instruction generated in addition to add/sub2. Tries to address the issue discussed here, https://github.com/RadeonOpenCompute/ROCm/issues/477

Im having to modify the instruction selection for uniform addcarry/subcarry node when one of the carry users have already been selected to VALU to avoid suboptimal pattern. I have some concerns here and would like to know if it could be done in a better way.

Diff Detail

Event Timeline

vikramRH created this revision.Feb 1 2023, 8:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 8:15 PM
vikramRH requested review of this revision.Feb 1 2023, 8:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 8:15 PM
arsenm added a comment.Feb 2 2023, 3:09 AM

Are there any new tests that demonstrate what you are trying to do?

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
893–894

Spaces after // and capitalize

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
5436

I don't see the point of this, you're just doing what the default legalization should do in 1 step instead of 2

llvm/test/CodeGen/AMDGPU/carryout-selection.ll
197

In a precommit can you convert this to generated checks? It's hard to see what's going on here

llvm/test/CodeGen/AMDGPU/udiv64.ll
352–354

This is a regression

llvm/test/CodeGen/AMDGPU/usubsat.ll
684–687

This is an improvement

alex-t added inline comments.Feb 20 2023, 9:16 AM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
899

if (!RC) is "true" which means getOperandRegClass has returned NULL we've got to evaluate the second part of the condition and will call the SIRegisterInfo::isSGPRClass with nullptr? Shouldn't there be OR instead?

alex-t added inline comments.Feb 20 2023, 9:36 AM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
906

What if N is not divergent but has just one user selected to VALU and has yet 10 SALU users? Would this still be optimal?

945

Does this really belong to this change?

arsenm requested changes to this revision.Aug 15 2023, 10:42 AM
This revision now requires changes to proceed.Aug 15 2023, 10:42 AM