T1 instructions not printed correctly inside IT blocks

[jmolloy]

Bugzilla Link	24340
Version	trunk
OS	All
Blocks	llvm/llvm-project#19939

Extended Description

Thumb1 instructions such as "rsbs" are written differently when inside IT blocks to reflect their change in semantics, according to the ARMARM.

When inside an IT block, these instructions don't set the flags. The ARMARM says (v7-M ARMARM, A7.7.117:

RSBS ,,#0 Outside IT block. RSB ,,#0 Inside IT block.

LLVM does not currently model this.

[llvmbot]

When I further run an incorrect .s file through llvm-mc, it detects the incorrect conditionals, so we're doing that part correctly during assembly:
llvm-mc it.s ... .code 16 _Z4testv: .thumb_func .fnstart ittee ne addne r0, r0, #​1 it.s:31:6: error: incorrect condition in IT block; got 'al', but expected 'ne' rsbs r1, r1, 2 ^ addeq r2, r2, #​3 it.s:33:5: error: incorrect condition in IT block; got 'al', but expected 'eq' mov r3, r3 ^

.code   16
bx  lr

...

In 32 bit mode we're printing the same errors but the ITTEE NE is never emitted, so that's also correct. I think the goal here should be to provide errors / advice at the cpp->s stage in addition to catching them while lowering to MC, since the diagnostics for inline asm will be positioned accurately there and it's an earlier shot at warning on the wrong syntax.

[llvmbot]

My first pass on this is that we should: 1) Preserve the IT instruction in assembly printing when we rewrite blocks, but not emit the actual instruction in ARM mode. This means disassembly won't know whether or not the instruction was originally entered by the user.

"The conditions specified in an IT instruction must match those specified in the syntax of the instructions in its IT block. When assembling to ARM code, assemblers check IT instruction syntax for validity but do not generate assembled instructions for them." From ARMv7-AR reference.

2) The IT block must be preserved in thumb mode since some instructions are not naturally conditional there, and we don't have a condition field for them in tablegen which makes both printing and verification must more difficult for the instructions of the type RSBS which have identically decoded forms which conditionally decode differently.

3) Although the IT block cannot normally set the "T" condition to AL, this is allowed if there is only one instruction in the block. This is presumably to allow overriding conditional instructions, but I can't track down the examples at the moment.

Anyway, that's what we're looking at here.

[llvmbot]

Well, a newer clang produced different results:

_Z4testv: .fnstart @APP ITTEE AL addal r0, r0, 1 rsbal r1, r1, 2 addhs r2, r2, 3 movhs r3, r3

.code   32
@NO_APP
bx  lr

Looking into how we handle this, IT instructions supposed to be generated in ARM; they're supposed to be verified against the condition codes in the IT instruction to make sure things match up, and they're definitely not right. AL is prohibited as unpredictable as an IT condition, and the HS that ended up on the second set of instructions is a good sign of that.

We appear to be getting this wrong both by not validating the conditions in the IT block for thumb mode and marking instructions with them, which is the original problem, and not validating conditions in its own implicitly created IT blocks in ARM mode. From what I can tell the current code is meant to create implicit IT blocks from conditionals if the right compiler flags are given and defaults to ARM for this, but doesn't actually do this because it's in a case which only occurs when an IT is encountered. ARM's recommendation was to show the instruction in assembly but not actually assemble it in ARM mode.

In thumb mode we also fail at verifying condition codes where possible and in applying the opposite "else" code in others.

To me it seems like the correct thing to do, assuming we don't assemble IT in ARM mode right now, is to both check for correct conditionals on instructions that may be conditional, and set them on unconditional instructions which we encounter in the thumb blocks, while the ARM blocks should just be verified against the IT for correctness of condition. Judging by the 4 command line flags related to this in ARMAsmParser.cpp I don't think there's a clear consensus on what we want to do here. I just know that what we are doing is verifiably wrong enough of the time that something should change. I'll do some thinking on it and look into how hard verifying a block properly will be.

[llvmbot]

Alright, I'll look into this further and hopefully have a patch later in the week.

[llvmbot]

Sure, feel free to reassign it to yourself. I haven't had a chance to look into this at all.

[llvmbot]

Rahul Chaudhry I can look at this if you don't have time. It's not major, but it doesn't match up well with the usual print.

[llvmbot]

Actually the triple is missing from the bitcode. Another bug or just me using a bad triple name?

[llvmbot]

I see the same issue. It happens with Thumb instructions which are required to set their predicate flag, verified with this test case:

I know the IT statement is optional, but I wanted to verify it was correct. void test() { __asm (

pragma thumb

    "ITTEE AL\n\t"
    "addal r0, r0, 1\n\t"
    "rsbal r1, r1, 2\n\t"
    "addhs r2, r2, 3\n\t"
    "movhs r3, r3\n\t"
);

}

clang itblock_test.cpp --target=thumbv7-pc-eabi -emit-llvm -c -o itblock.bc -fms-compatibility

Regular clang -S couldn't discover the triple (another bug?)

elgordo@ubuntu:~/Documents$ clang -S itblock.bc warning: overriding the module target triple with x86_64-pc-linux-gnu [-Woverride-module] 'cortex-a8' is not a recognized processor for this target (ignoring processor) '+dsp' is not a recognized feature for this target (ignoring feature) '+strict-align' is not a recognized feature for this target (ignoring feature) '+vfp3' is not a recognized feature for this target (ignoring feature) '-crypto' is not a recognized feature for this target (ignoring feature) '-neon' is not a recognized feature for this target (ignoring feature) 'cortex-a8' is not a recognized processor for this target (ignoring processor) 'cortex-a8' is not a recognized processor for this target (ignoring processor) '+dsp' is not a recognized feature for this target (ignoring feature) '+strict-align' is not a recognized feature for this target (ignoring feature) '+vfp3' is not a recognized feature for this target (ignoring feature) '-crypto' is not a recognized feature for this target (ignoring feature) '-neon' is not a recognized feature for this target (ignoring feature) 'cortex-a8' is not a recognized processor for this target (ignoring processor) 'cortex-a8' is not a recognized processor for this target (ignoring processor)

Finally, elgordo@ubuntu:~/Documents$ clang -S itblock.bc --target=thumbv7-linux-abi

and the following is output:

_Z4testv: @ @​_Z4testv .fnstart @ BB#0: @APP add r0, r0, #​1 rsb r1, r1, #​2 addhs r2, r2, #​3 movhs r3, r3

@NO_APP
bx  lr

.Lfunc_end0: .size _Z4testv, .Lfunc_end0-_Z4testv .cantunwind .fnend

.ident  "clang version 3.9.1-4ubuntu3~16.04.2 (tags/RELEASE_391/rc2)"
.section    ".note.GNU-stack","",%progbits

on another note, am I really running clang 3.9? Aren't we up to 5.0? Time to do a build. At least the same bugs are reproducing on my older version.

[llvmbot]

Is this still an issue? Is there a testcase?

We were seeing a similar issue while building Chrome browser for Chrome OS. It seems to be fixed now (https://code.google.com/p/chromium/issues/detail?id=558646).