Restrict ARM IT Blocks on Windows

[dpaoliello]

Summary

Per Microsoft's documentation (https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions?view=msvc-160#it-instructions) ARM IT Blocks should be restricted as if targeting ARMv8, even on older versions of ARM.

[mstorsjo]

This change seems fine to me, but can we add some sort of testcase for it? Do we have an existing testcase that check for the default behaviour when targeting armv8 somewhere?

[kristof.beyls]

The performance deprecation of certain forms of IT blocks has been reverted.
The performance deprecation notes are being removed from the Arm ARM.
I am expecting that in the near future, code generation for Arm will be adapted to not enable -mrestrict-it by default at all, following what was already done in gcc at https://gcc.gnu.org/legacy-ml/gcc-patches/2019-12/msg00134.html.

As such, I'm not sure if this patch is a good thing to implement.
Maybe the guidelines in Microsoft's document you point to (https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions?view=msvc-160#it-instructions) should be adapted to take into account the above described changes to the ArmARM?

[dpaoliello]

In D108679#2964617, @kristof.beyls wrote:

The performance deprecation of certain forms of IT blocks has been reverted.
The performance deprecation notes are being removed from the Arm ARM.
I am expecting that in the near future, code generation for Arm will be adapted to not enable -mrestrict-it by default at all, following what was already done in gcc at https://gcc.gnu.org/legacy-ml/gcc-patches/2019-12/msg00134.html.

As such, I'm not sure if this patch is a good thing to implement.
Maybe the guidelines in Microsoft's document you point to (https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions?view=msvc-160#it-instructions) should be adapted to take into account the above described changes to the ArmARM?

@kristof.beyls Can you please point me to any official documentation or announcement from ARM about this (I've tried looking myself but couldn't see anything)?
I'd be happy to abandon this change and get the Microsoft docs updated if these instructions are no longer deprecated.

[dpaoliello]

Added tests for Restricting IT blocks for v7 vs v8, optimizing for size and Windows.

[dpaoliello]

In D108679#2964349, @mstorsjo wrote:

This change seems fine to me, but can we add some sort of testcase for it? Do we have an existing testcase that check for the default behaviour when targeting armv8 somewhere?

I could not find any existing tests specifically checking if IT blocks are restricted or not, so I added tests for the existing behavior and the new Windows behavior.

[kristof.beyls]

In D108679#2965397, @dpaoliello wrote:

In D108679#2964617, @kristof.beyls wrote:

The performance deprecation of certain forms of IT blocks has been reverted.
The performance deprecation notes are being removed from the Arm ARM.
I am expecting that in the near future, code generation for Arm will be adapted to not enable -mrestrict-it by default at all, following what was already done in gcc at https://gcc.gnu.org/legacy-ml/gcc-patches/2019-12/msg00134.html.

As such, I'm not sure if this patch is a good thing to implement.
Maybe the guidelines in Microsoft's document you point to (https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions?view=msvc-160#it-instructions) should be adapted to take into account the above described changes to the ArmARM?

@kristof.beyls Can you please point me to any official documentation or announcement from ARM about this (I've tried looking myself but couldn't see anything)?
I'd be happy to abandon this change and get the Microsoft docs updated if these instructions are no longer deprecated.

I have not found a direct public announcement from Arm about this.
The "performance deprecation for certain forms of IT blocks" notes in the Arm Architecture Reference Manual (ArmARM) should have been all removed from the latest version at https://developer.arm.com/documentation/ddi0487/latest
I've found that a few places were missed and raised a defect against the Arm Architecture Reference Manual to get them all removed.
AFAIK, not having these "performance deprecation" notes in the ArmARM may end up being the only documentation about this.

[dpaoliello]

In D108679#2972326, @kristof.beyls wrote:

In D108679#2965397, @dpaoliello wrote:

In D108679#2964617, @kristof.beyls wrote:

The performance deprecation of certain forms of IT blocks has been reverted.
The performance deprecation notes are being removed from the Arm ARM.
I am expecting that in the near future, code generation for Arm will be adapted to not enable -mrestrict-it by default at all, following what was already done in gcc at https://gcc.gnu.org/legacy-ml/gcc-patches/2019-12/msg00134.html.

As such, I'm not sure if this patch is a good thing to implement.
Maybe the guidelines in Microsoft's document you point to (https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions?view=msvc-160#it-instructions) should be adapted to take into account the above described changes to the ArmARM?

@kristof.beyls Can you please point me to any official documentation or announcement from ARM about this (I've tried looking myself but couldn't see anything)?
I'd be happy to abandon this change and get the Microsoft docs updated if these instructions are no longer deprecated.

