On Thu, 24 Oct 2024 11:51:23 GMT, Quan Anh Mai <qa...@openjdk.org> wrote:
>> Ah, do you mean having a method in `Node` that holds the lowering code? I >> was originally planning on doing it this way, but I think it would pose some >> problems where certain nodes' `Lower()` methods would only be overridden on >> certain backends, which would become messy. One of my goals was to keep the >> lowering code separate from shared code, so new lowerings could be >> implemented by just updating the main `lower_node` function in the backend. >> About GVN, I think it makes sense to do it in a separate phase because GVN >> is used quite generally whereas lowering is only done once. Since the >> `transform_old` function in IGVN is pretty complex as well, I think it's >> simpler to just implement `Value()` and GVN separately. Thinking on it more >> I think Identity is probably a good idea too, since as you mention it can't >> introduce new nodes into the graph. Mainly I wanted to avoid the case where >> `Ideal()` could fold a lowered graph back into the original form, causing an >> infinite loop. > > I mean we might want to run another kind of `Ideal` that will replace the > normal `Ideal` on a node after its lowering. For example, consider this: > > vector<int,8> v; > u = v.withLane(0, a).withLane(1, b); > > This will be parsed into: > > vector<int,8> v; > v0 = InsertI(v, 4, a); > u = InsertI(v0, 5, b); > > And can be lowered to: > > vector<int,8> v; > vector<int,4> v1 = VectorExtract(v, 1); > v2 = InsertI(v1, 0, a); > v0 = VectorInsert(v, 1, v2); > vector<int,4> v3 = VectorExtract(v0, 1); > v4 = InsertI(v3, 1, b); > u = VectorInsert(v0, 1, v4); > > Which represents this sequence: > > ymm0; > vextracti128 xmm1, ymm0, 1; > vpinsrd xmm1, xmm1, a, 0; > vinserti128 ymm0, ymm0, xmm1, 1; > vextracti128 xmm1, ymm0, 1; > vpinsrd xmm1, xmm1, b, 1; > vinserti128 ymm0, ymm0, xmm1, 1; > > As you can imagine this sequence is pretty inefficient, what we really want > is: > > ymm0; > vextracti128 xmm1, ymm0, 1; > vpinsrd xmm1, xmm1, a, 0; > vpinsrd xmm1, xmm1, b, 1; > vinserti128 ymm0, ymm0, xmm1, 1; > > Looking back at the graph, we can `Identity` `v3` into `v2` since it is > pretty obvious that we just do an insert and extract from the same place. > However, to transform `u = VectorInsert(v0, 1, v4)` into `u = VectorInsert(v, > 1, v4)`, we would need an `Ideal`-like transformation to see that we just > insert into a location twice and remove the intermediate `VectorInsert`. > > As a result, in addition to ease of implementation, I think you may extend > `PhaseIterGVN` and override its `PhaseGVN::apply_ideal` to return `nullptr` > for now, and take advantages of `PhaseIterGVN::optimize` to do the iterative > transformation for you. Ah, I see what you mean now. I think this makes extending IGVN more appealing because we could continue to do Ideal on lowered nodes, as you mentioned. We could override `PhaseGVN::apply_ideal` to return `nullptr` when processing regular nodes, but run the other `Ideal` type when encountering lowered nodes. Do you think it would be better to add another method to `Node` or should we re-use the existing Ideal call, but lowering specific nodes are guarded with a new node flag? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1818344051
