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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
