- User Since
- Dec 5 2012, 4:53 PM (333 w, 1 h)
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
Mon, Apr 22
Reduce test a bit
Thu, Apr 18
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
I am hitting this assert in LuxMark with this patch:
Assertion failed: (IncomingDef->isPHI()), function lowerPhis, file ../lib/Target/AMDGPU/SILowerI1Copies.cpp, line 534.
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
- Code generation
- Running pass 'CallGraph Pass Manager' on module 'AMD_9348_7/t_9348_21.bc'.
- 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
Wed, Apr 17
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
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.
This is still requiring the TTI as a dependency, so I don't understand what this solves?
Tue, Apr 16
Mon, Apr 15
The test should go with the rest in test/Analysis/DivergenceAnalysis
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?
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?
Sun, Apr 14
Sat, Apr 13
LGTM. We should probably start matching these though
Fri, Apr 12
I don't understand exactly what the layering issue you're fixing is. Can you elaborate in the commit message?
Thu, Apr 11
Wed, Apr 10
Can you reduce the test further?
Tue, Apr 9
You should use computeRegisterLiveness instead of adding a more naive search for a def, which may not exist.
Sat, Apr 6
LGTM, although if -O0 works that would be preferable to introducing a new pass control flag