SIL: let SingleValueInstruction only inherit from a single SILNode. #35554
Conversation
|
@swift-ci test |
|
Build failed |
There was a problem hiding this comment.
Looks great! I'm happy to see the representative node stuff go away. And I think SILNode should be an implementation detail whose type isn't used outside the implementation of SILValue/Instruction.
What's the size of SingleValueInstruction now?
Loosely related commentary:
I would not want to add "dirty" bits to SILInstruction because it's generally bad design to cache analysis results in the IR, and you could reap the same benefits just by maintaining a unique ID or even an ordering. That lets you easily manage dense data structures that contain analysis results and can be useful for fast checks on relative instruction order. It's a lot harder to manage with instructions vs. blocks though. For ordering, you would want to leave holes in the numbering, and even with unordered IDs it'll end up with plenty of holes. That degrades the performance of data structures and get into a nasty game of heuristics.
Replace the `isa(SILNode *)` with `isa(SILInstruction *)` and `isa(SILValue)`. This is much clearer and it also works if the SILValue is a MultiValueInstructionResult of an apply instruction. Also, use `isa` instead of `classof` in canOptimize()
Split `markValueLive(SILNode *)` into `markValueLive(SILValue)` and `markInstructionLive(SILInstruction*)`
… when checking for invariant arguments. Use `getDefiningInstruction` instead of casting a a SILValue to SingleValueInstruction.
…om SILNode* to SILValue A small cleanup, NFC.
This resolves a FIXME. Also, use `getDefiningInstruction()` instead of `getRepresentativeSILNodeInObject()` NFC
…tUsersOfInstruction. A small cleanup, NFC.
…tion *)` A small cleanup, NFC
A small cleanup, NFC.
This removes the ambiguity when casting from a SingleValueInstruction to SILNode, which makes the code simpler. E.g. the "isRepresentativeSILNode" logic is not needed anymore. Also, it reduces the size of the most used instruction class - SingleValueInstruction - by one pointer. Conceptually, SILInstruction is still a SILNode. But implementation-wise SILNode is not a base class of SILInstruction anymore. Only the two sub-classes of SILInstruction - SingleValueInstruction and NonSingleValueInstruction - inherit from SILNode. SingleValueInstruction's SILNode is embedded into a ValueBase and its relative offset in the class is the same as in NonSingleValueInstruction (see SILNodeOffsetChecker). This makes it possible to cast from a SILInstruction to a SILNode without knowing which SILInstruction sub-class it is. Casting to SILNode cannot be done implicitly, but only with an LLVM `cast` or with SILInstruction::asSILNode(). But this is a rare case anyway.
eeckstein
force-pushed
the
single-silnode-in-singlevalueinstruction
branch
from
5220ca2 to
ff19917
Compare
|
@swift-ci smoke test |
|
@atrick the size of SingleValueInstruction is still 10 words (2 for the inst-list node, 1 for the parent, 3 for ValueBase and 4 for the debug location). Though, I believe we could get the debug location down to 3 words, or even less. |
|
@eeckstein oh well, ideally the debug location would be encoded in one word and the first operand could reside in the same cache line. |
| return ApplySite(); | ||
|
|
||
| auto kind = ApplySiteKind::fromNodeKind(i->getKind()); | ||
| static ApplySite isa(SILInstruction *inst) { |
There was a problem hiding this comment.
@eeckstein I wish you had also renamed this to cast or get. It is weird that this is isa since it doesn't return a bool.
I had a feature in mind which would have increased our instruction size. To not feel so bad about it, I tried to reduce the instruction size on another end. So I came up with this change. At the end I decided to not implement the feature, but I thought this change is a good thing, anyway.
Letting SingleValueInstruction only inherit from a single SILNode removes the ambiguity when casting from a SingleValueInstruction to SILNode, which makes the code simpler. E.g. the "isRepresentativeSILNode" logic is not needed anymore.
Also, it reduces the size of the most used instruction class - SingleValueInstruction - by one word.
Conceptually, SILInstruction is still a SILNode. But implementation-wise SILNode is not a base class of SILInstruction anymore.
Only the two sub-classes of SILInstruction - SingleValueInstruction and NonSingleValueInstruction - inherit from SILNode. SingleValueInstruction's SILNode is embedded into a ValueBase and its relative offset in the class is the same as in NonSingleValueInstruction (see SILNodeOffsetChecker).
This makes it possible to cast from a SILInstruction to a SILNode without knowing which SILInstruction sub-class it is.
Casting to SILNode cannot be done implicitly, but only with an LLVM
castor withSILInstruction::asSILNode(). But this is a rare case anyway.Only the last commit of this PR does the actual SILNode change. All previous commits contain small cleanups, which are required for the change, but are good for their own.