This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Undeprecate complex IT blocks
ClosedPublic

Authored by MarkMurrayARM on Jan 24 2022, 7:21 AM.

Details

Summary

AArch32/Armv8A introduced the performance deprecation of certain patterns
of IT instructions. After some debate internal to ARM, this is now being
reverted; i.e. no IT instruction patterns are performance deprecated
anymore, as the perfomance degredation is not significant enough.

This reverts the following:

"ARMv8-A deprecates some uses of the T32 IT instruction. All uses of
IT that apply to instructions other than a single subsequent 16-bit
instruction from a restricted set are deprecated, as are explicit
references to the PC within that single 16-bit instruction. This permits
the non-deprecated forms of IT and subsequent instructions to be treated
as a single 32-bit conditional instruction."

The deprecation no longer applies, but the behaviour may be controlled
by the -arm-restrict-it and -arm-no-restrict-it command-line options,
with the latter being the default. No warnings about complex IT blocks
will be generated.

Diff Detail

Event Timeline

MarkMurrayARM created this revision.Jan 24 2022, 7:21 AM
MarkMurrayARM requested review of this revision.Jan 24 2022, 7:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 24 2022, 7:21 AM
MarkMurrayARM retitled this revision from [ARM] AArch32/Armv8A introduced the performance deprecation of certain patterns of IT instructions. After some debate internal to ARM, this is now being reverted; i.e. no IT instruction patterns are performance deprecated anymore, as the... to [ARM] Undeprecate complex IT blocks.
MarkMurrayARM edited the summary of this revision. (Show Details)

Tweek commit message.

