This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Legalize G_[SU]DIVREM instructions
ClosedPublic

Authored by cdevadas on Apr 18 2021, 7:19 AM.

Diff Detail

Event Timeline

cdevadas created this revision.Apr 18 2021, 7:19 AM
cdevadas requested review of this revision.Apr 18 2021, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2021, 7:19 AM

LGTM except you don't need NoRegister all over the place

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
2827

Don't need the != NoRegister

2833–2837

Ditto

2836

Ditto

2983

Ditto

3068

This is default initialized

cdevadas updated this revision to Diff 346981.May 21 2021, 4:00 AM

Rebase + comments addressed.

arsenm accepted this revision.May 24 2021, 11:59 AM
This revision is now accepted and ready to land.May 24 2021, 11:59 AM
This revision was landed with ongoing or failed builds.May 24 2021, 10:22 PM
This revision was automatically updated to reflect the committed changes.
foad added inline comments.May 25 2021, 2:56 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
586–589

Matt usually objects to this indentation change on the grounds that clang-format is wrong :)

2985

Pull this buildICmp out of the "if"s, since it is identical in both cases, and you don't want to build it twice in the divrem case?

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h
113

It looks odd that you did not change the name of this function, but I see it is not implemented, so maybe just remove the declaration here?

cdevadas added inline comments.May 25 2021, 5:15 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
586–589

Oh. It was auto-formatted by clang-format.

2985

Thanks Jay. It is addressed with D103083.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h
113

Yes, it was a dead declaration. Cleaned it up with e3b8e6d48251

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp