There's a lot of clutter that makes this hard to review. All of the extra debug, special ifdef blocks, and commented out code should be removed.

Also needs a comment explaining why LCSSA is needed

This should really be expressed as a pass dependency, not explicitly adding the pass to the pipeline

r358892

In D59772#1473800, @hakzsam wrote:

Hi Matt,

This change introduces a regression with Overwatch and RADV (cf. https://bugs.freedesktop.org/show_bug.cgi?id=110476).

Here's the attached LLVM IR https://pastebin.com/raw/VqRF4unW

Can you have a look?
Thanks!

LGTM

Reduce test a bit

In D60858#1472184, @echristo wrote:

Looks ok to me. Are you worried about the correctness of the code? That seems to be a lot of matching?

ping

The AMDGPU usage could be changed to use this instead of TTI. There’s no real difference

I could do a MIR test that only runs the asm printer, which will be less useful whenever the long branch expansion is fixed

In D60858#1471490, @echristo wrote:

FWIW they're fine IMO too :)

Testcase and match seem a little long - any chance of shrinking either?

In D60858#1471444, @Orlando wrote:

Are the function attributes in the test necessary? Watching other reviews leads me to believe these are undesirable. Apart from this it LGTMy untrained eyes (I'm new to reviewing!).

In D60834#1471277, @nhaehnle wrote:

I think there are some misunderstandings here. None of the IR passes require LCSSA. The problem is in getting the divergence data into the SelectionDAG.

Specifically, you can have, in a weird mixture of IR and SelectionDAG:

loop:
  ...
  %uni = ...                             ; Value is uniform here
  ...
  br i1 %div, label %loop, label %next   ; Divergent loop exit

next:
  %0 = CopyFromReg N(corresponding to %uni)
  use %0

In this case, %0 must be labeled divergent. However, %0 does not exist at an IR level, and so the code in isSDNodeSourceOfDivergence can only query for the divergence of %uni. However, %uni itself is uniform.

One way to look at the problem is that DivergenceAnalysis::isDivergent is really "isDivergentAtDefinition", and what we need is a query "isDivergentAtUse". Implementing that query isn't entirely trivial, and LCSSA is effectively an alternative way of making the right query.

I am hitting this assert in LuxMark with this patch:
Assertion failed: (IncomingDef->isPHI()), function lowerPhis, file ../lib/Target/AMDGPU/SILowerI1Copies.cpp, line 534.
Stack dump:
0. Program arguments: /Users/matt/src/llvm/build_debug/bin/clang -cc1 -triple amdgcn-amd-amdhsa -emit-obj -disable-free -main-file-name t_9348_21.bc -mrelocation-model pic -pic-level 1 -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -fvisibility hidden -fapply-global-visibility-to-externs -target-cpu gfx900 -target-feature -wavefrontsize16 -target-feature -wavefrontsize32 -target-feature +wavefrontsize64 -target-feature -sram-ecc -target-feature -code-object-v3 -target-feature +cumode -dwarf-column-info -debugger-tuning=gdb -resource-dir /home/marsenau/builds/opencl_amdgpu_scratch/bin/lib/clang/8.0 -O3 -fdebug-compilation-dir /home/marsenau/src/LuxMark-3.1 -ferror-limit 19 -fmessage-length 201 -cl-kernel-arg-info -fobjc-runtime=gcc -fdiagnostics-show-option -vectorize-loops -vectorize-slp -mllvm -amdgpu-internalize-symbols -mllvm -amdgpu-early-inline-all -o /tmp/t_9348_21-9f1436.o -x ir AMD_9348_7/t_9348_21.bc -faddrsig

1. Code generation
2. Running pass 'CallGraph Pass Manager' on module 'AMD_9348_7/t_9348_21.bc'.
3. Running pass 'SI Lower i1 Copies' on function '@scheduler'

0 clang 0x0000000109a1f8bc llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1 clang 0x0000000109a1fe79 PrintStackTraceSignalHandler(void*) + 25
2 clang 0x0000000109a1db36 llvm::sys::RunSignalHandlers() + 118
3 clang 0x0000000109a23a42 SignalHandler(int) + 210
4 libsystem_platform.dylib 0x00007fff7ddd1b5d _sigtramp + 29
5 libsystem_platform.dylib 0x000000012a983938 _sigtramp + 2897944056
6 libsystem_c.dylib 0x00007fff7dc916a6 abort + 127
7 libsystem_c.dylib 0x00007fff7dc5a20d basename_r + 0
8 clang 0x0000000106af4af5 (anonymous namespace)::SILowerI1Copies::lowerPhis() + 1141
9 clang 0x0000000106af40ba (anonymous namespace)::SILowerI1Copies::runOnMachineFunction(llvm::MachineFunction&) + 186
10 clang 0x000000010860f8de llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 542
11 clang 0x0000000108bfab35 llvm::FPPassManager::runOnFunction(llvm::Function&) + 613
12 clang 0x0000000107f5c8ad (anonymous namespace)::CGPassManager::RunPassOnSCC(llvm::Pass*, llvm::CallGraphSCC&, llvm::CallGraph&, bool&, bool&) + 925
13 clang 0x0000000107f596ed (anonymous namespace)::CGPassManager::RunAllPassesOnSCC(llvm::CallGraphSCC&, llvm::CallGraph&, bool&) + 541
14 clang 0x0000000107f58ec1 (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) + 433
15 clang 0x0000000108bfb8d5 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) + 789

In D60834#1470916, @rtaylor wrote:

In D60834#1470764, @arsenm wrote:

In D60834#1470763, @arsenm wrote:

It sort of intuitively makes sense to me that the control flow lowering would like LCSSA. However, this should not be handled by adding it directly to the pass pipeline. You can add this as a dependency, e.g. AU.addRequiredID(LCSSAID);

I would also like to see the an IR->IR testcase showing LCSSA was implicitly run

Actually, what really requires LCSSA? Is it DivergenceAnalysis or StructurizeCFG directly?

How I understand the problem is that DA is not looking across blocks and therefore won't see that tmp62 is actually divergent in loop exit (though it is uniform in the loop). LCSSA provides a phi node for the loop exit block (where it is divergent) and allows DA to mark it divergent so that the s_buffer_load can be lowered to a buffer_load.

LGTM

In D60834#1470763, @arsenm wrote:

It sort of intuitively makes sense to me that the control flow lowering would like LCSSA. However, this should not be handled by adding it directly to the pass pipeline. You can add this as a dependency, e.g. AU.addRequiredID(LCSSAID);

I would also like to see the an IR->IR testcase showing LCSSA was implicitly run

It sort of intuitively makes sense to me that the control flow lowering would like LCSSA. However, this should not be handled by adding it directly to the pass pipeline. You can add this as a dependency, e.g. AU.addRequiredID(LCSSAID);

This is a workaround. The structurizer / annotator must be correct without relying on another pass to hide situations they don't handle correctly

LGTM

LGTM

LGTM

If the goal is to have a semantically always dereferencable stack pointer, I think we need to create a new addrspace. It would then be a no-op addrspacecast from an alloca, which the frontend desiring safe stack access would be responsible for inserting. We would then need to track the current global stack size in the ABI somewhere, and selection would need to insert this kind of bounds check code based on that

I don't understand the problem being solved here. Who/what is this intended to benefit? An out of bounds access is going to be undefined. I don't want to start trying to define in at an arbitrary point in the backend. Crashing on the invalid access is much easier problem to debug. This seems to be a partial replacement for asan? If this is intended as some user visible semantic fix, that requires more thought and probably needs to be a new address space.

LGTM

LGTM

This is still requiring the TTI as a dependency, so I don't understand what this solves?

LGTM

In D59971#1467546, @aemerson wrote:

Reviving this as the overall approach was fine, it seems the alignment of non pow2 types is assumed to be the alignment of the next largest pow-2 type, so we don't need to worry about alignment during the breakdown.

I did however change the legalization method to not use extracts/inserts, but instead use extending loads and truncating stores, so that the artifacts get combined away and it Just Works.

The test should go with the rest in test/Analysis/DivergenceAnalysis

LGTM

LGTM

ping

This still needs tests, but it seems this duplicate pass thing could use some work. Both passes have essentially the same switch to turn the atomic into the equivalent non-atomic. It seems to be me like AtomicExpandPass is a superset of LowerAtomic, except AtomicExpand seems to ignore fences

Do we really still need DumpCode? I've long wanted to remove it. Is there any real obstacle remaining to users switching to some combination of the assembler and disassembler?

In D60633#1466195, @tpr wrote:

I have cut down the test a bit more and put it into fneg-combines.ll. I did not manage to repro any problems with the other cases that I added fixes for.

I don't have a massive amount of time to spend on this. Would you prefer I land it as is, without tests for the other cases, or remove the precautionary fix for those cases and wait for someone else to possibly encounter them?

Apparently we have both lib/CodeGEn/AtomicExpandPass.cpp, and lib/Transforms/Scalar/LowerAtomic.cpp. It's unclear to me what the difference is, or why both exist

Needs tests. Also I'm about 90% sure I added this already?

Rebase

Fix UnableToLegalize

LGTM. We should probably start matching these though

I don't understand exactly what the layering issue you're fixing is. Can you elaborate in the commit message?

r358111

r358110

r358109

r358105

Can you reduce the test further?

You should use computeRegisterLiveness instead of adding a more naive search for a def, which may not exist.

LGTM, although if -O0 works that would be preferable to introducing a new pass control flag

In D60339#1456892, @aemerson wrote:

Is there a reasonably sized test?