Page MenuHomePhabricator

AMDGPU: Only use legal inline immediates with kill pseudo
ClosedPublic

Authored by arsenm on Jul 7 2016, 12:01 PM.

Details

Summary

Only if the value is negative or positive is what matters,
so use a constant that doesn't require an instruction to
materialize.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 63112.Jul 7 2016, 12:01 PM
arsenm retitled this revision from to AMDGPU: Only use legal inline immediates with kill pseudo.
arsenm updated this object.
arsenm added a reviewer: nhaehnle.
arsenm added a subscriber: llvm-commits.
arsenm updated this revision to Diff 63128.Jul 7 2016, 1:36 PM
arsenm edited edge metadata.

more tests

nhaehnle edited edge metadata.Jul 8 2016, 2:29 AM

I don't understand why changing the type to i32 is necessary. Isn't -1.0 also a legal inline immediate practically everywhere?

arsenm added a comment.Jul 8 2016, 8:50 AM

I don't understand why changing the type to i32 is necessary. Isn't -1.0 also a legal inline immediate practically everywhere?

It's not, it's just more natural to express a boolean with an integer

I agree, but the D3D designers in their infinite wisdom designed a KILL opcode with a floating point parameter, which we inherited via TGSI in Mesa, which in turn motivated this (indeed somewhat questionable) intrinsic.

Hmm. My concern is that (non-constant) KILL gets lowered to a floating point comparison, which treats NaNs specially. (i32)-1 is a NaN. I admit that it doesn't really matter since this patch uses a constant, but it still leaves me with an uncomfortable feeling.

I agree, but the D3D designers in their infinite wisdom designed a KILL opcode with a floating point parameter, which we inherited via TGSI in Mesa, which in turn motivated this (indeed somewhat questionable) intrinsic.

Hmm. My concern is that (non-constant) KILL gets lowered to a floating point comparison, which treats NaNs specially. (i32)-1 is a NaN. I admit that it doesn't really matter since this patch uses a constant, but it still leaves me with an uncomfortable feeling.

The intrinsic itself's type doesn't change, this is just an internal implementation detail

arsenm updated this revision to Diff 63780.Jul 12 2016, 11:41 PM
arsenm edited edge metadata.

Use -1.0

I forgot about this patch. I investigated our kill code generation some more, and it turns out that changing the "default" kill intrinsic to take an i1 instead of a float leads to better code anyway.

So if this change here is urgent to you for some reason, just go ahead and commit as-is and I'll rebase my changes on top of it, otherwise I'll soon post a change that should end up having the same effect.

I forgot about this patch. I investigated our kill code generation some more, and it turns out that changing the "default" kill intrinsic to take an i1 instead of a float leads to better code anyway.

So if this change here is urgent to you for some reason, just go ahead and commit as-is and I'll rebase my changes on top of it, otherwise I'll soon post a change that should end up having the same effect.

Removing the kilp intrinsic would also be nice

Yes, I'm working on a combination of patches for LLVM and Mesa that replaces both llvm.AMDGPU.kill and llvm.amdgpu.kilp with an llvm.amdgcn.kill(i1) intrinsic, where the argument is true if the thread should be killed.

arsenm accepted this revision.Jul 19 2016, 9:46 AM
arsenm added a reviewer: arsenm.

r275988

This revision is now accepted and ready to land.Jul 19 2016, 9:46 AM
arsenm closed this revision.Jul 19 2016, 9:46 AM