This is an archive of the discontinued LLVM Phabricator instance.

Add feature to determine if host architecture is 64-bit in llvm-lit
ClosedPublic

Authored by jakehehrlich on Oct 31 2017, 11:27 AM.

Details

Summary

I have a test that I'd like to add to llvm that demands using more than 32-bits worth of address space. This test can't be run on 32-bit systems because they don't have enough address space. The host triple should be used to determine this instead of config.host_arch because on Debian systems config.host_arch is not correct. This change adds the "host-arch-is-64bit" feature to allow tests to restrict themselves to the 64-bit case.

Diff Detail

Repository
rL LLVM

Event Timeline

jakehehrlich created this revision.Oct 31 2017, 11:27 AM
zturner added inline comments.Oct 31 2017, 11:41 AM
test/lit.cfg.py
171 ↗(On Diff #121023)

So if I remember correctly from discussing on IRC, HOST_ARCH is the equivalent of running uname -p, but now we're adding host-arch-is-64bit which is not a simple function of HOST_ARCH, since uname -p might return unknown.

If so, I think this is going to lead to confusion. To be honest, if HOST_ARCH is unreliable as you say, then we shouldn't be using it for anything and I would just assume deprecate it in favor of having everything be based on llvm_host_triple.

With that in mind, I think it would be better to just delete this feature.

173–175 ↗(On Diff #121023)

This is again slightly confusing, because what does "host arch" mean? Does it mean the physical processor installed in the machine? The bitness of the OS that is running? Or the bitness of the LLVM toolchain that is running? In this case it's the latter (the bitness of the LLVM toolchain that is running), so can we call this "llvm-64-bits" or something to make it more clear what this feature actually represents?

Also, as a minor nit, I think it would be slightly more pythonic to write this as:

known_arches = ["x86_64", "mips64", "ppc64", "aarch64"]
if any(config.llvm_host_triple.startswith(x) for x in known_arches):
  config.available_features.add("llvm-64-bits")
  1. Deleted config.host_arch as a feature
  2. Changed host-arch-is64bit to llvm-64-bits
  3. Changed code to by more idiomatic as pointed out by Zach
zturner accepted this revision.Nov 1 2017, 5:24 PM

I actually think that the llvm-64-bits feature should go in the common config.py setup, but I don't think we're quite there yet. It would require some additional work to make sure everyone who uses LLVM can agree on a common set of variables that must be passed in through configure, and currently we don't have that.

So I think this is good for now, and maybe in the future I'll try to generalize it a bit so this can be moved into config.py

This revision is now accepted and ready to land.Nov 1 2017, 5:24 PM
This revision was automatically updated to reflect the committed changes.