Is this builtin function now obsolete?

(Carl Peto) #1

Hi all,

I am trying to fix a unit test in my latest PR.

In order to do so, I'm trying to reverse engineer SIL into swift code that would produce this SIL, to check how it behaves on various platforms.

sil @remove_trunc_utos_zext_of_and : $@convention(thin) (Builtin.Word) -> (Builtin.Word) {
bb0(%0 : $Builtin.Word):
  %1 = integer_literal $Builtin.Word, 4611686018427387903
  %2 = builtin "and_Word"(%0 : $Builtin.Word, %1 : $Builtin.Word) : $Builtin.Word
  %3 = builtin "zextOrBitCast_Word_Int64"(%2 : $Builtin.Word) : $Builtin.Int64
  %4 = builtin "u_to_s_checked_conversion_Int64"(%3 : $Builtin.Int64): $(Builtin.Int64, Builtin.Int1)
  %5 = tuple_extract %4 : $(Builtin.Int64, Builtin.Int1), 0
  %6 = tuple_extract %4 : $(Builtin.Int64, Builtin.Int1), 1
  cond_fail %6 : $Builtin.Int1
  %8 = builtin "truncOrBitCast_Int64_Word"(%5 : $Builtin.Int64) : $Builtin.Word
  return %8 : $Builtin.Word

I can't figure out how to reverse engineer this into swift. I tried something like...

func remove_trunc_utos_zext_of_and(i1: Int) -> Int {
	let i2: Int = 4611686018427387903
	let i3: Int = i1 & i2
	let i4: UInt64 = UInt64(i3)
	let i5: Int64 = Int64(bitPattern: i4)
	return Int(i5)

But I can't figure out how to produce the builtin function u_to_s_checked_conversion_Int64 in SIL. Grepping through the standard library and compiler source code, I can't see anything that would emit this builtin?

Is it something that used to be in the standard library but isn't any more?

Basically I am trying to figure out if the above SIL could realistically occur on 32 bit platforms, given that SIL itself isn't a language, it's an intermediate representation? If the answer is "no", then maybe it's reasonable to elide this unit test on 32 bit platforms?

(Joe Groff) #2

You can invoke any builtin from Swift code by referencing it from the Builtin module, if you invoke the swift compiler with the -parse-stdlib flag:

func foo(x: Builtin.Int64) -> Builtin.Int64 {
  return Builtin.u_to_s_checked_conversion_Int64(x)

Note that -parse-stdlib also disables implicit import of the Swift standard library module, so if your test also uses standard library facilities, you'll need to explicitly import Swift as well. You shouldn't have to implement anything to support the builtin, since the compiler already knows how to turn this into LLVM IR, and the LLVM backend should generate assembly language for the target.

(Carl Peto) #3

Thanks Joe.

I think I was a bit unclear with my reasoning. There were sort of two questions in my original post, from your answer and from a bit of thinking, I'll try to answer both of them and check with you if I'm correct?

First question...

"I cannot find any way of producing Builtin.u_to_s_checked_conversion_Int64 in SIL using 'normal' swift, given that Builtin isn't available unless you're parsing stdlib. Also this function is not called anywhere in the current version of stdlib that I can see. Given these two things, does that mean that u_to_s_checked_conversion_Int64 is now obsolete and a candidate for removing from the language one day?"

(This might be a swift evolution topic rather than for discussion on here.)

My answer is...

"I might have missed somewhere in stdlib that uses the function, in my quick search, and even if it is not used at the moment, it may be best to keep these functions in the Builtin module, so that variants of stdlib now and later can use them."

Second question...

"Assuming we are keeping this function, the above unit test doesn't make sense on a 32-bit platform. The only reason it hasn't broken in the past, arguably, is that before the PR I raised, the internal width of Builtin.Word was not specified to match the real value for the llvm target triple. What should be done with this unit test?"

My answer...

"I think it makes sense in my PR to separate out this unit test from the rest of sil_combine.sil and create versions for 32-bit targets and for 64-bit targets."

Do these answers make sense to you Joe? Do you think they are correct?

(Joe Groff) #4

It's intentional that Builtin.Word is always treated as a 64-bit integer at the SIL stage, since SIL tries to remain independent of target layout. The SILCombine-level behavior should not change because of target.

(Adrian Zubarev) #5

Offtopic: why are there two similar accounts (@carlos42421 and @carlos4242)? You probably should ask @forum_admins to merge them, no?

(John McCall) #6

If the [su]_to_[us]_checked_conversion_.* functions are unused, and we don't intend to add uses of them, we should remove them. Because of the restrictions on the builtin module, that does not require evolution approval, just a patch.

(Joe Groff) #7

@carlos42421 mentioned that he grepped the standard library for them, but the uses could be hidden behind gyb code generation. It wouldn't hurt to remove the builtins and see if anything breaks, though.

(John McCall) #8

I just grepped for checked_conversion and got no hits. It's hard to imagine how that part would be gyb'ed.

(Steve Canon) #9

Yeah, I'll be happy to review a "remove them and run tests to make sure nothing broke" PR.

(John McCall) #10

Yeah, it looks like they're a legacy of the era when we quadratically gyb'ed the value-preserving conversion initializers between integer types instead of having generic initializers defined in SignedInteger / UnsignedInteger extensions. I guess we decided that we got good enough behavior from that.

(Steve Canon) #11

Right, the protocols became rich enough and the compiler smart enough to get the codegen we need out of generic implementations.

(Carl Peto) #12

Cool. Shall I try that idea as a separate PR to the one I'm working on for 16-bit pointer fixes or all together? @scanon @John_McCall

@Joe_Groff - good point, I watched the video of your lecture about the design principles of SIL at the 2015 LLVM developers meeting. Although it's slightly artificial in this case I think? Because the stdlib type Int has a platform specific size. That means effectively Int has platform specific behaviour, it then uses the underlying internal type Builtin.Word when lowered into SIL which is considered to be int 64 and then lowers to IR, where the internal data is represented in a platform specific manner. Seems a bit odd? (I guess you could say that the Int datatype has always been odd, including in C.)

(@DevAndArtist - yeah... massively annoying, something to do with me using GitHub on my home and work machines and it not signing in using the same account. I'll try to fix later. carlos4242 is my real GitHub.)

(John McCall) #13

As a separate PR, please.

(Joe Groff) #14

We had tried to insulate SIL from the platform-specific size of Int in the past by implementing it in terms of Word, though nowadays Int uses the Builtin.IntNN type of the platform-specific size for other reasons. The only places we still use Word internally I believe are for pointer-to-integer conversions, which are still representable abstractly independent of the specific integer size.

(Carl Peto) #15

OK, I'll create a new PR for attempting to remove that Builtin and seeing if it breaks. With the slow build times on my laptop, plus doing it in the evenings I'm afraid it'll take me a few days.

On the Builtin.Word related unit test, I'll go back to my original PR and rethink it based on what you've said Joe.

(Steve Canon) #16

I'm fine with you throwing up a PR and letting the CI test it, in this particular case, since we're collectively pretty confident that nothing will break.

(Carl Peto) #17

For people following this thread later, the discussion has now moved to this PR: