Page MenuHomePhabricator

[MLIR][SPIRV] Add (de-)serialization support for SpecConstantOpeation.
ClosedPublic

Authored by ergawy on Dec 20 2020, 2:56 AM.

Details

Summary

This commit adds support for (de-)serializing SpecConstantOpeation. One
thing worth noting is that during deserialization, we assign a fake ID to
enclosed ops inside SpecConstantOpeation. We need to do this in order
for deserialization logic to properly update ID to value map and to
later reference the created value from the sibling 'spv::YieldOp'.

Diff Detail

Event Timeline

ergawy created this revision.Dec 20 2020, 2:56 AM
ergawy requested review of this revision.Dec 20 2020, 2:56 AM
ergawy added inline comments.Dec 20 2020, 2:59 AM
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
3460

That was an implementation error in my previous patch. Apologies for the no-so-thorough testing. Fixed and added a test.

ergawy updated this revision to Diff 312974.Dec 20 2020, 5:45 AM

Add more comments.

mravishankar requested changes to this revision.Dec 21 2020, 1:55 PM
mravishankar added inline comments.
mlir/lib/Target/SPIRV/Deserialization.cpp
1738

This works fine I guess, but what do you think about just "creating a new stack"

Basically valueMap is the current defined values. You can create an new map and use that to collect values from within the region

DenseMap<uint32_t, Value> newValueMap;
std::swap(valueMap, newValueMap);

<... process the body ...>

std::swap(newValueMap, valueMap)

Sorry if this is totally off here. I might be missing some context (this has been flushed from cache now)

This revision now requires changes to proceed.Dec 21 2020, 1:55 PM
ergawy added inline comments.Dec 22 2020, 1:54 AM
mlir/lib/Target/SPIRV/Deserialization.cpp
1738

Thanks for your comment and review.

I might have missed what you mean but the problem here is that auto-generated deserialization methods depend on the list of input operands having an ID for the corresponding value in SPIR-V input module. For OpSpecConstantOp, we have a special case: there no SPIR-V ID for the result of the wrapped/enclosed op. So what is represented as a single ID (SSA value) in the SPIR-V module needs to be represented using 2 MLIR SSA values in the spv one:

  • The MLIR ID for the result of spv.SpecConstantOperation.
  • The MLIR ID for the result of wrapped/enclosed op within its region.

Let me know if I misunderstood.

ergawy updated this revision to Diff 314506.Jan 4 2021, 10:48 PM

Rebase and bing! :).

antiagainst requested changes to this revision.Jan 5 2021, 6:36 AM

Thanks for the contribution and sorry about the delay; I was out of office for quite some time. :)

mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
3460

No worries. I didn't catch this properly either. :)

mlir/lib/Target/SPIRV/Deserialization.cpp
1738

You can still have the fake ID for the enclosed instruction. We are constructing a transient instruction for the enclosed instruction and invoke the normal instruction processing functionality for it. What Mahesh means is swapping the "context" before processing the transient instruction. The context includes the valueMap and insertion point. So before parsing the transient instruction, if we swap the valueMap with a temporary specOpValueMap, then we are still inserting the fake ID into the specOpValueMap. It's just that the specOpValueMap will only ever contain one fake ID and there is no chance we collide. The specOpValueMap can just live in this particular function. :)

1739

s/spv/SPIR-V/. Similarly for the following occurances.

In comment we prefer the fully-spelled-out name as we don't suffer from symbol limitations as in code. :)

1759

s/materialzied/materialized/

1767

What about first creating the spec op, set the insertion point to its body (with proper guard), and then process the transient instruction? This can avoid us from doing the block manipulation right? Following the above comment, this is part of swapping the "context".

mlir/lib/Target/SPIRV/Serialization.cpp
745

This isn't correct. Spec constant is a kind of constant; spv.SpecConstantOperation also follows in this category. So it should be in typesGlobalValues. This becomes a bit tricky and sorry I didn't catch this earlier. The deserialization will become more complicated because the spv.SpecConstantOperation will be parsed from module scope and we want it to be in function scope. This isn't something entirely new though; it's how spv.constant are handled. They are also parsed in the module scope and materialized in function scope when use. You can see how it's handled in getValue and constantMap. For spv.SpecConstantOperation, we need to keep track of the enclosed opcode and its operands' result IDs. When materializing, use getValue to automatically materialize operand normal/spec constant, and then construct the spv.SpecConstantOperation.

mlir/test/Target/SPIRV/spec-constant.mlir
104

Change one of the operand to be a normal constant?

This revision now requires changes to proceed.Jan 5 2021, 6:36 AM
ergawy updated this revision to Diff 315302.Jan 7 2021, 11:18 PM
ergawy marked 4 inline comments as done.

Handle review comments:

  • Use different value map for emitting SpecConstOperation.
  • Small change in testing code.
ergawy added inline comments.Jan 7 2021, 11:18 PM
mlir/lib/Target/SPIRV/Deserialization.cpp
1738

Ah, I see what Mahesh and you mean. I guess that works since OpSpecConstantOp takes operands whose instructions are materialized while the consuming instruction is being emitted.

1767

If we do this, since the transient instruction has operands that are constants, global vars, or spec constants, then the instructions to materialize these references will be emitted in the spec op's region. This is the main reason I think we need such manipulation here; we first need to materialize all references, if any, in the parent region and then isolate the spec op's enclosed op.

Let me know if what you are saying is going over my head :D.

ergawy marked an inline comment as done.Jan 7 2021, 11:19 PM
antiagainst requested changes to this revision.Jan 9 2021, 6:32 AM

Awesome! I just have a few nits left now. Marked as blocking because we need to avoid change Clang code.

clang-tools-extra/clangd/ClangdLSPServer.cpp
623 ↗(On Diff #315302)

Accidental change or rebase leftover?

Anyway we need to revert the change here. :)

mlir/lib/Target/SPIRV/Deserialization.cpp
521

Just define a struct instead of using std::tuple? It will make the code more readable. Also we should use SmallVector instead of ArrayRef here. I guess ArrayRef is okay because we have access to the underlying SPIR-V blob right now; but it can be a potential hazard given ArrayRef does not owns the data.

1758

s/creaed/created/

1763

What about using llvm::SaveAndRestore to avoid manually swap?

1767

You are right. Sorry I left this comment earlier than the comment about serializing into typesGlobalValues.

This revision now requires changes to proceed.Jan 9 2021, 6:32 AM
ergawy updated this revision to Diff 315658.Jan 10 2021, 1:10 AM
ergawy marked 5 inline comments as done.

Handle review comments.

ergawy marked 2 inline comments as done.Jan 10 2021, 1:11 AM
ergawy added inline comments.
clang-tools-extra/clangd/ClangdLSPServer.cpp
623 ↗(On Diff #315302)

No idea where that came from. Rebasing made it go away.

antiagainst accepted this revision.Jan 10 2021, 7:58 AM

Awesome, thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 10 2021, 10:59 PM
This revision was automatically updated to reflect the committed changes.
ergawy marked an inline comment as done.