Please define what "complex" is, in the commit message and (if it doesn't make the description a whole paragraph) the command line help.

Given that we're flipping a default for v8 thumb targets does this warrant a release note?

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
10977

Is this warning something you'd still want if you opted into restricting IT blocks? Or can we say that because there has/will not be any core produced that follows the deprecation, you would never need the warning.

MarkMurrayARM added inline comments.Jan 24 2022, 9:21 AM
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
10977

No - in effect, deprecation is gone entirely. There is nothing to warn about. You can ask the compiler not to do it, but if you craft complex IT blocks by hand, that is up to you.

MarkMurrayARM marked an inline comment as done.Jan 24 2022, 9:22 AM

I may have missed this in the many test changes but are we left with a test that demonstrates that the codegen restriction still works? I see some that set one or the other, any with both that would catch us breaking this feature later?

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
10977

Got it, agreed.

MarkMurrayARM edited the summary of this revision. (Show Details)

Update commit message as per reviewer request.

Please define what "complex" is, in the commit message and (if it doesn't make the description a whole paragraph) the command line help.

Given that we're flipping a default for v8 thumb targets does this warrant a release note?

I've updated the commit message.

I'll look at the release notes next.

Add release note entry.

dmgreen added inline comments.
llvm/test/CodeGen/ARM/hoist-and-by-const-from-lshr-in-eqcmp-zero.ll
460

Can you make sure you run update_ll_test_checks on this file? It should fill in all these check lines for you.

llvm/test/CodeGen/ARM/ifcvt-branch-weight.ll
22

I'm not sure this is still checking anything useful. How has the full output changed? Is there not a block with two outputs anymore?

llvm/test/CodeGen/ARM/speculation-hardening-sls.ll
38

What happened to this check line? Should it be bxgt lr now?

67

Why are these removed?

llvm/test/CodeGen/Thumb2/v8_IT_3.ll
4

I'm not sure what this test is doing. Can you just remove -arm-restrict-it and update the check lines?

DavidSpickett added inline comments.Jan 27 2022, 8:11 AM
llvm/docs/ReleaseNotes.rst
73

I would mention the name of the option that changed default here. E.g.

"No deprecation warnings will be generated and -mrestrict-it is now always off by default. Previously it was on by default for Armv8 and off for all other architecture versions."

If I'm a user scanning release notes then I can grep my project makefiles and not have to care what Complex IT blocks even means if it's something I haven't specifically set.

llvm/lib/Target/ARM/ARMSubtarget.cpp
62

This implies the two are the same:
DefaultIT: normal IT blocks
RestrictedIT: IT blocks minus complex IT blocks, so, normal IT blocks?

Like if someone said oh here's the box of normal puzzles I wouldn't expect to find a complex one in there.

Perhaps "Generate any type of IT block"? Then the second one is more obviously the first minus complex.

</nitpick>

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
10960–10961

This is now unused.

john.brawn added inline comments.Jan 27 2022, 8:21 AM
llvm/test/CodeGen/ARM/2013-05-05-IfConvertBug.ll
2–5

The code generated for -mtriple=thumbv7-apple-ios and -mtriple=thumbv8 is almost the same, so instead of changing the CHECK-V8 lines and adding new CHECK-RESTRICT-IT lines, you could instead rename all of the current CHECK-V8 lines to CHECK-RESTRICT-IT and instead have -mtriple=thumbv8 check the CHECK lines, i.e.
; RUN: llc < %s -mtriple=thumbv8 | FileCheck %s

llvm/test/CodeGen/ARM/arm-and-tst-peephole.ll
181–183

The checks here are kind of a mess, in that if we look at the generated code it looks like

.LBB0_1:                                @ %tailrecurse.switch
                                        @   in Loop: Header=BB0_3 Depth=1
        cmp     r3, #1
        it      ne
        bxne    lr
.LBB0_2:                                @ %sw.bb
                                        @   in Loop: Header=BB0_3 Depth=1
        orr.w   r1, r3, r1, lsl #1
        adds    r2, #4
        add.w   r12, r12, #1
.LBB0_3:                                @ %tailrecurse
                                        @ =>This Inner Loop Header: Depth=1
        ldr     r3, [r2, #-4]
        ands    r3, r3, #3
        beq     .LBB0_2
@ %bb.4:                                @ %tailrecurse.switch
                                        @   in Loop: Header=BB0_3 Depth=1
        cmp     r3, #3
        itt     eq
        moveq   r0, r2
        bxeq    lr
.LBB0_5:                                @ %tailrecurse.switch
                                        @   in Loop: Header=BB0_3 Depth=1
        cmp     r3, #2
        bne     .LBB0_1
@ %bb.6:                                @ %sw.bb8
        add     r1, r12
        add.w   r0, r0, r1

We have a load of V8-NEXT lines where it's variously: checking the instruction opcode; checking the entire instruction; checking for some text that appears in a comment (e.g. V8-NEXT: %tailrecurse.switch); checking that a comment appears but not what's in it (e.g. V8-NEXT: @).

I think it would make more sense to restructure so it's a sequence of

; V8-LABEL: @ %llvm_ir_name_in_a_comment
; V8: first_instruction_in_block
; V8-NEXT: next_instruction
; v8-NEXT: etc.
llvm/test/CodeGen/ARM/arm-bf16-pcs.ll
190

This, and the cases in other tests where we have a uxth/uxtb that moves, looks rather strange and not something I'd expect given that there's no IT here. Do you know what's going on here?

FWIW, a 2 stage build on armv8 with -mthumb didn't turn up any codegen Gremlins. (that's how I saw the unused var)

Address reviewer comments.

MarkMurrayARM marked 3 inline comments as done.Feb 3 2022, 4:52 AM
MarkMurrayARM added inline comments.
llvm/test/CodeGen/ARM/ifcvt-branch-weight.ll
22

It is a failure caused by my change.

llvm/test/CodeGen/Thumb2/v8_IT_3.ll
4

It's checking restricted ID blocks in position-independent code. I don't see what your request will achieve?

MarkMurrayARM marked an inline comment as done.Feb 3 2022, 4:56 AM

The actual code change looks fine to me, providing we can clean up these test changes a bit and no-one else has any other comments.

llvm/test/CodeGen/ARM/2013-05-05-IfConvertBug.ll
305

If you autogenerate check lines we have to make sure we remove the old ones, but I'm not sure the new check lines are filling in all the details. They can do that if the check lines dont match between multiple run lines with the same check-prefix.

It is probably simplest to change the check lines to
; RUN: llc < %s -mtriple=thumbv8 -arm-restrict-it | FileCheck -check-prefix=CHECK-V8 %s
; RUN: llc < %s -mtriple=thumbv7 -arm-restrict-it | FileCheck -check-prefix=CHECK-V8 %s
And remove all the other differences from this file.

llvm/test/CodeGen/ARM/arm-bf16-pcs.ll
190

Given restrictIT we run Thumb2SizeReduce earlier, which can change the scheduling.

llvm/test/CodeGen/ARM/ifcvt-branch-weight.ll
22

From what I can tell, this test is trying test that the block weights are correct after ifcvt. It would be best to make sure that is still being tested.

It's probably best to add -arm-restrict-it to this test, so it can keep testing the same thing.

llvm/test/CodeGen/ARM/speculation-hardening-sls.ll
38

Change this to ; NOHARDENTHUMB: {{bxgt lr$}}
It should then be the only change needed in this file, the other checks will fall into place without needing any modification. It looks like different lines were matching than what was expected, which threw off the ones below.

llvm/test/CodeGen/Thumb2/v8_IT_3.ll
4

Oh OK. I hadn't realized this test was for restricts it blocks specifically. In that case maybe just add -arm-restrict-it to the thumbv8 lines and leave the check lines as-is. That would then be the same as v8_IT_5.ll.

Otherwise, this needn't have both CHECK-PIC-RESTRICT-IT and CHECK-RESTRICT-IT, one of the two should be fine.

Fix more tests.

MarkMurrayARM marked 4 inline comments as done.Feb 4 2022, 9:19 AM

Address review comments

MarkMurrayARM marked 3 inline comments as done.Feb 7 2022, 5:16 AM
dmgreen accepted this revision.Feb 7 2022, 5:21 AM

Thanks for the updates. LGTM.

llvm/test/CodeGen/ARM/ifcvt-branch-weight.ll
20–21

I think this should still be bb.2.bb2

This revision is now accepted and ready to land.Feb 7 2022, 5:21 AM
MarkMurrayARM marked 2 inline comments as done.Feb 7 2022, 7:24 AM
MarkMurrayARM added inline comments.
llvm/test/CodeGen/ARM/ifcvt-branch-weight.ll
20–21

If I do that the test fails. It's definitely like this in the generated code.

This revision was landed with ongoing or failed builds.Feb 7 2022, 7:48 AM
This revision was automatically updated to reflect the committed changes.
MarkMurrayARM marked an inline comment as done.
llvm/test/MC/ARM/deprecated-v8.s