C++/C#: Remove Instruction::getResultType() and friends
#2217
Conversation
Now that the language-neutral IR type system is in place, this PR gets rid of the language-specific IR type APIs that caused the original problem. I've fixed up a fair bit of code to avoid these, including range analysis and sign analysis. These last two are almost shareable between C# and C++ now that they depend on `IRType`, but I haven't done the actual work to officially share them.
| @@ -799,11 +758,13 @@ class ConstantInstruction extends ConstantValueInstruction { | |||
| } | |||
|
|
|||
| class IntegerConstantInstruction extends ConstantInstruction { | |||
| IntegerConstantInstruction() { getResultType() instanceof Language::IntegralType } | |||
| IntegerConstantInstruction() { | |||
| getResultIRType() instanceof IRIntegerType or getResultIRType() instanceof IRBooleanType | |||
There was a problem hiding this comment.
A line break would make this clearer.
|
The qlformat test is failing. |
| i.getResultType() instanceof IntegralType or | ||
| i.getResultType() instanceof PointerType | ||
| i.getResultIRType() instanceof IRIntegerType or | ||
| i.getResultIRType() instanceof IRAddressType |
There was a problem hiding this comment.
I was just trying to use the range analysis library to validate an index into a stack-allocated array, and I found that it didn't work. After a long debugging session, I figured out that it was because this predicate and SafeCastInstruction didn't treat a glvalue as a pointer, so a local variable subjected to array-to-pointer decay would not become a Bound. After I'd fixed that, I remembered this PR and came back here to discover that this innocent-looking change also has exactly that effect! So that's good.
The question is now what it means for performance that all variable addresses are now Bounds. @rdmarsh2, what do you think about these changes to the range analysis library? @dave-bartolomeo, please check how performance and size of boundedInstruction change.
There was a problem hiding this comment.
For local variables with an array type or a struct type with an array member, that should be an improvement. I'm a bit nervous about the performance implications of all variable addresses being bounds. It should have a bigger impact on the size of boundedInstruction than on the time to compute the stage, but that means the performance hit will come from users of the library rather than the library itself.
| } | ||
|
|
||
| class FloatConstantInstruction extends ConstantInstruction { | ||
| FloatConstantInstruction() { getResultType() instanceof Language::FloatingPointType } | ||
| FloatConstantInstruction() { getResultIRType() instanceof IRFloatingPointType } |
There was a problem hiding this comment.
Would tend to prefer this.getResultIRType() here and elsewhere.
|
@dbartol A friendly reminder that this PR is needed by @Cornelius-Riemenschneider for his project of extending the range analysis to be interprocedural. |
|
Any update on this? |
Now that the language-neutral IR type system is in place, this PR gets rid of the language-specific IR type APIs that caused the original problem. I've fixed up a fair bit of code to avoid these, including range analysis and sign analysis. These last two are almost shareable between C# and C++ now that they depend on
IRType, but I haven't done the actual work to officially share them.