In D123231#3584389, @ruiling wrote:

I had tried a different approach to avoid inserting excessive number of boolean values during the loop-exit-unify in D127831. I just did some testing of that change against the LLVM IR Brendon shared with me. It shows the change could help reducing the number of registers as well as compile time. But it is sad that I still hit the error: "unhandled SGPR spill to memory" from SGPRSpillBuilder in SIRegisterInfo.cpp. Can the limitation be fixed? I did some register pressure comparison, seems the way I proposed would use much less VGPR than (D123230 + D123231), but use more SGPR. I haven't looked further why there is such behavior difference. I think we need more investigation to know why. But looks like D127831 might help us generate better code because we can use one VGPR as the backup storage for spilling of 64/32 SGPRs. And the idea used there is much easy to follow.

Same typo in the change description, last paragraph ... middle "of" the block.

Looks straightforward enough to me. If the value is a PHI, it must be at the beginning of the current block. If it is outside this block, then whoever registered it is confident that it dominates this block. Is it correct that the SSAUpdater will not end up in a state where it tries to use a phi to produce another phi in the same block? Because that might race against the assumption that the used phi is available earlier in the block.

The change itself looks okay to me, to the extent that it does not seem to break the structurizer. Just by putting nodes in the right order, it is still able to generate predicates that preserve the original control flow. I think that's good enough to boldly make this change. I do have some comments/questions about the tests.

The code looks okay to me, but still need @arsenm to check the effect on the AMDGPU lit test. There seem to be superficial changes to the control flow, but Matt might be able to say whether the test still serves its original purpose after this change.

In D126525#3544916, @skatkov wrote:

In D126525#3544896, @sameerds wrote:

Even then, a target independent test in LLVM IR will certainly be useful.

That would be good but SSA Updater is not triggered directly as a pass. I found the motivation case in tailduplication codegen pass, so I've written it in mir format. Writing it in LLVM IR will just include a lot of other passes where the changes might disappear the case I'm testing. I though the more precise test is better than something general.

If you'd like I can try to create unittest triggering directly SSA Updater.

In D126525#3544805, @skatkov wrote:

In D126525#3542391, @djtodoro wrote:

This is interesting! Can you please make a simple/dedicated LLVM IR case for this only, so we can go over the example to get the whole picture?

llvm/test/CodeGen/AArch64/tail-dup-redundant-phi.mir is dedicated test for this patch and https://reviews.llvm.org/D126523. This test is not landed yet. I just uploaded it here as diff to show the difference.

Requesting @arsenm to take a look at the AMDGPU test.

