Add sextOrBitCast to conversions of Builtin.Word #19600

Kaiede · 2018-09-28T00:16:06Z

An error was hit when attempting to build Swift on a 32-bit Linux host. It was asserting when attempting to run ‘RedunantLoadElim” during the optimizer. The reason why is a bit messy.

First, Builtin.Word is always considered to be 64-bit when it is a SILType. This means the optimizer, when working with a Builtin.Word, will create 64-bit APInts when converting from other types.

Second, the SIL being output for a particular literal looked like this:
Int2048 (integer_literal) : Int32 (Signed Truncation) : Word (zextOrBitCast)

The constant fold behavior would convert this into an integer_literal of the Builtin.Word type. But because of the zext cast, if the original literal was -1, it would become +4 billion during folding.

This normally isn’t a problem except when SIL is still attempting to optimize. (See First)

Third, The Redundant Load Elimination pass was attempting to make sense of an index_addr instruction which was indexing at -1. This happens when you perform “-= 1” on an UnsafePointer. Because SIL was interpreting Word’s size incorrectly, it caused the RLE pass to ask “what is at ptr[4billion]?” Since there’s no feasible answer for such a question, the answer was “nothing”, which tripped the assert.

This fix uses sign extension when converting “upward” and the Integer type is signed, otherwise it will still use zero extension.

Resolves SR-8870.

Kaiede · 2018-09-28T00:18:29Z

FYI, I'm still doing a couple last checks on this change. It should be fine, but I re-enabled the zext conversions after my last build.

It also lacks a discrete test. But I'd be happy to take pointers on what sort of test this should have.

stdlib/public/core/IntegerTypes.swift.gyb

milseman · 2018-09-28T14:12:26Z

A test may have to be written in SIL for the iPhone simulator, if that's the only place this change is visible on Darwin.

@eeckstein who can advise?

An error was hit when attempting to build Swift on a 32-bit Linux host. It was asserting when attempting to run ‘RedunantLoadElim” during the optimizer. The reason why is a bit messy. First, Builtin.Word is always considered to be 64-bit when it is a SILType. This means the optimizer, when working with a Builtin.Word, will create 64-bit APInts when converting from other types. Second, the SIL being output for a particular literal looked like this: Int2048 (integer_literal) : Int32 (Signed Truncation) : Word (zextOrBitCast) The constant fold behavior would convert this into an integer_literal of the Builtin.Word type. But because of the zext cast, if the original literal was -1, it would become +4 billion during folding. This normally isn’t a problem **except** when SIL is still attempting to optimize. (See First) Third, The Redundant Load Elimination pass was attempting to make sense of an index_addr instruction which was indexing at -1. This happens when you perform “-= 1” on an UnsafePointer. Because SIL was interpreting Word’s size incorrectly, it caused the RLE pass to ask “what is the ProjectionKind at ptr[4billion]?” Since there’s no feasible answer for such a question, the answer was “nothing” when the Projection’s kind() was checked, which tripped the assert. This fix uses sign extension when converting “upward” and the Integer type is signed, otherwise it will still use zero extension.

jckarter

LGTM

Kaiede · 2018-09-28T16:35:28Z

I've updated with the feedback from @xwu and my last set of checks with that version are good.

@milseman Regarding the test, I'm not 100% sure that writing it in SIL really helps? The change only affects the SIL generated from the Int32/UInt32 (and Int64/UInt64) types, and so any test in SIL is really testing the "Builtin.Word is always 64-bit during Optimization" behavior during constant folding. Although maybe @eeckstein has a better idea how to write a test to prevent regressions here.

EDIT: Alternatively, since this bug causes a build break and you cannot build the stdlib if the Optimizer is throwing a fit, perhaps that is a sufficient test for now? But if that's true, it does stress the need for a reliable CI node on a 32-bit Linux host. x86 Linux would be ideal for this, IMO, but the ELF32 dynamic linker issues with LLVM would need to be fixed before that can be reality.

jckarter · 2018-09-28T17:29:31Z

I feel like a source-level test would be the most robust thing. Execution-testing some Swift code that exercises one of the formerly problematic situations should be more robust and ensure that any refactorings we do in the future here don't break things even if all the implementation details completely change.

Kaiede · 2018-09-28T17:50:20Z

So effectively a snippet where if it compiles, the test passes, and if it doesn't compile, the test fails? Something like the following maybe?

func uninlinableFunc(ptr: UnsafePointer<UInt8>) -> UInt8? {
    ptr -= 1 // Causes an index_addr of -1 to be emitted.
   return ptr.pointee // Causes us to load the result of index_addr after optimization
}

let base: UnsafePointer<UInt8> = // Allocate some memory
// Write Something Interesting into the elements of memory
let index = base + 2
print(uninlinableFunc(ptr: index))

eeckstein · 2018-09-28T20:24:56Z

This fix is in the stdlib.
But we should also fix the crash in RLE. Optimizations should not crash for any swift input code.

