[PATCH] D72189: [SystemZ] Support -msoft-float

Mon Jan 6 06:08:32 PST 2020

uweigand added a comment.

Just a couple of general comments for now, not yet going into implementation details.

The point of -msoft-float is to ensure that the compiler generates code that never touches any floating-point (or vector) register.    To ensure this mainly requires:

- A change of the ABI to not use floating-point or vector registers to pass anything.   In particular, this means -msoft-float implies -mno-vx so that nothing using vector registers; in addition, the ABI is changed by -msoft-float so that floating-point values are passed like integer values of the same size (in GPRs and/or on the stack).

- Changing the compiler to ensure nothing creates any implicit use of floating-point (or vector) registers, e.g. to temporarily hold values, or to implement auto-vectorization.

(Some targets, in particular ARM, allow a third mode that uses the soft-float ABI at call boundaries, but is still allowed to make internal use of floating-point registers otherwise.  We've never supported this on SystemZ.)

To implement the second part in clang/LLVM, I believe we need to

- Have -msoft-float imply -mno-vx (either on the clang side, or the LLVM side, or both).   This doesn't seem to be implemented in the patch yet.

- If -msoft-float is active, have all floating-point (and vector) types be marked as not legal in LLVM.   This is done by the patch (at least for floating-point types -- vector types should be covered as soon as -msoft-float implies -mno-vx).

- Remove any other uses of the floating-point (or vector) register classes in SystemZ target code.  In particular, I think we need to stop using FP classes to implement the 'f' constraint in SystemZTargetLowering::getRegForInlineAsmConstraint.  Not sure if I missed anything else.

Now, to implement the first part (ABI), I think we need additional changes both on the clang and LLVM side:

- For arguments of type float or double, I think this should be handled on the LLVM side.  In fact, this may be done automatically if the types are marked as not legal, since then they should be converted to i32 or i64 by the type legalizer (TypeSoftenFloat action).  This needs to be verified (and test cases!) though.

- The special case handling of aggregates with a single float/double member needs to be deactivated.  I think this should be done in clang, probably best by having SystemZABIInfo::isFPArgumentType return false if msoft-float is active.

- Variable argument handling needs to be updated to match.  One part (on the LLVM side) is already in this patch.  But there needs to be a corresponding change in clang, in SystemZABIInfo::EmitVAArg -- the InFPRs flag always must be false with -msoft-float.  (Why isn't this using SystemZABIInfo::isFPArgumentType in the first place?)

Now as to the various interfaces:

- In LLVM, you now have both a **feature** (+soft-float, -soft-float) and a function attribute ("use-soft-float").  I'm not really sure we need both ... and in fact, X86 doesn't do it that way either.  I think we don't need the explicit feature here.

- In clang-cc1, common code supports both the -mfloat-abi={soft,hard} flag (to define the FP ABI only) and the -msoft-float flag (to probihit internal use of FP registers).  I guess it makes sense to just re-use those same flags on SystemZ (as your patch does), even if we don't actually support the "Soft-FP ABI but allow internal FP use" mode.

- However in the clang driver, your patch now also supports both the -msoft-float/-mhard-float and -mfloat-abi=... flags.  Since GCC does not support -mfloat-abi=... on SystemZ, I believe we should not support it either in clang, for consistency.

As a general note on testing, since -msoft-float defines a new ABI, you'd need a set of system libraries build with -msoft-float in order to run any real tests.  Given that we've never really supported -msoft-float for user-space, no current SystemZ distro actually provides those libraries.  So there probably cannot be any better tests than just normal .ll unit tests.

The only user of -msoft-float is the Linux kernel -- which doesn't use libraries anyway.  However, the kernel does rely on -msoft-float because kernel code **must not** ever access floating-point registers since those are not saved/restored when switching between user and kernel code.  So if we want to enable building the kernel with clang, it is mandatory to correctly support -msoft-float.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72189/new/

https://reviews.llvm.org/D72189