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

Wed Jan 15 13:29:08 PST 2020

jonpa updated this revision to Diff 238345.
jonpa added a comment.

> 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.

-msoft-float now gives "-vector" in addition to "use-soft-float"="true"
In the subtarget, HasSoftFloat clears HasVector, so that "soft-float"="true" attribute implies "-vector" on the LLVM function.

> 
> 
> - 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).

Yes, covered implicitly by -mno-vx, but thought it was more readable to have it in the same if clause.

> 
> 
> - 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.

Done. I did not find anything else either.  I tried making a test case to check if the machine verifier would help us to detect any use of fp/vector regs with -soft-float. It seems that it currently does not do this. I edited a .mir test case by inserting a vector instruction using vector regs while the "+soft-float" attribute was present. The verifier did not catch this as an error, but I think we could patch the verifier once we have the soft-float support in the backend (this patch).

> - 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.

Hope to have covered most of the calling convention for ingoing and outgoing arguments as well as return values in soft-float-args.ll. I reused the existing test cases for the multiple return values and inserted the generated code there, for which I can at least say is not using any fp/vector registers.  I started by collecting simple new tests soft-float*.ll, but then realized it might be even better to rerun existing tests with SOFT-FLOAT checks instead, as in eg vec-args-06.ll. Not sure if that's needed (to redo the tests)?

> 
> 
> - 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.

Done, with added test matching output. I am a little curious as to why this is needed...

> - 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?)

I implemented that and updated systemz-abi.c accordingly to pass (again, not sure about the details here). (Seems that isFPArgumentType() takes a QualType argument, and not Type...)

> - 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.

Removed the adding of the feature. Also aemoved the explicit generation of "-mfloat-abi" "hard", in AddSystemZTargetArgs (like X86).

> 
> 
> - 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.

Added a check against using -mfloat-abi with the clang driver.

When building SPEC, I got one failure due to the generation of a s390.tdc intrinsic, which resulted in

  SoftenFloatOperand Op #1: t6: i32 = llvm.s390.tdc TargetConstant:i64<6064>, t4, Constant:i64<2730>

  Do not know how to soften this operator's operand!
  UNREACHABLE executed at /home/ijonpan/llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:765!

It seems to me best to not do this optimization, so I disabled that in SystemZTDC.cpp with a check for the soft-float function attribute. I am however not quite happy with this, because if the function attribute in the test case is removed, and then -mattr=soft-float is passed to llc, this still fails. I wanted to get the subtarget and do the query to (SystemZ) Subtarget.hasSoftFloat(), but I haven't yet been able to figure out how to do that in SystemZTDC.cpp. SystemZTargetTransformInfo has the SystemZSubtarget subtarget pointer, but I don't know how to access it that way either... :-/

BTW, I see that hasVectorEnhancment1/2 returns true even with when hasVector() returns false. This is probably a separate topic, but to me it seems preferred to return the expected values. The subtarget features are built up nicely in an incremental way so one might be mislead to believe that this is always reflected when checking for subtarget features. Or maybe it's enough to just assume that since the types are not legal, this will never matter..?

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

https://reviews.llvm.org/D72189

Files:
  clang/lib/Basic/Targets/SystemZ.cpp
  clang/lib/Basic/Targets/SystemZ.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
  clang/lib/Driver/ToolChains/Arch/SystemZ.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/systemz-abi.c
  clang/test/CodeGen/systemz-abi.cpp
  clang/test/Driver/systemz-float.c
  llvm/lib/Target/SystemZ/SystemZFeatures.td
  llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
  llvm/lib/Target/SystemZ/SystemZISelLowering.h
  llvm/lib/Target/SystemZ/SystemZSubtarget.cpp
  llvm/lib/Target/SystemZ/SystemZSubtarget.h
  llvm/lib/Target/SystemZ/SystemZTDC.cpp
  llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp
  llvm/lib/Target/SystemZ/SystemZTargetMachine.h
  llvm/test/CodeGen/SystemZ/args-07.ll
  llvm/test/CodeGen/SystemZ/soft-float-01.ll
  llvm/test/CodeGen/SystemZ/soft-float-02.ll
  llvm/test/CodeGen/SystemZ/soft-float-03.ll
  llvm/test/CodeGen/SystemZ/soft-float-04.ll
  llvm/test/CodeGen/SystemZ/soft-float-args.ll
  llvm/test/CodeGen/SystemZ/soft-float-inline-asm-01.ll
  llvm/test/CodeGen/SystemZ/soft-float-inline-asm-02.ll
  llvm/test/CodeGen/SystemZ/soft-float-inline-asm-03.ll
  llvm/test/CodeGen/SystemZ/soft-float-inline-asm-04.ll
  llvm/test/CodeGen/SystemZ/vec-args-06.ll
  llvm/test/CodeGen/SystemZ/vec-args-07.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72189.238345.patch
Type: text/x-patch
Size: 56272 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200115/372b5dee/attachment-0001.bin>