Thanks for the fix. LGTM.

In D131276#3703399, @cfang wrote:

In D131276#3703034, @b-sumner wrote:

In D131276#3702934, @arsenm wrote:

I also think we're still missing a module flag to indicate the code object version

The module flag was implemented quite a while back. See D119026 from February. From a recent compile:

!llvm.module.flags = !{!0, !1, !2}
!0 = !{i32 1, !"amdgpu_code_object_version", i32 500}

We were using --amdhsa-code-object-version=5 to run LIT tests. Do you mean this flag will no longer take effect if we
switch to module flag for code object version?

Is anything else needed for this patch or any additional review comments?

This is an interesting bug. The patch does fix a flaw with the current Phi generation by the pipeliner. I think the approach here is a good one. It is a little difficult to understand the reason needed for the two different VRMaps though (much of the code in generatePhis is difficult to understand). But, the two maps do reduce some of the complexity in the code, which is good. I'll accept it once the test case comment is resolved. Thanks for fix the issue!

A couple more tests are needed. A test for for the image intrinsic. Also an assembler test for the v_illegal encoding.

In D123231#3611240, @ruiling wrote:

The issue @critson reported in the related change reminds to take a careful look of this change. I think there is one critical correctness issue need to be fixed. When we are moving the exit block into the loop, we need to make sure there is no convergent operation in the exit block, otherwise we may change the threads that will participate the convergent operation if the threads exit the loop non-uniformly. So we have to scan all the instructions in the exit block before making the reorder decision.

Hi @critson. Thanks for reducing the test case. I don't see an error with the test case though, so I may be missing something. Or, perhaps I need to see the complete test case? Here's my understanding. With the patch, control flow still converges at TrueExit. It's just that the Exit3 and Exit4 blocks have moved in the CFG so that they appear prior to loop.exit.guard rather than after. Following the exit paths from F, G, and C. I think that should be ok?

Hi @critson, thanks for pointing out the issue, and a potential fix. I'm looking at it now.

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.

Updates to the comments in the improve-order.ll test, based up reviewer suggestions.

Thanks for making this change. It's an interesting case. We handled a related issue, with subregisters, by adding code to preprocessPhiNodes to create copies. I don't have a problem with approach in this patch. There may be other cases in this code that need a similar change? Not sure...

The rationale is for library code that looks like:
void example(float *p, float v) {

if (_ISA_verison == 9008 || __ISA_verison == 9010)
   global_atomic_fadd(p, v);
else
   generic_atomic_fadd(p, v);

}

No changes with this patch. It's been a while since this patch was added. I was investigating an internal test failure, which turns out to be unrelated to this patch. So, I'd like to move forward with reviewing this again.

This looks good. Just a couple of minor comments.

Hi @sgundapa, just checking if you made a similar change that wasn't upstreamed?

In D123231#3450908, @sameerds wrote:

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?

Made several changes based upon suggestions and to fix a correctness issue:

• The new function, reorderNodes, must appear before collectInfos because collectInfos collects branch predicate information based upon the order of the nodes, so the wrong branch information was used after nodes were reordered.
• Changed name of the function, from improveNodeOrder to reorderNodes because changing the order is an improvement under certain conditions.
• Only reorder the nodes for large regions. The size of the region can be changed with a command-line option. The issues fixed by this patch are seen only with very large regions, so this flag limits the impact of this patch. I'm open to other ways to do this.
• Since reorderNodes appears before collectInfos, it no longer can use Visited. Instead, reorderNodes keeps track of the basic blocks in Order and uses that as part of the criteria to reorder. We only want to reorder a node that has a predecessor in Order.

Messed up the last revision by including another patch. This hopefully fixes it. The only new change is an additional test case, three_loops, added to test/Transforms/UnifyLoopExits/reduce_guards.ll

Added a test to show the case when we need to move the guard block predecessor from an outer loop to an inner loop. Three nested loops are needed for this. Initially, the guard block predecessor is added to the outer loop, but then need to change to the middle loop.

Changes based upon review comments.

It's great to see extensions to the pipeliner so that it works on more targets and loops that don't assume a hardware loop. A little while back a couple of functions, see shoulgnoreForPipelining, were added to support regular loops. However, I don't see that function used. Perhaps that effort was never completely finished? Do we need to get rid of that function, or try to combine the work you've done with that work?

Changes based upon review. Don't just skip over insertelements with a
non-constant index. Instead, fall through to include additional cost
calculation.

There are some clang-format issues to be addressed before committing. Other than that, the patch LGTM. Thanks!

Looks good to me.

Looks good to me. Thanks!

Changes so that the VGPR to AGPR spill slot can be removed if stack slot coloring has not allocated multiple objects to the same slot. Added a couple extra test cases to check for different cases when the slot can/can't be removed.

In D115996#3203344, @rampitec wrote:

In D115996#3203263, @bcahoon wrote:

In D115996#3203119, @rampitec wrote:

Will the slot remain in the final allocation?

Yes. At least in the example I'm seeing.

Is there any way to get rid of them? Check they are actually dead, or move code later?

In D115996#3203119, @rampitec wrote:

Will the slot remain in the final allocation?

In D109762#3059988, @aeubanks wrote:

I talked to some people and we've decided that the best thing to do would be to refactor out the nontrivial unswitching part into a function pass. Nontrivial unswitching is fairly special in the kinds of transforms it does.

Add CHECKs for the alias scope metadata. Removed unintentional ws.

In D109762#2999963, @aeubanks wrote:

you should probably check that this doesn't affect compile times too much since we're creating some new (potentially expensive) analyses without any sort of caching

Added amdgpu-registered-target to test case.

Moved checks for pointer type in ScaledReg and BaseRegs to outside the loop.

In D100149#2816330, @foad wrote:

LGTM for AMDGPU, and I guess you haven't changed the behaviour for AArch64. What about other GlobalISel targets like MIPS and X86? Will they suddenly start seeing G_SBFX/G_UBSX where they weren't expecting them? Anyway I've added a couple more globalisel reviewers.

Rebased

In D104245#2818751, @foad wrote:

Thanks for doing this. I think it's important that we reach agreement on these details.

I would suggest these rules (1):

G_SBFX, G_UBFX: 0 <= lsb < lsb + width <= bitwidth (all values unsigned)

This implies that width is not allowed to be 0.

Improved description of legal values, based upon review comments.

Addressed review comments. Fixed doxygen comment, and a check for width <= 32. Also, when converting the G_SBFX/G_UBFX to a shift sequence for the sign extend, clarify that the expectation is that the width range is between 1 and 64-Offset.