Select8/Select16 expansion does not preserve existing basic block fallthrough #123
Labels
A-llvm
Affects the LLVM AVR backend
bug
has-llvm-commit
This issue should be fixed in upstream LLVM
has-reduced-testcase
A small LLVM IR file exists that demonstrates the problem
Comments
|
p.s. I forked and added a test that captures it: https://github.com/carlos4242/llvm couldn't figure out PR |
|
Before I lost my laptop, I found the step that was causing the broken reordering. The pass in question was "Control Flow Optimizer". After that pass, the code is in the wrong order, with the last block falling through into the next function. Unfortunately I lost the debug output with the laptop but the instruction order is definitely fine before this step and definitely broken after this step. And it stays broken all the way down into machine code. What I can't quite understand is how this pass can cause broken ordering because it does not seem to have target specific code? |
|
Putting more colour in this, if I compile using the same build of llc but for target x86 then it works fine. So it's pretty definitely an AVR thing. :-/ Look at the code in x86 code is ordered sensibly after branch folding... But AVR code is miscompiled... |
|
@brainlag, @TimNN, @dylanmckay can anyone give any ideas how to track this down? I've done a full debug/pass log on this bug: https://gist.github.com/carlos4242/bb47ee95ba4b3638a58be92316c80c88 I'm struggling to debug this because I'm not sure what pass is at fault. Earlier I thought it was the "Control Flow Optimiser" but I'm not so sure now. For a start, I couldn't figure out why this issue is only affecting AVR (I did an output to X86_64 and it's fine). I think that pass is rearranging the machine basic blocks but the issue is that one of the blocks should have a branch at the end to jump to the function epilog. Looking about 1/3 of the way down, I can see this... ...comparing the basic blocks, e.g. From a naive point of view, I'd say that either legalisation or instruction selection for Anyone else have any ideas? |
|
Confirmed on llvm branch avr-rust-2018-12-18 |
|
Update for the benefit of @DoubleHyhpen_gitlab... the original Swift code that this LLVM came from... The overall program runs a mains dimmer switch for a light in my front room! It's I2C controlled by my raspberry pi, which is running homebridge.js. The function that is miscompiling is |
|
This happens at |
|
I've written a script that takes an LLVM IR test file and generates 89 different permutations, each disabling a different pass in the LLVM pipeline. I've ran the test with each of these 89 passes disabled, and the issue still occurs with each of them disabled. Here is the script. #! /usr/bin/env ruby
require 'fileutils'
file_path = ARGV[0] or raise "no LL test file given on command line"
file_contents = File.read(file_path)
possible_passes_raw = <<END
-disable-2addr-hack
-disable-a15-sd-optimization
-disable-adv-copy-opt
-disable-basicaa
-disable-block-placement
-disable-bpf-peephole
-disable-branch-fold
-disable-cgp
-disable-cgp-branch-opts
-disable-cgp-ext-ld-promotion
-disable-cgp-gc-opts
-disable-cgp-select2branch
-disable-cgp-store-extract
-disable-cleanups
-disable-complex-addr-modes
-disable-const64
-disable-constant-hoisting
-disable-copyprop
-disable-debug-info-print
-disable-demotion
-disable-dfa-sched
-disable-early-ifcvt
-disable-early-taildup
-disable-gisel-legality-check
-disable-hcp
-disable-hsdr
-disable-icp
-disable-ifcvt-diamond
-disable-ifcvt-forked-diamond
-disable-ifcvt-simple
-disable-ifcvt-simple-false
-disable-ifcvt-triangle
-disable-ifcvt-triangle-false
-disable-ifcvt-triangle-false-rev
-disable-ifcvt-triangle-rev
-disable-inlined-alloca-merging
-disable-interleaved-load-combine
-disable-lanai-mem-alu-combiner
-disable-lftr
-disable-libcalls-shrinkwrap
-disable-licm-promotion
-disable-lsr
-disable-machine-cse
-disable-machine-dce
-disable-machine-licm
-disable-machine-sink
-disable-memcpy-idiom
-disable-memmove-idiom
-disable-memop-opt
-disable-merge-into-combines
-disable-mergeicmps
-disable-mr-partial-inlining
-disable-non-allocatable-phys-copy-opt
-disable-nounwind-inference
-disable-ondemand-mds-loading
-disable-packetizer
-disable-partial-inlining
-disable-partial-libcall-inlining
-disable-peephole
-disable-phi-elim-edge-splitting
-disable-post-ra
-disable-postra-machine-licm
-disable-postra-machine-sink
-disable-preheader-prot
-disable-preinline
-disable-promote-alloca-to-lds
-disable-promote-alloca-to-vector
-disable-sched-critical-path
-disable-sched-cycles
-disable-sched-hazard
-disable-sched-height
-disable-sched-live-uses
-disable-sched-physreg-join
-disable-sched-reg-pressure
-disable-sched-stalls
-disable-sched-vrcycle
-disable-separate-const-offset-from-gep
-disable-shifter-op
-disable-simplify-libcalls
-disable-spill-fusing
-disable-spill-hoist
-disable-ssc
-disable-store-widen
-disable-symbolication
-disable-tail-calls
-disable-tail-duplicate
-disable-vecdbl-nv-stores
-disable-verify
-disable-vp
END
possible_passes = possible_passes_raw.lines.map(&:chomp)
new_dir_name = "#{File.basename(file_path)}-without-passes"
new_dir = File.join(File.dirname(file_path), new_dir_name)
FileUtils.mkdir_p(new_dir)
possible_passes.each do |possible_pass|
pass_name = possible_pass.gsub("-disable-", "")
test_without_pass = file_contents.gsub("$DISABLE_PASSES", possible_pass)
if test_without_pass == file_contents
raise "file does not contain a '$DISABLE_PASSES' for substitution"
end
test_path = File.join(new_dir, "disable-#{pass_name}.ll")
File.write(test_path, test_without_pass)
endThe bug is not caused by any of the passes mentioned. |
|
Here is the output with control flow optimiztion disabled (under the branch folding pass) _TF4main9i2cUpdateFT8registerVs5UInt85valueS0__T_: ; @_TF4main9i2cUpdateFT8registerVs5UInt85valueS0__T_
; %bb.0: ; %entry
push r17
cpi r24, 7
brne LBB0_1
rjmp LBB0_7
LBB0_1: ; %entry
cpi r24, 6
breq LBB0_2
rjmp LBB0_10
LBB0_2:
ldi r24, 5
cpi r22, 5
brlo LBB0_4
; %bb.3:
mov r24, r22
LBB0_4:
ldi r17, 90
cpi r22, 91
brsh LBB0_6
; %bb.5:
mov r17, r24
LBB0_6:
sts _Tv4main11delayFactorVs5UInt8, r17
mov r22, r17
clr r23
ldi r18, 100
ldi r19, 0
ldi r20, 0
ldi r21, 0
movw r24, r20
call __mulsi3
sts _Tv4main7delayUsVs6UInt32+3, r25
sts _Tv4main7delayUsVs6UInt32+2, r24
sts _Tv4main7delayUsVs6UInt32+1, r23
sts _Tv4main7delayUsVs6UInt32, r22
ldi r24, 34
ldi r25, 0
mov r22, r17
call _TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_
pop r17
ret
LBB0_7:
ldi r18, 1
cpi r22, 0
brne LBB0_9
; %bb.8:
ldi r18, 0
LBB0_9:
mov r24, r18
andi r24, 1
sts _Tv4main7enabledSb, r24
ldi r24, 35
ldi r25, 0
mov r22, r18
call _TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_
pop r17
ret
LBB0_10:
pop r17
ret
|
|
It's definitely occurring in the branch folding pass. |
|
The incorrect fallthrough is being created somewhere within this block in I figured this out by progressively guarding all of the different optimization logic blocks in the pass with |
|
Here, the branch folding pass is trying to minimize the number of instructions (particularly, branch instructions) by reorganizing basic blocks so that basic blocks fallthrough to each other, rather than requiring explicitly branch instructions. In this particular case, LLVM notices that LLVM is deciding to move
bb.10 (%ir-block.9):
; predecessors: %bb.1, %bb.4
$r17 = POPRd implicit-def $sp, implicit $sp
RET
$13 = void
bb.4 (%ir-block.2):
; predecessors: %bb.3, %bb.5
successors: %bb.10(0x80000000); %bb.10(100.00%)
liveins: $r17
STSKRr @_Tv4main11delayFactorVs5UInt8, $r17 :: (store 1 into `i8* getelementptr inbounds (%Vs5UInt8, %Vs5UInt8* @_Tv4main11delayFactorVs5UInt8, i64 0, i32 0)`)
$r23r22 = ZEXT $r17, implicit-def dead $sreg
$r19r18 = LDIWRdK 100
$r21r20 = LDIWRdK 0
$r25r24 = COPY $r21r20
CALLk &__mulsi3, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r23r22, implicit $r25r24, implicit $r19r18, implicit $r21r20, implicit-def $sp, implicit-def $r23r22, implicit-def $r25r24
STSWKRr @_Tv4main7delayUsVs6UInt32 + 2, killed $r25r24 :: (store 2 into `i32* getelementptr inbounds (%Vs6UInt32, %Vs6UInt32* @_Tv4main7delayUsVs6UInt32, i64 0, i32 0)` + 2)
STSWKRr @_Tv4main7delayUsVs6UInt32, killed $r23r22 :: (store 2 into `i32* getelementptr inbounds (%Vs6UInt32, %Vs6UInt32* @_Tv4main7delayUsVs6UInt32, i64 0, i32 0)`, align 4)
$r25r24 = LDIWRdK 34
$r22 = COPY killed $r17
CALLk @_TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r25r24, implicit $r22, implicit-def $sp
RJMPk %bb.10
$14 = void |
|
I think the problem is here. This is an IR dump right at the start of the bb.8 (%ir-block.8):
; predecessors: %bb.7, %bb.9
successors: %bb.10(0x80000000); %bb.10(100.00%)
liveins: $r18
$r24 = COPY $r18
$r24 = ANDIRdK killed $r24(tied-def 0), 1, implicit-def dead $sreg
STSKRr @_Tv4main7enabledSb, killed $r24 :: (store 1 into `i1* getelementptr inbounds (%Sb, %Sb* @_Tv4main7enabledSb, i64 0, i32 0)`)
$r25r24 = LDIWRdK 35
$r22 = COPY killed $r18
CALLk @_TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r25r24, implicit $r22, implicit-def $sp
bb.9 (%ir-block.8):
; predecessors: %bb.7
successors: %bb.8(0x80000000); %bb.8(100.00%)
$r18 = LDIRdK 0
RJMPk %bb.8
bb.10 (%ir-block.9):
; predecessors: %bb.1, %bb.4, %bb.8
$r17 = POPRd implicit-def $sp, implicit $sp
RETLook at the block This seems invalid. bb.9 (%ir-block.8):
; predecessors: %bb.7
successors: %bb.8(0x80000000); %bb.8(100.00%)
$r18 = LDIRdK 0
RJMPk %bb.8
bb.10 (%ir-block.9):
; predecessors: %bb.1, %bb.4
$r17 = POPRd implicit-def $sp, implicit $sp
RET
bb.8 (%ir-block.8):
; predecessors: %bb.7, %bb.9
liveins: $r18
$r24 = COPY $r18
$r24 = ANDIRdK killed $r24(tied-def 0), 1, implicit-def dead $sreg
STSKRr @_Tv4main7enabledSb, killed $r24 :: (store 1 into `i1* getelementptr inbounds (%Sb, %Sb* @_Tv4main7enabledSb, i64 0, i32 0)`)
$r25r24 = LDIWRdK 35
$r22 = COPY killed $r18
CALLk @_TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $
r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r25r24, implicit $r22, implicit-def $sp
# End machine code for function _TF4main9i2cUpdateFT8registerVs5UInt85valueS0__T_. |
|
Earlier on in the pass pipeline, the call did branch to the terminating block via fallthrough. The predecessor and successor lists matched the underlying instructions and basic blocks. bb.2 (%ir-block.8):
; predecessors: %bb.0
successors: %bb.3(0x80000000); %bb.3(100.00%)
%2:ld8 = LDIRdK 0
%3:ld8 = LDIRdK 1
CPIRdK %1:ld8, 0, implicit-def $sreg
%4:ld8 = Select8 killed %3:ld8, killed %2:ld8, 1, implicit $sreg
%5:ld8 = ANDIRdK %4:ld8(tied-def 0), 1, implicit-def dead $sreg
STSKRr @_Tv4main7enabledSb, killed %5:ld8 :: (store 1 into `i1* getelementptr inbounds (%Sb, %Sb* @_Tv4main7enabledSb, i64 0, i32 0)
`)
ADJCALLSTACKDOWN 0, 0, implicit-def dead $sp, implicit-def dead $sreg, implicit $sp
%6:dldregs = LDIWRdK 35
$r25r24 = COPY %6:dldregs
$r22 = COPY %4:ld8
CALLk @_TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $
r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r25r24, implicit $r2
2, implicit-def $sp
ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit-def dead $sreg, implicit $sp
bb.3 (%ir-block.9):
; predecessors: %bb.4, %bb.2, %bb.1
RET |
|
IR dump before "Expand ISel Pseudo-instructions" pass. bb.2 (%ir-block.8):
; predecessors: %bb.0
successors: %bb.3(0x80000000); %bb.3(100.00%)
%2:ld8 = LDIRdK 0
%3:ld8 = LDIRdK 1
CPIRdK %1:ld8, 0, implicit-def $sreg
%4:ld8 = Select8 killed %3:ld8, killed %2:ld8, 1, implicit $sreg
%5:ld8 = ANDIRdK %4:ld8(tied-def 0), 1, implicit-def dead $sreg
STSKRr @_Tv4main7enabledSb, killed %5:ld8 :: (store 1 into `i1* getelementptr inbounds (%Sb, %Sb* @_Tv4main7enabledSb, i64 0, i32 0)
`)
ADJCALLSTACKDOWN 0, 0, implicit-def dead $sp, implicit-def dead $sreg, implicit $sp
%6:dldregs = LDIWRdK 35
$r25r24 = COPY %6:dldregs
$r22 = COPY %4:ld8
CALLk @_TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $
r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r25r24, implicit $r2
2, implicit-def $sp
ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit-def dead $sreg, implicit $sp
bb.3 (%ir-block.9):
; predecessors: %bb.4, %bb.2, %bb.1
RETLooks legit, from the store to Then after the Expand ISel pseudos pass bb.9 (%ir-block.8):
; predecessors: %bb.2, %bb.10
successors: %bb.3(0x80000000); %bb.3(100.00%)
%4:ld8 = PHI %3:ld8, %bb.2, %2:ld8, %bb.10
%5:ld8 = ANDIRdK %4:ld8(tied-def 0), 1, implicit-def dead $sreg
STSKRr @_Tv4main7enabledSb, killed %5:ld8 :: (store 1 into `i1* getelementptr inbounds (%Sb, %Sb* @_Tv4main7enabledSb, i64 0, i32 0)
`)
ADJCALLSTACKDOWN 0, 0, implicit-def dead $sp, implicit-def dead $sreg, implicit $sp
%6:dldregs = LDIWRdK 35
$r25r24 = COPY %6:dldregs
$r22 = COPY %4:ld8
CALLk @_TF3AVR11writeEEPROMFT7addressVs6UInt165valueVs5UInt8_T_, <regmask $r2 $r3 $r4 $r5 $r6 $r7 $r8 $r9 $r10 $r11 $r12 $r13 $r14 $
r15 $r16 $r17 $r28 $r29 $r3r2 $r5r4 $r7r6 $r9r8 $r11r10 $r13r12 $r15r14 $r17r16 $r29r28>, implicit $sp, implicit $r25r24, implicit $r2
2, implicit-def $sp
ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit-def dead $sreg, implicit $sp
bb.10 (%ir-block.8):
; predecessors: %bb.2
successors: %bb.9(0x80000000); %bb.9(100.00%)
RJMPk %bb.9
bb.3 (%ir-block.9):
; predecessors: %bb.4, %bb.7, %bb.9
RETAt this point, This bug is likely caused by the Expand ISel pseudo instructions pass, which calls into AVR-specific expansion logic, corrupting the predecessor and successor lists, making them no longer reflect the realized control flow. |
|
Bug is probably in The branch folding pass is just operating on invalid assumptions. |
|
The only three instructions expanded by this function are:
|
|
I believe the expansion issue is occurring in the third one, |
|
Fix incoming. |
dylanmckay
added a commit
to dylanmckay/llvm
that referenced
this issue
Jan 20, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123.
|
Here's a fix, try it out @carlos4242, let me know if it fixes the bug and I'll upstream it. From 30136884e213e8641eb1da332de462399fa6e920 Mon Sep 17 00:00:00 2001
From: Dylan McKay <me@dylanmckay.io>
Date: Sun, 20 Jan 2019 23:20:48 +1300
Subject: [PATCH] [AVR] Insert unconditional branch when inserting MBBs between
blocks with fallthrough
This updates the AVR Select8/Select16 expansion code so that, when
inserting the two basic blocks for true and false conditions, any
existing fallthrough on the previous block is preserved.
Prior to this patch, if the block before the Select pseudo fell through
to the subsequent block, two new basic blocks would be inserted at the
prior fallthrough point, changing the fallthrough destination.
The predecessor or successor lists were not updated, causing the
BranchFolding pass at -O1 and above the rearrange basic blocks, causing
an infinite loop. Not to mention the unconditional fallthrough to the
true block is incorrect in of itself.
This patch modifies the Select8/16 expansion so that, if inserting true
and false basic blocks at a fallthrough point, the implicit branch is
preserved by means of an explicit, unconditional branch to the previous
fallthrough destination.
Thanks to Carl Peto for reporting this bug.
This fixes avr-rust bug https://github.com/avr-rust/rust/issues/123.
---
lib/Target/AVR/AVRISelLowering.cpp | 9 ++++
test/CodeGen/AVR/avr-rust-issue-123.ll | 65 ++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
create mode 100644 test/CodeGen/AVR/avr-rust-issue-123.ll
diff --git a/lib/Target/AVR/AVRISelLowering.cpp b/lib/Target/AVR/AVRISelLowering.cpp
index 3116eb2a5032..e95a41472bff 100644
--- a/lib/Target/AVR/AVRISelLowering.cpp
+++ b/lib/Target/AVR/AVRISelLowering.cpp
@@ -1634,6 +1634,15 @@ AVRTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
MachineFunction *MF = MBB->getParent();
const BasicBlock *LLVM_BB = MBB->getBasicBlock();
+ MachineBasicBlock *FallThrough = MBB->getFallThrough();
+
+ // If the current basic block falls through to another basic block,
+ // we must insert an unconditional branch to the fallthrough destination
+ // if we are to insert basic blocks at the prior fallthrough point.
+ if (FallThrough != nullptr) {
+ BuildMI(MBB, dl, TII.get(AVR::RJMPk)).addMBB(FallThrough);
+ }
+
MachineBasicBlock *trueMBB = MF->CreateMachineBasicBlock(LLVM_BB);
MachineBasicBlock *falseMBB = MF->CreateMachineBasicBlock(LLVM_BB);
diff --git a/test/CodeGen/AVR/avr-rust-issue-123.ll b/test/CodeGen/AVR/avr-rust-issue-123.ll
new file mode 100644
index 000000000000..19e3f9a13fb3
--- /dev/null
+++ b/test/CodeGen/AVR/avr-rust-issue-123.ll
@@ -0,0 +1,65 @@
+; RUN: llc -O1 < %s -march=avr | FileCheck %s
+
+; This test ensures that the Select8/Select16 expansion
+; pass inserts an unconditional branch to the previous adjacent
+; basic block when inserting new basic blocks when the
+; prior block has a fallthrough.
+;
+; Before this bug was fixed, Select8/Select16 expansion
+; would leave a dangling fallthrough to an undefined block.
+;
+; The BranchFolding pass would later rearrange the basic
+; blocks based on predecessor/successor list assumptions
+; which were made incorrect due to the invalid Select
+; expansion.
+
+; More information in
+; https://github.com/avr-rust/rust/issues/123.
+
+%UInt8 = type <{ i8 }>
+%UInt32 = type <{ i32 }>
+%Sb = type <{ i1 }>
+
+@delayFactor = hidden global %UInt8 zeroinitializer, align 1
+@delay = hidden global %UInt32 zeroinitializer, align 4
+@flag = hidden global %Sb zeroinitializer, align 1
+
+declare void @eeprom_write(i16, i8)
+
+define hidden void @update_register(i8 %arg, i8 %arg1) {
+entry:
+ switch i8 %arg, label %bb7 [
+ i8 6, label %bb
+ i8 7, label %bb6
+ ]
+
+bb: ; preds = %entry
+ %tmp = icmp ugt i8 %arg1, 90
+ %tmp2 = icmp ult i8 %arg1, 5
+ %. = select i1 %tmp2, i8 5, i8 %arg1
+ %tmp3 = select i1 %tmp, i8 90, i8 %.
+ store i8 %tmp3, i8* getelementptr inbounds (%UInt8, %UInt8* @delayFactor, i64 0, i32 0), align 1
+ %tmp4 = zext i8 %tmp3 to i32
+ %tmp5 = mul nuw nsw i32 %tmp4, 100
+ store i32 %tmp5, i32* getelementptr inbounds (%UInt32, %UInt32* @delay, i64 0, i32 0), align 4
+ tail call void @eeprom_write(i16 34, i8 %tmp3)
+ br label %bb7
+
+bb6: ; preds = %entry
+ %not. = icmp ne i8 %arg1, 0
+ %.2 = zext i1 %not. to i8
+ store i1 %not., i1* getelementptr inbounds (%Sb, %Sb* @flag, i64 0, i32 0), align 1
+
+ ; CHECK: LBB0_{{[0-9]+}}:
+ ; CHECK: call eeprom_write
+ ; CHECK-NEXT: LBB0_{{[0-9]+}}
+ ; CHECK-NEXT: pop r{{[0-9]+}}
+ ; CHECK-NEXT: ret
+
+ tail call void @eeprom_write(i16 35, i8 %.2)
+ br label %bb7
+
+bb7: ; preds = %bb6, %bb, %entry
+ ret void
+}
+ |
dylanmckay
added
bug
A-llvm
dylanmckay
changed the title
|
This looks good to me. I've had a think through how the code works, and it looks correct. Plus I tested it on my original sample and it produces exactly the right code. Cool. :) Tested and approved as far as I'm concerned. |
|
oh, one caveat... the unit test isn't strong enough, I just tested it on the old compiler, pre-patch and it passed... I'll beef it up and send you a patch on just the unit test |
bogner
pushed a commit
to bogner/llvm-zipper-prototype
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351718 91177308-0d34-0410-b5e6-96231b3b80d8
llvm-git-migration
pushed a commit
to llvm/llvm-project
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. llvm-svn: 351718
|
Good catch, thanks! I committed and then immediately reverted it. I will recommit once the test is improved. |
earl
pushed a commit
to earl/llvm-mirror
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351718 91177308-0d34-0410-b5e6-96231b3b80d8
llvm-git-migration
pushed a commit
to llvm/llvm-project
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. llvm-svn: 351721
bogner
pushed a commit
to bogner/llvm-zipper-prototype
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351721 91177308-0d34-0410-b5e6-96231b3b80d8
|
I've taken the updated testcase from here and committed the patch upstream in r351721 (llvm/llvm-project@5c23410). Cherry-picked the upstream patch in cea79f7. |
earl
pushed a commit
to earl/llvm-mirror
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351721 91177308-0d34-0410-b5e6-96231b3b80d8
dylanmckay
pushed a commit
to dylanmckay/llvm
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351718 91177308-0d34-0410-b5e6-96231b3b80d8
dylanmckay
pushed a commit
to dylanmckay/llvm
that referenced
this issue
Jan 21, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351721 91177308-0d34-0410-b5e6-96231b3b80d8
|
Awesome. :) |
dylanmckay
pushed a commit
to avr-rust/llvm
that referenced
this issue
Jan 26, 2019
…with fallthrough This updates the AVR Select8/Select16 expansion code so that, when inserting the two basic blocks for true and false conditions, any existing fallthrough on the previous block is preserved. Prior to this patch, if the block before the Select pseudo fell through to the subsequent block, two new basic blocks would be inserted at the prior fallthrough point, changing the fallthrough destination. The predecessor or successor lists were not updated, causing the BranchFolding pass at -O1 and above the rearrange basic blocks, causing an infinite loop. Not to mention the unconditional fallthrough to the true block is incorrect in of itself. This patch modifies the Select8/16 expansion so that, if inserting true and false basic blocks at a fallthrough point, the implicit branch is preserved by means of an explicit, unconditional branch to the previous fallthrough destination. Thanks to Carl Peto for reporting this bug. This fixes avr-rust bug avr-rust/rust-legacy-fork#123. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@351721 91177308-0d34-0410-b5e6-96231b3b80d8
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm wondering if this is a regression on #83 or me using a wrong branch that doesn't have a fix in (i'm losing track) or a genuine new bug.
Anyway, I've built from this commit...
and I'm seeing out of order assembly produced from my IR when optimisations are turned on in llc. I started noticing weird behaviour on my AVR program and finally tracked it to broken MC.
This function in LLVM IR...
is producing this MC...
It looks like the BB
LBB4_7should be at the end.Any suggestions?
The text was updated successfully, but these errors were encountered: