[SILOpt] Add ZeroInitializer to isValidStaticInitializerInst

[davezarzycki]

No description provided.

[davezarzycki]

@swift-ci please smoke test

[davezarzycki]

Ping. Hi @gottesmm and @eeckstein – This seems like a simple QoI fix, right? Am I missing anything?

[eeckstein]

This builtin needs to be supported in IRGen's emitConstantValue, too.
Can you add an "-emit-ir" test as well (you can do it in the same test file)?

[davezarzycki]

Hi @eeckstein – I feel like I'm missing something. Why does emitConstantValue need to special case BuiltinValueKind::ZeroInitializer when the code gen for this builtin already emits a constant? I ask because I'm not observing any behavioral differences with your suggestion.

[davezarzycki]

I did some sanity checks. The code you requested is definitely being called but it's not making an observable difference in the IR with the simple test I provided. Do I need some non-trivial Swift/SIL to observe the change you requested?

[eeckstein]

Hm, don't you hit the

llvm_unreachable("unsupported builtin for constant expression");

?

[rjmccall]

I think the key question is whether this is actually emitted as a constant initializer. I don't know if changing this one SIL predicate is sufficient for that.

[davezarzycki]

My mistake. I wasn't doing a proper A versus B comparison. Patch updated.

@swift-ci please smoke test

[davezarzycki]

Updated CHECK to use a regular expression to account for macOS versus Linux differences.

@swift-ci please smoke test

[eeckstein]

Can you add a test for this case?

[davezarzycki]

Sure, is there a good arbitrary width constant test I can copy from?

[eeckstein]

Can you fix the indentation here?

[davezarzycki]

Hmm... I just ran the code through git clang-format and this was the sub-optimal result. I can either re‑indent all of the case statements to match clang style, or have this change match the rest of the case statements. What would you prefer?

[eeckstein]

I don't mind

[davezarzycki]

I dropped support for arbitrary width integers for two reasons: 1) using Builtin.zeroInitializer() with Builtin.IntLiteral type would be strange and 2) making that work would require more builtins to be handled in SIL and IRGen's constant handling logic.

I also overrode git clang-format and made the new case statement match the neighboring case statements.

@swift-ci please smoke test

[eeckstein]

Can you add a check here, which returns false if it's an arbitrary width integer?

[davezarzycki]

Updated to have isValidStaticInitializerInst return false if the zero initializer has arbitrary width.

@swift-ci please smoke test

[davezarzycki]

Replaced uses of AnyBuiltinIntegerType with BuiltinIntegerType to avoid arbitrary width problems.

@swift-ci please smoke test

[davezarzycki]

Added float and vector support. What's left before a LGTM?

@swift-ci please smoke test

[rjmccall]

Not supporting BuiltinIntegerLiteralType is appropriate since IIUC we want this builtin to guarantee a zero-bit representation, which is not a valid inhabitant of that type (there always has to be at least one word of data, to avoid that being a special case).

[eeckstein]

Thanks, LGTM!