Remove the special case for i32.
#4543
Conversation
For the few remaining uses of the builtin i32 type, manually build an `IntType(Signed, 32)` value instead. These are: - The return type of `Run`. - The type that int literals in an `if` expression are converted into. - The type of an array index expression. We should consider converting those three cases away from `i32` over time.
There was a problem hiding this comment.
Generally looks good, though I don't think this is removing all the special-casing I'd expected. Note too that Add(i) is going to be more efficient than AddUnsigned(llvm::APInt(64, i)). I think the order of preference should be:
- Add (because it can avoid an APInt conversion)
- AddSigned (because it can avoid an unsigned -> signed conversion)
- AddUnsigned (primarily exists for lex; see comments at Canonicalize away bit width and embed small integers into
IntIds #4487 (comment))
| @@ -88,9 +88,6 @@ class File : public Printable<File> { | |||
| // compute this information. | |||
| auto GetIntTypeInfo(TypeId int_type_id) const -> IntTypeInfo { | |||
| auto inst_id = types().GetInstId(int_type_id); | |||
| if (inst_id == InstId::BuiltinIntType) { | |||
There was a problem hiding this comment.
I think @chandlerc's intent had been to remove this full function upon the i32 type removal (see TODO), are you planning that as a separate PR? That is to say, this is still keepign some of the side-effects of i32 special-casing.
I'd particularly like to make sure there's a clear path forward due to the parallel of GetIntTypeInfo and IsSignedInt, given they're in separate files.
There was a problem hiding this comment.
Ah, I'd removed these TODOs, but that ended up in the next PR branch rather than this one. Now they're removed in this PR instead.
Given the existence of the IntLiteral type, which is an integer type that's not an IntType, I don't think the TODOs are appropriate. I did add a TODO here to move this function to TypeStore, though -- I think it should either go there or become a non-member function given that we have been trying to avoid File accumulating a bunch of utility methods.
| @@ -105,8 +105,7 @@ class TypeStore : public Yaml::Printable<TypeStore> { | |||
| // compute this information. | |||
| auto IsSignedInt(TypeId int_type_id) const -> bool { | |||
| auto inst_id = GetInstId(int_type_id); | |||
| if (inst_id == InstId::BuiltinIntType || | |||
| inst_id == InstId::BuiltinIntLiteralType) { | |||
| if (inst_id == InstId::BuiltinIntLiteralType) { | |||
There was a problem hiding this comment.
Note, similarly here about removal.
Co-authored-by: Jon Ross-Perkins <[email protected]>
Co-authored-by: Jon Ross-Perkins <[email protected]>
For the few remaining uses of the builtin
i32type, manually build anIntType(Signed, 32)value instead. These are:Run.ifexpression are converted into.We should consider converting those three cases away from
i32overtime.