[PATCH] D93591: [MLIR][SPIRV] Add (de-)serialization support for SpecConstantOpeation.

[Kareem Ergawy via Phabricator via cfe-commits]

ergawy added inline comments.


================
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1791
+
+  // Since the enclosed op is emitted in the current block, split it in a
+  // separate new block.
----------------
antiagainst wrote:
> 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".
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.


================
Comment at: mlir/lib/Target/SPIRV/Deserialization.cpp:1738
+
+  // Instructions wrapped by OpSpecConstantOp need an ID for their
+  // Deserializer::processOp<op_name>(...) to emit the corresponding spv dialect
----------------
antiagainst wrote:
> ergawy wrote:
> > mravishankar wrote:
> > > 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)
> > 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.
> 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. :)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93591/new/

https://reviews.llvm.org/D93591