[TargetParser][AArch64] Believe runtime feature detection

[workingjubilee]

In #90365 it was reported that TargetParser arrives at the wrong conclusion regarding what features are enabled when attempting to detect "native" features on the Raspberry Pi 4, because it (correctly) detects it as a Cortex-A72, but LLVM (incorrectly) believes all Cortex-A72s have crypto enabled. Attempt to help ourselves by allowing runtime information derived from the host to contradict whatever we believe is "true" about the architecture.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[workingjubilee]

I have no idea which tests are tests I should ignore the breakage of or which tests I should investigate. I have never dealt before with a tree that allows regular breakage to be checked in to its core test suite.

[davemgreen]

Thanks for working on adding this. "crypto" is an old feature-name now, which has been split-up into aes+sha2, and the newer sha3+sm4 features in certain situations. It would technically be incorrect to turn "aes+sha2" into "crypto" if "crypto" is turned back into "sha2+aes+sha3+sm4". I think it might be aes = CAP_AES&CAP_PMULL and sha2 = CAP_SHA1&CAP_SHA2, so they don't necessarily fit into the LLVMFeatureStr map unfortunately.

Can we split this into individual sha2 and aes features? And can we add a test to TargetParserTest.cpp?

[workingjubilee]

@davemgreen That seems like blatant feature creep. I mean, I'll have to see if there is any test framework for testing native feature detection, but the rest?

It would technically be incorrect to turn "aes+sha2" into "crypto" if "crypto" is turned back into "sha2+aes+sha3+sm4"

And where does that condition actually obtain?

On the other hand, I am happy to incorporate such fixes to adjust the target feature schema for LLVM to make it more accurate if I am granted liberty to actually correct the inaccuracies.

[workingjubilee]

g-d forbid, this is actually done? crypto is just inferred to mean something based on aarch64 version? what.

https://github.com/llvm/llvm-project/blob/4447e255a908c4e1a2863374eaee4bc98e773c3d/llvm/lib/TargetParser/AArch64TargetParser.cpp#L184C53-L184C57

let me rephrase my previous statement: you want me to delete the crypto feature because it is incoherent, and leave it to downstreams to decide if they want to support this grab-bag of whatever, right?

[workingjubilee]

eh? but this contradicts it? er?

llvm-project/llvm/lib/Target/AArch64/AArch64Features.td

Lines 134 to 144 in a4b44c0

	// Crypto has been split up and any combination is now valid (see the
	// crypto definitions above). Also, crypto is now context sensitive:
	// it has a different meaning for e.g. Armv8.4 than it has for Armv8.2.
	// Therefore, we rely on Clang, the user interfacing tool, to pass on the
	// appropriate crypto options. But here in the backend, crypto has very little
	// meaning anymore. We kept the Crypto definition here for backward
	// compatibility, and now imply features SHA2 and AES, which was the
	// "traditional" meaning of Crypto.
	let FMVDependencies = "+aes,+sha2" in
	def FeatureCrypto : Extension<"crypto", "Crypto",
	"Enable cryptographic instructions", [FeatureNEON, FeatureSHA2, FeatureAES]>;

hm, so the part that does the inference predates the other, but only by a few months, this year...

[workingjubilee]

...and why is crypto detection behind the aarch64 ifdef? 32-bit Arm CPUs can support aes and sha2, can't they? I mean maybe they usually won't, but it doesn't seem correct to refuse to detect it if the kernel indicates otherwise.

[davemgreen]

Hi - That is true, it has always been like that so shouldn't be the job of this patch to try and fix it. If you can add some testing though (hopefully to TargetParserTest.cpp) this should be OK.

[workingjubilee]

I don't see how native feature detection is tested in TargetParserTest.cpp? Is it just... not?

We'd need to mock the host detection functions called in order to do so, right?

[davemgreen]

Oh I was meaning in llvm/unittests/TargetParser/Host.cpp. It has tests for parsing things that look like cpuinfo.

[workingjubilee]

Oh I was meaning in llvm/unittests/TargetParser/Host.cpp. It has tests for parsing things that look like cpuinfo.

Sure, but it doesn't have any tests for sys::getHostCPUFeatures, because there are none, because the entire schematic for looking up host features is "do something target-dependent at runtime", and thus is currently untestable, unless someone alters the signature for that function for every single platform...

...and considering my incredibly meager C++ knowledge, it seems less effort to simply fix the definitions of the Armv8-A Cortex targets.

[workingjubilee]

This only becomes unit-testable, even for just AArch64, with a reasonable amount of effort (as opposed to the unreasonable amount I just cited) if it is acceptable to kill off the ifdefs and then expose the internal string -> StringMap parser.

But it may be a better idea to instead just switch to using HWCAP and HWCAP2? Unless LLVM still supports being built with glibc 2.15 or less???

[davemgreen]

OK. We would usually want some form of testing for a change like this, but it might be alright so long as we verify it is working manually. As you can tell this bit of code isn't super well maintained, and could do with a number of improvements.

Are you sure this function gets called for -mcpu=native, or are you using this from somewhere that isn't clang?

[davemgreen]

I know a number of the existing variable names are already wrong, but llvm prefers CamelCase for variable names.

[workingjubilee]

Ehm, yes?

llvm-project/clang/lib/Driver/ToolChains/Arch/ARM.cpp

Lines 592 to 605 in 9f44d5d

	// Add CPU features for generic CPUs
	if (CPUName == "native") {
	llvm::StringMap<bool> HostFeatures;
	if (llvm::sys::getHostCPUFeatures(HostFeatures))
	for (auto &F : HostFeatures)
	Features.push_back(
	Args.MakeArgString((F.second ? "+" : "-") + F.first()));
	} else if (!CPUName.empty()) {
	// This sets the default features for the specified CPU. We certainly don't
	// want to override the features that have been explicitly specified on the
	// command line. Therefore, process them directly instead of appending them
	// at the end later.
	DecodeARMFeaturesFromCPU(D, CPUName, Features);
	}

Rust primarily uses this:

llvm-project/llvm/include/llvm-c/TargetMachine.h

Lines 231 to 233 in 9f44d5d

	/** Get the host CPU's features as a string. The result needs to be disposed
	with LLVMDisposeMessage. */
	char* LLVMGetHostCPUFeatures(void);

[davemgreen]

OK. I think this might be used in clang for ARM but not AArch64 at the moment. If you can change the variable names, and you have tested this in your use-case (rust?) then this looks OK to me.

[davemgreen]

There is a PR to use this function in clang in #97749.

[ElvinaYakubova]

@workingjubilee Hi! Do you plan to continue working on this?

[workingjubilee]

Did anyone get the license plate of that runaway thought?

[davemgreen]

This looks sensible to me. Thanks for making the change. I don't believe it gets used from clang yet, hopefully #97749 can improve upon that.

LGTM

[workingjubilee]

Thank you for the review! Sorry about getting brain-flattened.

[github-actions]

@workingjubilee Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!