[PATCH] D66924: [NewGVN] Add phi-of-ops instr as user of FoundVal.

Tue Mar 30 05:04:40 PDT 2021

fhahn updated this revision to Diff 334116.
fhahn added a comment.

Rebased & updated patch to be more targeted; we only need to add the operands as users if we do not create a real PHI node. In that case, the link between operands and PHI-of-ops is not materialized in IR.

In D66924#1852508 <https://reviews.llvm.org/D66924#1852508>, @efriedma wrote:

> If nobody else steps up to review, I will, I guess... although I probably won't have time next week.

Sorry for not getting back to you sooner Eli!

> Some very basic questions to help me get up to speed:
>
> 1. It looks like AdditionalUsers is related to the "In order to make the GVN mostly-complete" paragraph in the top of the file?  What's the general rule for figuring out whether a given user needs to be marked?

I think the purpose is slightly different and described here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/NewGVN.cpp#L545

I'll try to summarize as I understand it:

NewGVN works by notifying all users of a value, if the number of that value changed (e.g. because there's a new leader or a value to moved to a new CC). The problem with PHI-of-ops is that those are new PHIs that potentially get created along the way. Once we created an actual PHI node for a PHI-of-ops, there is a real IR instruction with real operands, so if any of those operands change, the updates will reach the PHI through the def-use chains in the IR.

But there are cases where we do not create a real PHI node (e.g. because it is not possible at the moment, or because it simplifies to an existing value). In those cases, we need to track values that may cause us to create a real PHI later on. This is done by adding values as 'additional' users of the PHI-of-ops root instruction. The main case is if we cannot find a valid operand for a PHI-of-ops in one of the predecessors. If any of the operands change, we might be able to create a real PHI-of-ops, so we need to re-visit the PHI-of-ops. We need to add the operands as additional users. (line 2735).

In the case this patch fixes, we do not create a real PHI node, because it simplifies to an existing value *at the moment*. No real PHI created means in this case the dependency between the PHI-of-op and its operands is not materialized in the IR. If any of the operands changes later on, we might not be able to simplify the PHI-of-ops to an existing value and we may need to create a real PHI. But in order to do so, we need to be notified once one of the operands to the PHI-of-ops changes. To do so, we need to add the operands as additional users.

> 2. It looks like there are currently seven callers of addAdditionalUsers, and this patch brings it up to 8.  Do you think with this patch, we've covered all the places that need to call it?  Are there any patterns related to addAdditionalUsers which could be refactored?

I think there's at least another case we are missing, which is causing https://bugs.llvm.org/show_bug.cgi?id=35074. I'll take another look at the underlying issue there.

I am not sure if there's a way to make this more general, while retaining the original intent. From my previous discussions with Daniel Berlin on this topic, my impression was that we should aim to populate AdditionalUsers as surgical as possible. For example, we could catch most/all issues in this area by always adding all operands of a PHI-of-ops as additional users. But this means we have to process many unnecessary and redundant users in a lot of cases and it effectively hides the places were we fail to track the right dependencies.

After writing all this reasoning up, I also think adding the additional users around lines 2752-2753 is actually unnecessary. But I think that's probably best addressed as follow-up.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66924

Files:
  llvm/lib/Transforms/Scalar/NewGVN.cpp
  llvm/test/Transforms/NewGVN/phi-of-ops-simplified-to-existing-value-then-changes-again.ll
  llvm/test/Transforms/NewGVN/pr36501-changed-after-3.ll
  llvm/test/Transforms/NewGVN/pr42422-changed-after-2.ll
  llvm/test/Transforms/NewGVN/pr42557-changed-after-1.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66924.334116.patch
Type: text/x-patch
Size: 27100 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210330/34c05f8b/attachment.bin>