I have not found a direct public announcement from Arm about this.
The "performance deprecation for certain forms of IT blocks" notes in the Arm Architecture Reference Manual (ArmARM) should have been all removed from the latest version at https://developer.arm.com/documentation/ddi0487/latest
I've found that a few places were missed and raised a defect against the Arm Architecture Reference Manual to get them all removed.
AFAIK, not having these "performance deprecation" notes in the ArmARM may end up being the only documentation about this.

Looks like you're right: I'm going to go ahead and abandon this change.

[mstorsjo]

In D108679#2975603, @dpaoliello wrote:

Looks like you're right: I'm going to go ahead and abandon this change.

@dpaoliello Should the Windows ARM ABI docs be updated to stop discouraging those forms then too? And I guess armasm should stop warnings on such forms? Can you file requests internally to get that started?

llvm/lib/Target/ARM/ARMSubtarget.cpp

void ARMSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) {
   // registers are the 4 used for parameters.  We don't currently do this
   // case.
 
   SupportsTailCall = !isThumb1Only() || hasV8MBaselineOps();
 
   if (isTargetMachO() && isTargetIOS() && getTargetTriple().isOSVersionLT(5, 0))
     SupportsTailCall = false;
 
+  // Multi-instruction IT Blocks have been deprecated in ARMv8, additionally
+  // Windows does not permit them, even for older ARM versions.
+
   switch (IT) {
   case DefaultIT:
-    RestrictIT = hasV8Ops() && !hasMinSize();
+    RestrictIT = (hasV8Ops() && !hasMinSize()) || isTargetWindows();
     break;
   case RestrictedIT:
     RestrictIT = true;
     break;
   case NoRestrictedIT:
     RestrictIT = false;
     break;
   }

llvm/test/CodeGen/ARM/restrict-it-minsize.ll

+; RUN: llc < %s -mtriple=thumbv7-linux-gnueabihf | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv8-linux-gnueabihf | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv7-pc-windows-msvc | FileCheck %s --check-prefix=CHECK-RESTRICTIT
+; RUN: llc < %s -mtriple=thumbv8-pc-windows-msvc | FileCheck %s --check-prefix=CHECK-RESTRICTIT
+
+; Verifies that IT blocks are unrestricted when optimizing for size, except on Windows.
+
+define i32 @bundled_instruction(i32* %addr, i32** %addr2, i1 %tst) minsize optsize {
+; CHECK-LABEL: bundled_instruction:
+; CHECK: itee ne
+; CHECK: movne  r0, #0
+; CHECK: ldmeq r3!, {r0}
+; CHECK: streq r3, [r1]
+; CHECK-RESTRICTIT: beq     {{.+}}0_2
+; CHECK-RESTRICTIT:  movs    r0, #0
+; CHECK-RESTRICTIT: {{.+}}0_2:
+; CHECK-RESTRICTIT: ldm r3!, {r0}
+; CHECK-RESTRICTIT: str r3, [r1]
+  br i1 %tst, label %true, label %false
+
+true:
+  ret i32 0
+
+false:
+  %res = load i32, i32* %addr, align 4
+  %next = getelementptr i32, i32* %addr, i32 1
+  store i32* %next, i32** %addr2
+  ret i32 %res
+}

llvm/test/CodeGen/ARM/restrict-it.ll

+; RUN: llc < %s -mtriple=thumbv7-linux-gnueabihf | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv8-linux-gnueabihf | FileCheck %s --check-prefix=CHECK-RESTRICTIT
+; RUN: llc < %s -mtriple=thumbv7-pc-windows-msvc | FileCheck %s --check-prefix=CHECK-RESTRICTIT
+; RUN: llc < %s -mtriple=thumbv8-pc-windows-msvc | FileCheck %s --check-prefix=CHECK-RESTRICTIT
+
+; Verifies that restricted IT blocks are enabled for v8, and always on Windows.
+
+define i32 @bundled_instruction(i32* %addr, i32** %addr2, i1 %tst) {
+; CHECK-LABEL: bundled_instruction:
+; CHECK: itee ne
+; CHECK: movne  r0, #0
+; CHECK: ldreq  r0, [r3], #4
+; CHECK: streq  r3, [r1]
+; CHECK-RESTRICTIT: beq     {{.+}}0_2
+; CHECK-RESTRICTIT:  movs    r0, #0
+; CHECK-RESTRICTIT: {{.+}}0_2:
+; CHECK-RESTRICTIT: ldr     r0, [r3], #4
+; CHECK-RESTRICTIT: str     r3, [r1]
+  br i1 %tst, label %true, label %false
+
+true:
+  ret i32 0
+
+false:
+  %res = load i32, i32* %addr, align 4
+  %next = getelementptr i32, i32* %addr, i32 1
+  store i32* %next, i32** %addr2
+  ret i32 %res
+}