Page MenuHomePhabricator

libclc: Fix rounding during type conversion
ClosedPublic

Authored by daniels on Jun 17 2020, 2:58 AM.

Details

Summary

The rounding during type conversion uses multiple conversions, selecting
between them to try to discover if rounding occurred. This appears to
not have been tested, since it would generate code of the form:

float convert_float_rtp(char x)
{
  float r = convert_float(x);
  char y = convert_char(y);
  [...]
}

which will access uninitialised data. The idea appears to have been to
have done a char -> float -> char roundtrip in order to discover the
rounding, so do this.

Discovered by inspection.

Signed-off-by: Daniel Stone <daniels@collabora.com>

Diff Detail

Event Timeline

daniels created this revision.Jun 17 2020, 2:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 2:58 AM

I don't have a good way to test this, but tbh I don't think anyone else does either, given it came from POCL so has been broken for as long as libclc's had any support for it. Looking into POCL just now, it looks like they applied the same fix a few years ago. There are other fixes as well which might be good to take up, but I'm not going to propose any invasive changes, since SPIR-V doesn't (can't) use these helpers at all: the LLVM-SPIRV-Translator specifically looks for calls to the convert_* functions and replaces them with equivalently-decorated SPIR-V OpConvert*.

I think these routines need to be rewritten and tested with CTS on platforms that use them.

I totally agree, but I don't have those platforms so I won't be progressing this myself.

This definitely looks like the correct fix. I had originally included it in D94013, but it makes more sense as separate commit.

while the fix is clearly correct, it'd be nice to do a test run and report changes.
I can run it on my carrizo machine, but the conversion tests take more than a day.

jvesely accepted this revision.Mar 2 2021, 9:01 PM

it took a while to test both versions. this patch fixes the following conversion tests:

	 *** 622) convert_floatn_rtp( uintn ) FAILED ** 
	 *** 627) convert_floatn_rtp( intn ) FAILED ** 
	 *** 639) convert_floatn_rtz( doublen ) FAILED ** 
	 *** 649) convert_floatn_rtz( longn ) FAILED ** 
	 *** 689) convert_doublen_rtz( doublen ) FAILED **

The number of failed cases goes down from 42 to 37

This revision is now accepted and ready to land.Mar 2 2021, 9:01 PM

it took a while to test both versions. this patch fixes the following conversion tests:

	 *** 622) convert_floatn_rtp( uintn ) FAILED ** 
	 *** 627) convert_floatn_rtp( intn ) FAILED ** 
	 *** 639) convert_floatn_rtz( doublen ) FAILED ** 
	 *** 649) convert_floatn_rtz( longn ) FAILED ** 
	 *** 689) convert_doublen_rtz( doublen ) FAILED **

The number of failed cases goes down from 42 to 37

Great, thanks ... is there any reason not to land this now?

@daniels Do you have commit access?

@daniels Do you have commit access?

Nope, I need someone to land this for me please.

This revision was automatically updated to reflect the committed changes.