LGTM! My sincere apologies for the delay ... I have no excuse :(

Sorry I missed this earlier. The name of the new test "issue55099.ll" doesn't really convey much information. It should be renamed to something more informative, like "preserve-child-loops.ll"

In D125522#3521160, @sunziping2016 wrote:

Sorry, I'm new here, so I know little about the workflows. I've just updated the title and the summary of this revision. I don't know whether you mean this. And I've noticed there is a build failure on x64 debian. Should I fix this?

I think the change description is missing the point and needs to be rephrased. The modified function was incorrectly (not unnecessarily) ignoring grandchild loops, and this change fixes the bug. In particular, this fixes the handling of the loop { inner, body }. The TODO in the same function is talking about the b1 self loop, which may be "unnecessarily" lost, but that is a different issue.

I am not much familiar with DirectX and HLSL, so unable to review this patch. It might help to post on the Discourse under Clang Frontend:

The changes in GenericCycleInfo look good to me. But please do wait for reviewers looking at MachineSink.

Do note that the cycle hierarchy is implementation-defined, and hence the reported cycle depth is not an invariant property of the program being compiled. For example, in the following CFG:

In D123231#3445997, @ruiling wrote:

which increases compilation time and register pressure.

Have you looked at which part is responsible for the compilation time increase? Is is possible that we hit inefficiency in certain pass?
The "register pressure" here specifically means SGPR usage, right?

I don't think it is a good idea to move exist blocks into the loop especially when threads in a wave exit the loop in different iterations. Executing the exit block after loop should benefit for runtime performance.

In D123231#3445000, @foad wrote:

In specific cases, this patch reduces the number of values needed
after loop by moving the exit block to the loop.

I think "into the loop" would be clearer if that's what you mean.

This patch clearly follows the same pattern as other attributes. At least I didn't spot anything different. Please give it a day for others to comment, otherwise good to go.

Needs more reviewers ... I would suggest authors of previous non-trivial changes to the structurizer, and their reviewers.

LGTM! But not sure if @yaxunl and/or @arsenm should have another look at the final version.

In D121951#3411856, @scott.linder wrote:

@yaxunl Does excluding device-libs via COV_None make sense?

@sameerds Would you still rather we just not add this attribute in the frontend at all? I'm OK with it if that is the consensus

In D121951#3392470, @scott.linder wrote:

In D121951#3391472, @sameerds wrote:

The check for "__ockl_hostcall_internal" is not longer relevant with the new hostcall attribute. Can we simply remove this check? What is the situation where the warning is still useful?

I wasn't aware of the new attribute, I'm happy to just delete it.

The check for "__ockl_hostcall_internal" is not longer relevant with the new hostcall attribute. Can we simply remove this check? What is the situation where the warning is still useful?

Please remove the JIRA ticket number from the commit description.

Please wait for a day to give other reviewers a chance to respond.

Please fix the commit description so that the first line is self-contained and separated from the rest by a blank line. This matters a lot when looking at the output of "git log ---oneline". In particular, the start with "in this work we implement" is pretty much unnecessary.

Please fix the commit description so that the first line is self-contained and separated from the rest by a blank line. This matters a lot when looking at the output of "git log ---oneline".

use AAPointerInfo; add more tests to demonstrate the same

added tests for i128 load. hostcall position is now independent of subtarget.

further simplified the callback return value; moved hostcall info out of the subtarget

eliminated NeedsHostcall by simply checking the return value of the walk over all calls

Fixed stale comments that mention the attribute amdgpu-queue-ptr in the positive sense.

In D112696#3181899, @sameerds wrote:

In D112696#3177463, @JDevlieghere wrote:

I've reverted this in 02940d6d22022dbad45efa0c9a5cc772ba844da4 because it breaks the modules builds:

https://green.lab.llvm.org/green/job/clang-stage2-rthinlto/4858/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39112/

I managed to reproduce the breakage locally with -DLLVM_ENABLE_MODULES=On. With my latest revision, I can see that the error specifically mentioning my files has disappeared, but the build does not complete. @JDevlieghere do you have any advice on perfectly reproducing that build locally? Are there other things to be specified on the cmake line?

In D112696#3177463, @JDevlieghere wrote:

I've reverted this in 02940d6d22022dbad45efa0c9a5cc772ba844da4 because it breaks the modules builds:

https://green.lab.llvm.org/green/job/clang-stage2-rthinlto/4858/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39112/

• rebase
• The opaque member class Compute broke module builds.
• Moved compute to an extern template class in namespace llvm.
• Cleaned out a spurious class declaration that was not used anywhere.

Reopening revision to help with pre-submit checks, before reapplying the reverted commit.

• rebase
• addressed review comments

• rebase
• removed the "vacuous root" from the cycle description

• rebase
• replace Dfg and Ssa with uppercase versions
• eliminate any interpretation of nullptr as root cycle
• move two functions from CycleInfo to Cycle, just like LoopInfo
• small cleanup to the cycle terminology, pointed out by @nhaehnle

In D114146#3142269, @simoll wrote:

All good points. PDT and the comment on strict loop compactness are leftovers. I will remove PDT in a followup patch.

Not related to the changes proposed in this review, but it seems the PDT is not used by the implementation. It might be good to take out that dependency.

rebase

• Fixed some doxygen comments.
• Eliminated the "root" cycle.
• Cycle should not point to CycleInfo; it's hard to update that reference. Fixed the side-effects of eliminating that reference.
• Fixed names mentioned in review comments.
• Extracted two helper functions: getTopLevelParent() and moveToNewParent(). Both very helpful in improving readability.
• SsaContext now keeps a reference to the Function so that user analyses can retrieve it. Forward looking change, found to be useful in later analyses not introduced yet.

"uint" broke the Windows build

• added specific wording from paper to description of cycle tree
• added LLVM_DEBUG to implementation
• fixed typos

Addressed review comments