Regarding tests: I agree with @jckarter that the best test for the stdlib fix would be an execution test.
For a fix of the RLE crash, a SIL test would be the better (even if it only triggers on 32-bit linux).

Kaiede · 2018-09-28T22:46:56Z

This fix is in the stdlib.
But we should also fix the crash in RLE. Optimizations should not crash for any swift input code.

Which is fair, although the fix for SIL Optimizer is a bit more complicated (the thread is gnarly with details). I kinda hope the plan there wouldn't be to just abort the optimization if given a nonsense location to check for loading? The real fix should be to actually fix it so that Builtin.Word is the correct size for the target so constant folding doesn't produce the nonsense, but seeing as that's been marked with FIXMEs already, I suspect someone's already looked at the amount of work that is and decided against it at that time.

My thoughts here are that the RLE piece is behaving as expected by asserting that ptr[4294967295] is nonsensical when creating the swift::Projection. What do you think the fix is here?

Kaiede · 2018-09-29T15:55:23Z

Turns out there’s already a test for RLE which catches this issue, it’s just a bit hard to run when stdlib is failing to build. So the SIL test for this does exist, and does fail, even with the stdlib fix. So, I’ll eat my hat a bit, since I guess it’s also established in the code history that this optimization shouldn’t be failing like this.

jckarter · 2018-10-01T16:58:52Z

@swift-ci Please test

swift-ci · 2018-10-01T16:59:21Z

Build failed
Swift Test OS X Platform
Git Sha - ccb415ad09f6201b3adc0bc0f5854063ae14d69b

swift-ci · 2018-10-01T17:00:37Z

Build failed
Swift Test Linux Platform
Git Sha - ccb415ad09f6201b3adc0bc0f5854063ae14d69b

Kaiede · 2018-10-01T17:45:03Z

Ugh, it looks like the CI doesn't like that I did an amend commit here. Lesson Learned. ☹️

jckarter · 2018-10-01T17:48:01Z

No problem, it restarts automatically.

Kaiede · 2018-10-01T17:50:46Z

Yeah, just weird that it was trying to sync a commit that no longer exists.

jckarter · 2018-10-01T18:52:35Z

Yeah, that's unfortunately a bug in the Jenkins git plugin we've had to live with for a while.

jrose-apple requested review from moiseev, jckarter and milseman September 28, 2018 01:18

xwu reviewed Sep 28, 2018

View reviewed changes

stdlib/public/core/IntegerTypes.swift.gyb Outdated Show resolved Hide resolved

jckarter approved these changes Sep 28, 2018

View reviewed changes

Kaiede mentioned this pull request Sep 28, 2018

Enable i686 as a Host Platform #19607

Merged

Kaiede mentioned this pull request Oct 1, 2018

[arm32] [4.2] assertion 'hasVal' failed linking Swift.o uraimo/buildSwiftOnARM#26

Closed

jckarter merged commit b824f28 into apple:master Oct 1, 2018

moiseev mannequin mentioned this pull request Oct 9, 2018

[SR-8870] _builtinWordValue should zext/sext based on signedness #51376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sextOrBitCast to conversions of Builtin.Word #19600

Add sextOrBitCast to conversions of Builtin.Word #19600

Kaiede commented Sep 28, 2018

Kaiede commented Sep 28, 2018 •

edited

milseman commented Sep 28, 2018

jckarter left a comment

Kaiede commented Sep 28, 2018 •

edited

jckarter commented Sep 28, 2018

Kaiede commented Sep 28, 2018 •

edited

eeckstein commented Sep 28, 2018

Kaiede commented Sep 28, 2018 •

edited

Kaiede commented Sep 29, 2018

jckarter commented Oct 1, 2018

swift-ci commented Oct 1, 2018

swift-ci commented Oct 1, 2018

Kaiede commented Oct 1, 2018

jckarter commented Oct 1, 2018

Kaiede commented Oct 1, 2018

jckarter commented Oct 1, 2018

Add sextOrBitCast to conversions of Builtin.Word #19600

Add sextOrBitCast to conversions of Builtin.Word #19600

Conversation

Kaiede commented Sep 28, 2018

Kaiede commented Sep 28, 2018 • edited

milseman commented Sep 28, 2018

jckarter left a comment

Choose a reason for hiding this comment

Kaiede commented Sep 28, 2018 • edited

jckarter commented Sep 28, 2018

Kaiede commented Sep 28, 2018 • edited

eeckstein commented Sep 28, 2018

Kaiede commented Sep 28, 2018 • edited

Kaiede commented Sep 29, 2018

jckarter commented Oct 1, 2018

swift-ci commented Oct 1, 2018

swift-ci commented Oct 1, 2018

Kaiede commented Oct 1, 2018

jckarter commented Oct 1, 2018

Kaiede commented Oct 1, 2018

jckarter commented Oct 1, 2018

Kaiede commented Sep 28, 2018 •

edited

Kaiede commented Sep 28, 2018 •

edited

Kaiede commented Sep 28, 2018 •

edited

Kaiede commented Sep 28, 2018 •

edited