This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Fix round lowering
ClosedPublic

Authored by arsenm on Mar 15 2020, 11:15 AM.

Details

Summary

I used the implementation for floor instead of round. It also turns
out the OpenCL builtin library wasn't using the round builtin, but
implemented the expanded form.

Diff Detail

Event Timeline

arsenm created this revision.Mar 15 2020, 11:15 AM
foad added a comment.Mar 16 2020, 6:33 AM

Looks OK technically. Why is it useful to lower round and floor in terms of trunc? Is trunc somehow more primitive, or more commonly legal? Does the AMDGPU backend use any of this?

Looks OK technically. Why is it useful to lower round and floor in terms of trunc? Is trunc somehow more primitive, or more commonly legal? Does the AMDGPU backend use any of this?

We need the round lowering. I think the floor lowering is now dead in the DAG path (but R600 might need it for f64 if that were ever implemented)

I looked through the implementation of lowerIntrinsicRound, it looks good for me.

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-intrinsic-round.mir
1

If the previous version of this file tested intrinsic floor, maybe it makes sense to keep that version (with necessary changes) under a new name, to keep tests for floor?

foad accepted this revision.Mar 16 2020, 7:19 AM
foad added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4691

Not related to your patch, but couldn't this be if (result > src) ?

This revision is now accepted and ready to land.Mar 16 2020, 7:19 AM
arsenm closed this revision.Mar 16 2020, 8:37 AM
arsenm marked 2 inline comments as done.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4691

I don't see why not. I"m guessing this has been blindly copied around since AMDIL times, which didn't have all the compare types

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-intrinsic-round.mir
1

There's already a floor test, we didn't actually need the floor expansion