[PATCH] D118044: [ARM] Undeprecate complex IT blocks

Dave Green via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 4 02:01:50 PST 2022


dmgreen added a comment.

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.



================
Comment at: llvm/test/CodeGen/ARM/2013-05-05-IfConvertBug.ll:196
 ; Hard-coded registers comes from the ABI.
 ; CHECK-LABEL: wrapDistance:
 ; CHECK: cmp r1, #59
----------------
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.


================
Comment at: llvm/test/CodeGen/ARM/arm-bf16-pcs.ll:190
 ; BASE-THUMB-NEXT:    strh.w r0, [sp, #6]
+; BASE-THUMB-NEXT:    uxth r1, r0
 ; BASE-THUMB-NEXT:    mov r0, r5
----------------
john.brawn wrote:
> 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?
Given restrictIT we run Thumb2SizeReduce earlier, which can change the scheduling.


================
Comment at: llvm/test/CodeGen/ARM/ifcvt-branch-weight.ll:22
+; CHECK: bb.1.bb2:
+; CHECK: successors: %bb.2(0x40000000)
 
----------------
MarkMurrayARM wrote:
> dmgreen wrote:
> > 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?
> It is a failure caused by my change. 
>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.


================
Comment at: llvm/test/CodeGen/ARM/speculation-hardening-sls.ll:38
 ; NOHARDENARM:     {{bxgt lr$}}
-; NOHARDENTHUMB:   {{bx lr$}}
 ; ISBDSB-NEXT: dsb sy
----------------
dmgreen wrote:
> What happened to this check line? Should it be bxgt lr now?
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.


================
Comment at: llvm/test/CodeGen/Thumb2/v8_IT_3.ll:4
 ; RUN: llc < %s -mtriple=thumbv8 -arm-atomic-cfg-tidy=0 -relocation-model=pic | FileCheck %s --check-prefix=CHECK-PIC
-; RUN: llc < %s -mtriple=thumbv7 -arm-atomic-cfg-tidy=0 -arm-restrict-it -relocation-model=pic | FileCheck %s --check-prefix=CHECK-PIC
+; RUN: llc < %s -mtriple=thumbv7 -arm-atomic-cfg-tidy=0 -arm-restrict-it -relocation-model=pic | FileCheck %s --check-prefix=CHECK-PIC-RESTRICT-IT --check-prefix=CHECK-RESTRICT-IT
 
----------------
MarkMurrayARM wrote:
> dmgreen wrote:
> > I'm not sure what this test is doing. Can you just remove -arm-restrict-it and update the check lines?
> It's checking restricted ID blocks in position-independent code. I don't see what your request will achieve?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118044/new/

https://reviews.llvm.org/D118044



More information about the cfe-commits mailing list