Address review comments

Addressed review comments: Use ranges in for loop and split generateWaitcntInstBefore to reuse its second half for generateWaitcntBlockEnd.
Also changed applyPreexistingWaitcnt to take a MachineBasicBlock::instr_iterator instead of a MachineBasicBlock::iterator to match the caller.

Addressed review comments and fixed debug build.

Add support for nested loops, and fixes the case where we have no terminator but preexisting s_waitcnt instructions. I still need to address @foad comments.

@piotr I ran compile time testing and the patch has no significant impact. Worst case is 1.1% and is the only one above 1%. Average is below 0.1%.

Gentle ping @foad @nhaehnle

Rebased and addressed @foad comment.

@foad I updated the patch. It is a lot simpler than the previous, and it fixes both the GFX9 and GFX10 cases. But it may still have a significant impact on compile time, I cannot think of another way to do that without visiting all the instructions from the loops :(

I agree that the code is a lot more complicated with this patch than it was with the previous patch. I think this improvement cannot be implemented simply by looking at the WaitcntBrackets without requiring all this refactoring.
So this means we have two choices:

1. Original patch: Before inserting the waitcnts, visit all the loop instructions a single time (no fixed-point) until we visit all the instructions, or until we find an instruction that invalidates the optimization. Depending on what we found, flush in preheaders or not before generating the waitcnts. This is a lot simpler.
2. New patch with refactoring: No need to visit the instructions before inserting the waitcnt, but we need to compute two brackets for each block, and keep two waitcnt lists until we decide which one we want to generate. This is a lot more complicated.

The original motivation to work on 2. was concerns about compile-time impact of 1., but because we need to compute two brackets for each block, I actually don't think that 2. is more efficient. Both the GFX9 and GFX10 improvements can be implemented with either 1. or 2. So for the sake of simplicity, I think I should revert back to the original patch.

Refactoring of the pass. We compute the brackets for both the flushed and non-flushed versions of each outer loop until we finish visiting the loop or until we decide it is not worth to flush in the preheader. Instead of generating the waitcnts, we add them to two separate lists (for the flushed and non-flushed versions). After all the blocks are visited, we generate one of the two lists depending on the decision we made.

@arsenm the idea is to rotate the processing loop (runOnMachineFunction) of the SIInsertWaitcnt pass, so we can make decisions from predecessors, not the actual machine IR loop containing the waitcnt:

In D115747#3194595, @foad wrote:

Note that the loop over basic blocks in runOnMachineFunction currently looks like:

for each block:
  get saved state for this block
  process block
  merge new state into saved state for each successor

But if necessary we could change it to:

for each block:
  merge saved state from each predecessor
  process block
  save state for this block

That makes sense, thanks for the comments. So maybe I can start by trying to implement the idea of rotating the processing loop and making a decision from the predecessors and see if that solution would be enough to fix the most common cases we observed. Then, if control flow inside loops is really an issue with that solution, I can try the other solution based on GPR liveness, but I also don't like the idea of having another fixed-point loop that could significantly degrade compilation time.

Revert changes improving the pass implementation, these changes will be addressed in another patch.

In D101939#2741529, @foad wrote:

In D101939#2740008, @arsenm wrote:

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

On small functions I think the run time of this pass is dominated by the two calls to TRI->getAllocatableSet, which means two identical calls to SIRegisterInfo::getReservedRegs, which has a very large fixed cost. Is there some way the Reserved register set can be cached (in the SIMachineFunctionInfo object?)?

Add the TargetRegisterInfo::getAllocatableSets method to avoid calling getReservedRegs multiple times when we want allocatable sets for multiple register classes.

In D101939#2740008, @arsenm wrote:

Can we address it being slow instead of just turning of everything that is? There's no reason it should be slow, but the way it uses liveness information is horrible

In D101414#2738462, @foad wrote:

In D101414#2738443, @critson wrote:

llc-pipeline.ll has problems with -DLLVM_ENABLE_EXPENSIVE_CHECKS=1 as instruction verification seems to still take place.

test/CodeGen/X86/opt-pipeline.ll uses grep -v to filter these lines out.

In D101414#2737354, @thakis wrote:

This breaks incremental builders: http://45.33.8.238/linux/45830/step_12.txt

Ptal. You'll need a -o /dev/null and an rm -f for the temp file to clean up bots (can remove the latter after a bit)

Enable the passes only from -O2 or when explicitly enabled with flags. Also update the test case to ensure the passes are run for -O1 when explicitly enabled.

Remove checks for the "Pass Arguments:" lines in the test case.

In D101414#2721285, @arsenm wrote:

Needs a testcase. I think we have some pass pipeline tests already to show what's run (maybe not for -O1, I didn't know anyone actually used it)

Add test case

Update patch so it applies upstream.

Specialize DenseMapInfo directly with the PPC TOC key type instead of VariantKind.

Generate uninitialized external TLS data with the .csect directive instead of .comm

I forgot to link the commit with this differential, but the patch has been committed: https://github.com/llvm/llvm-zorg/commit/45c4f238dc6fd9855b8578aa3ca3b8db336efb7e

Add support for variables with internal linkage.

Rebase and reduce PPC test case.

LGTM, I only have a minor comment.

Thanks for addressing the comments, LGTM now.

LGTM

Gentle ping :)

Use isISA3_1 instead of hasP10Vector and add P9 run line in test case.

LGTM

Thanks a lot for working on that Amy ! I have some comments on the patch.

Remove unrelated NFC change, simplify lane mask computation and MIR code

Rebase and fix comment

Good catch, thanks for fixing that.

LGTM, just a minor comment regarding the multiclass.

Update patch so it applies to ToT

Fix comments

Why do we need these flags for PLXVP and PSTXVP ? Please, add a test case showing why this is required.

LGTM, thanks.

Update comments