Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

aarch64: -march=armv8-a produces armv8.2 'bfc' instruction #41921

Closed
arndb mannequin opened this issue Jul 11, 2019 · 15 comments
Closed

aarch64: -march=armv8-a produces armv8.2 'bfc' instruction #41921

arndb mannequin opened this issue Jul 11, 2019 · 15 comments
Assignees
@nickdesaulniers
Labels
bugzilla Issues migrated from bugzilla c

Comments

@arndb
Copy link
Mannequin

arndb mannequin commented Jul 11, 2019

Bugzilla Link 42576
Resolution FIXED
Resolved on Nov 25, 2019 10:48
Version trunk
OS Linux
Blocks #4440 #42705
CC @DougGregor,@efriedma-quic,@nickdesaulniers,@zygoloid,@smithp35,@TNorthover,@tstellar
Fixed by commit(s) r373655 62a16ca

Extended Description

Building arm64 linux randconfig kernels, I came across this assembler error:

/tmp/sdma_v4_0-f95fd3.s: Assembler messages:
/tmp/sdma_v4_0-f95fd3.s:44: Error: selected processor does not support `bfc w0,#1,#5'

I reduced the test case to

int a, b, c;
int __order_base_2() { return a > 1 ? __builtin_constant_p(a) ?: b : 0; }
int sdma_v4_0_rb_cntl(int p1) {
  int d = c ?: __order_base_2();
  p1 = (p1 & ~0x0000003EL) | (62 & (d << 1));
  return p1;
}
$ clang-8  --prefix=/home/arnd/cross/x86_64/gcc-8.1.0-nolibc/aarch64-linux/bin/ -no-integrated-as -O2 sdma_v4_0.i --target=aarch64-linux -S -o- -march=armv8.1-a

https://godbolt.org/z/8Gcjl9 shows the 'bfc' instruction being generated, but not the error message as it is unable to call the external assembler correctly.

According to https://sourceware.org/ml/binutils/2015-11/msg00221.html, the 'bfc' instruction is only valid in armv8.2 or higher but llvm uses it for any armv8 version.
@arndb
Copy link
Mannequin Author

arndb mannequin commented Jul 11, 2019

assigned to @nickdesaulniers

@TNorthover
Copy link
Contributor

It's an alias so the underlying instruction exists in v8.0. Gating it on the architecture is weird.

We might want to change how we print it when targeting < v8.2, but I'm pretty disinclined to diagnose an error in our own assembler.

@nickdesaulniers
Copy link
Member

-march=armv8.1-a

Orthogonal, Arnd, is there a config that sets that specific march flag?

but I'm pretty disinclined to diagnose an error in our own assembler.

Would be a bug in AsmPrinter?

@TNorthover
Copy link
Contributor

Would be a bug in AsmPrinter?

In LLVM terms, if the current behaviour is considered a bug, it would be fixed in what's called the "InstPrinter" in lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp.

There's specific C++ code to detect and print "bfc", which could be modified to only do that when targeting v8.2a or above.

How's that for a carefully hedged reply!

@arndb
Copy link
Mannequin Author

arndb mannequin commented Jul 11, 2019

-march=armv8.1-a

Orthogonal, Arnd, is there a config that sets that specific march flag?

I don't think so, I added that to the test case for clarification.

If I remember correctly, the kernel is always built for armv8-a rather than a later version, to provide the best compatibility with existing CPUs (most of what is in production today is still armv8-a, not 8.1 or 8.2).

It's an alias so the underlying instruction exists in v8.0. Gating it on the
architecture is weird.

We might want to change how we print it when targeting < v8.2, but I'm
pretty disinclined to diagnose an error in our own assembler.

Makes sense. As I understand section A1.7.4 in the ARM ARM, an assembler is only required to understand bfc when targetting v8.2, but it is may choose to interpret it correctly anyway.

@TNorthover
Copy link
Contributor

Makes sense. As I understand section A1.7.4 in the ARM ARM, an assembler is only required to understand bfc when targetting v8.2, but it is may choose to interpret it correctly anyway.

Nicely put! I wish I'd thought of that when writing my reply.

@arndb
Copy link
Mannequin Author

arndb mannequin commented Oct 2, 2019

I see the bug is still present in clang-9 and clang-10, so I'm resending my workaround for the kernel.

@nickdesaulniers
Copy link
Member

https://reviews.llvm.org/D68356

@nickdesaulniers
Copy link
Member

https://reviews.llvm.org/rL373655

@tstellar
Copy link
Collaborator

tstellar commented Oct 4, 2019

https://reviews.llvm.org/rL373655

Tim, is this OK to merge to the release/9.x branch?

@TNorthover
Copy link
Contributor

I think so.

@efriedma-quic
Copy link
Collaborator

(Reopening to track 9.0.1.)

@tstellar
Copy link
Collaborator

Merged: 62a16ca

@nico
Copy link
Contributor

nico commented Nov 27, 2021

mentioned in issue #42705

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 27, 2021

changed the description

@nico nico mentioned this issue Sep 19, 2019
Closed
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nickdesaulniers nickdesaulniers

Labels
bugzilla Issues migrated from bugzilla c
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nico @tstellar @TNorthover @nickdesaulniers @efriedma-quic @llvmbot