This is an archive of the discontinued LLVM Phabricator instance.

Fix for bug 34002
AcceptedPublic

Authored by maya.madhavan on Sep 18 2018, 9:36 PM.

Details

Summary

When a label is encountered while an open 'it' block is under construction (under construction because more conditional instructions can be added to it), the gathered block should be emitted before the label is emitted.
Due to the location from where the onLabelParsed function was being called, the label is emitted first and the 'it' block flushed after, resulting in incorrect code.

Diff Detail

Event Timeline

maya.madhavan created this revision.Sep 18 2018, 9:36 PM
maya.madhavan added a comment.EditedSep 18 2018, 9:39 PM

Verification

  1. Rebuilt with changes.
  2. Output of test noted in bug:
.text
.p2align        1
.code   16
.globl  f1
.globl  f2

f1:

.thumb_func
cmp     r0, #10



it      pl
movpl   r0, #0

f2:

.thumb_func
movs    r1, #0

.Ltmp:

cmp     r0, #0
ittt    pl
addpl   r1, r1, r0
subpl   r0, r0, #1
bpl     .Ltmp
mov     r0, r1
bx      lr

The label 'f2' is in the expected location now.

  1. make check (in progress)
olista01 added inline comments.Sep 19 2018, 1:34 AM
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
9451–9454

Can this call be removed now? doBeforeLabelEmit and onLabelParsed are only ever called in pairs before and after emitting the label, so there should never be anything to flush here, and as you've pointed out doing it here is incorrect.

test/MC/ARM/implicit-it-generation.s
70

I think this test already exposes the bug, we just need to add CHECK lines to make sure the labels are emitted in the correct place. That would be clearer than adding a second test to check the label locations.

Updated test case and call per Oliver Stannard's suggestion.

maya.madhavan marked 2 inline comments as done.Sep 19 2018, 8:36 AM
maya.madhavan added inline comments.
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
9451–9454

Fixed, thanks!

test/MC/ARM/implicit-it-generation.s
70

Updated - thanks!

olista01 accepted this revision.Sep 19 2018, 8:39 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sep 19 2018, 8:39 AM