This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix llvm.amdgcn.div.scale lowering
AbandonedPublic

Authored by kerbowa on Apr 12 2020, 11:19 PM.

Details

Reviewers
foad
arsenm
Summary

Update lowering to match the comment in IntrinsicsAMDGPU.td and
GlobalISel FDIV lowering. Also see https://reviews.llvm.org/D77547

Diff Detail

Event Timeline

kerbowa created this revision.Apr 12 2020, 11:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2020, 11:19 PM
arsenm added inline comments.Apr 13 2020, 7:29 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6010

Is there an isNull or something to use instead of negate?

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.div.scale.ll
21

I checked these were the same order as the final DAG output, so are these inconsistent now?

kerbowa updated this revision to Diff 256976.Apr 13 2020, 7:54 AM

Use isNull.

kerbowa marked an inline comment as done.Apr 13 2020, 7:55 AM
kerbowa added inline comments.
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.div.scale.ll
21

DAG lowering was wrong. I thought we never used the intrinsic directly so it didn't matter.

arsenm added inline comments.Apr 13 2020, 9:55 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.div.scale.ll
21

I think this is really "comment is wrong". In case there are any users, we should maintain the current behavior

kerbowa added inline comments.Apr 13 2020, 11:23 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.div.scale.ll
21

I thought we were sure it wasn't being used.. Seems like we would have heard about it otherwise.

Anyway I agree it's safer to change the comment and the GlobalISel FDIV lowering so I'll abandon this.

Can this be closed now?

kerbowa abandoned this revision.Apr 20 2020, 8:11 AM