Quuxplusone
commented
4 years ago Attached test.i (86226 bytes, text/plain): Input file
Open Quuxplusone opened 4 years ago
Quuxplusone
commented
4 years ago Attached test.i (86226 bytes, text/plain): Input file
Quuxplusone
commented
4 years ago Reproduced on trunk clang.
I creduced this, looking for movhi.w in the warnings and got:
$ cat example.i
a;
fn1() {
if (fn1 >= 2) {
fn2(640, a);
} else
fn2(0, a);
}
(C warnings removed)
$ /work/open_source/nightly-llvm-debug/llvm-build/bin/clang -c -Wall --
target=arm-none-eabi -mcpu=cortex-r52 -mfpu=fpv5-sp-d16 -mthumb -fshort-enums -
Os -save-temps example.i
<...>
6 warnings generated.
example.s:41:2: warning: deprecated instruction in IT block
movhi.w r0, #640
^
Fixing those C warnings will get you a movgt.w:
$ cat example.i
int a;
void fn2(int, int);
void fn1(int b) {
if (b >= 2) {
fn2(640, a);
} else
fn2(0, a);
}
$ /work/open_source/nightly-llvm-debug/llvm-build/bin/clang -c -Wall --
target=arm-none-eabi -mcpu=cortex-r52 -mfpu=fpv5-sp-d16 -mthumb -fshort-enums -
Os -save-temps example.i
example.s:39:2: warning: deprecated instruction in IT block
movgt.w r2, #640
^
We can guess that these are coming from the same decision point. I have not
looked for that yet, bugpoint might help there.We made some progress:
When we get MI ARM::t2MOVi #imm , Thumb2SizeReduction.cpp try to reduce the
size of MI.
But when imm > 255, we could not reduce to tMOVi8. t2MOVi is reserved.
However, t2MOVi is deprecated in IT instruction in v8. Bugpoint might help
there.
Notes in ARMv8-A Architecture Reference Manual, section "Partial deprecation of
IT"
https://usermanual.wiki/Pdf/ARM20Architecture20Reference20ManualARMv8.1667877052.pdf
"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."Yes it looks like restrictIT is on but by the time we make the IT block, the
wide instruction is already there and it gets wrapped up.
lib/Target/ARM/Thumb2ITBlockPass.cpp:
// v8 IT blocks are limited to one conditional op unless -arm-no-restrict-it
// is set: skip the loop
if (!restrictIT) {
This means we only get one conditional in the IT block, but it's the wide
encoding from earlier and as you said we already tried to use the smaller
encoding. So we can't just replace it.
One way to go is to also check that the instruction we're putting in the block
is not a wide encoding.
This might have some performance implications on certain cores, though if it
does it's because we're relying on this implicit detail. I'll ask around.It seems that we missed some instruction's judgement.
There are two ways:
1. As you say, to check that the instruction we're putting in the block is not
a wide encoding, but how could we get the useful 16bit instructions?
2. We can specially reduce the 32bit instruction in RuduceSpecial(), but we
don't know the other instructions we miss to check.
Like here, I am not sure it's correct:
t2MOVi(imm > 256) -> tMOVi16 + tMOVr. (tMOVr is eligible).We can at least use the same check the assembler does to not build an IT block
around the wide instruction once it has been selected. (isV8EligibleForIT)
I'm trying that locally.
Splitting them into 16 bit ops is going to depend on what we're doing. As the
other v8 IT restriction is there can only be one conditional instruction in the
block.
lib/Target/ARM/Thumb2ITBlockPass.cpp:
// v8 IT blocks are limited to one conditional op unless
movhis seem better candidates than the movgt for example. Would be interesting
to see what the effects are.Doh. My understanding of IT blocks is lacking. With my suggested change:
example.s:41:2: error: predicated instructions must be in IT block
movgt.w r2, #640
^
The instruction *must* be in a block. So yes you're right, splitting it into 16
bit versions would be the way to go.I try to split it into 16 bit versions, like here:
t2MOVi(imm > 256) -> tMOVi16 + tMOVr. (tMOVr is eligible)
but tMOVi16 (conditional) is also not eligible. So we get another same
warning...
movhi.w r2, #380
Another problem is that we should one virtual register(tGPR) to get the imm,
but it core dump on TraceDefUses()
May have something wrong.Split it into 16 bit versions is not a good way, we could not confirm that
every instr splited is eligible.
Maybe we should check if we should produce IT Block more earlier? AND Is there
any other better way?I talked to my colleagues at Arm and it turns out that this deprecation has not
been followed by the majority of cores. gas does not warn for this situation by
default anymore for this reason.
$ cat /tmp/test.s
.text
.syntax unified
.thumb_func
fn1:
it hi
movhi.w r0, #640
b fn1
(older gcc build)
$ arm-none-eabi-gcc -c -march=armv8-a -mthumb /tmp/test.s -o /tmp/test.o
/tmp/test.s: Assembler messages:
/tmp/test.s:6: IT blocks containing 32-bit Thumb instructions are deprecated in
ARMv8
(newer gcc build)
$ ./arm-none-eabi-gcc -c -march=armv8-a -mthumb /tmp/test.s -o /tmp/test.o
<no warning>
You can opt into the checking by adding an option to GAS: (there's -mrestrict-
it for code gen for GCC)
$ ./arm-none-eabi-gcc -c -march=armv8-a -mthumb /tmp/test.s -o /tmp/test.o -Wa,-
mwarn-restrict-it
/tmp/test.s: Assembler messages:
/tmp/test.s:6: IT blocks containing 32-bit Thumb instructions are performance
deprecated in ARMv8-A and ARMv8-R
There is a mention in the ARMARM of a bit that can be set in hardware to
disable these instructions but as far as I can tell that is purely for
debugging purposes in the case that there were an implementation that cared
about this.
So I think a reasonable way forward would be to make this restriction off by
default in clang as it is in gas, for assembly. No 32->16 bit conversion
necessary.
On the codegen side of things one of my colleagues posted this while back if
you're interested: https://reviews.llvm.org/D71992
Maybe it fits with your current direction, maybe not.
Do you agree that simply disabling the warning when assembling makes sense at
least?Relevant Binutils commit:
commit 24f19ccb8907b8d2bafb905a5db1a3537084d522
Author: Andre Vieira <andre.simoesdiasvieira@arm.com>
Date: Wed Dec 11 15:53:26 2019 +0000
[gas][arm] Add -mwarn-restrict-it
Add a -m{no-}warn-restrict-it option to control IT related warnings in
ARMv8-A and ARMv8-R. This is disabled by default.
Committed on behalf of Wilco Dijkstra.Thanks a lot. This(In reply to David Spickett from comment #9)
> I talked to my colleagues at Arm and it turns out that this deprecation has
> not been followed by the majority of cores. gas does not warn for this
> situation by default anymore for this reason.
Thanks a lot. It's the same as what I think about. Control IT related warnings
is the best way.Great. Sounds like you already know your way around but just in case, the
warning is emitted here: https://github.com/llvm/llvm-
project/blob/master/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp#L10788
Further clarity on GCC's option -mrestrict-it. It's "on" by default but
internally almost all the CPUs override it. So if you explicitly enable it,
it's on otherwise it's almost always off. See:
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-12/msg00134.html
I think this fits with Sam Parker's direction in the linked patch.
test.i(86226 bytes, text/plain)