First attempt of reusing code from other targets to recognize -msoft-float on SystemZ.
Seems to work, and tests are passing.
Differential D72189
[SystemZ] Support -msoft-float jonpa on Jan 3 2020, 3:47 PM. Authored by
Details
First attempt of reusing code from other targets to recognize -msoft-float on SystemZ. Seems to work, and tests are passing.
Diff Detail Event TimelineComment Actions Don't save fp-regs with varargs + soft-float Not sure how to use -msoft-float (tried linking various libraries but did not work...) Is this a priority for the next LLVM release? Comment Actions 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:
(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
Now, to implement the first part (ABI), I think we need additional changes both on the clang and LLVM side:
Now as to the various interfaces:
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. Comment Actions
-msoft-float now gives "-vector" in addition to "use-soft-float"="true"
Yes, covered implicitly by -mno-vx, but thought it was more readable to have it in the same if clause.
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).
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)?
Done, with added test matching output. I am a little curious as to why this is needed...
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...)
Removed the adding of the feature. Also aemoved the explicit generation of "-mfloat-abi" "hard", in AddSystemZTargetArgs (like X86).
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..?
Comment Actions
Ah, I see. But note that you're now not supporting "use-soft-float" at all (which I think is fine at this step!), so you should update all tests to no longer use "use-soft-float".
Comment Actions
Done. Addded a new test systemz-float-02.c to check that -msoft-float actually works all the way to assembly output. The type v4si using vector_size() gives a different output compared to gcc, so I guess this is implementation defined? See inline comments.
Comment Actions Ok, good.
What's the difference in assembler? Could this be because you've implemented the VectorABI check the wrong way around? (If soft-float, then we have *no* VectorABI!)
Comment Actions
Oh! I misunderstood your previous comment "with -msoft-float, GCC also falls back to the 16-byte vector alignment, so we must match that for ABI compatibility" to mean that (source code) vectors should be aligned to 16 bytes in memory. I also think I saw GCC outputing an NLL to align a store in such a case, but I can't reproduce so I guess I was confused.
Also with the latest updates of the patch, I see for this function: typedef int v4si __attribute__ ((vector_size (16))); v4si foo(v4si *Dst) { return *Dst; } gcc -S -O3 -march=z14 -msoft-float foo: lmg %r4,%r5,0(%r3) stmg %r4,%r5,0(%r2) br %r14 clang -S -O3 -march=z14 -msoft-float foo: # @foo l %r0, 0(%r2) l %r3, 4(%r2) l %r4, 8(%r2) l %r5, 12(%r2) lr %r2, %r0 br %r14 It seems that gcc is loading from %r3, while clang is loading from %r2, and gcc is returning on the stack, while clang seems to be returning in registers.
Comment Actions Yes, that's what I meant. Maybe what's confusing is that on our platform, the "vector ABI" only has an 8-byte alignment for vector types (which is more efficient), while the "old" (non-vector) ABI has a 16-byte alignment (for historical reasons).
That looks completely broken. Note that current clang, when using the non-vector ABI, does generate code that is compatible to what GCC generates, so this must have been broken by this patch somehow. (Possibly the problem is when checking for the vector ABI in TargetInfo.cpp, see inline comment.
Comment Actions Patch updated per review to reset HasVector globally both in clang/lib/CodeGen/TargetInfo.cpp (constructor argument) and clang/lib/Basic/Targets/SystemZ.h (handleTargetFeatures()). This seems to fix the problem with the test case - now the clang output seems to match gcc: foo: # @foo # %bb.0: # %entry mvc 8(8,%r2), 8(%r3) mvc 0(8,%r2), 0(%r3) br %r14 Comment Actions Hello, this breaks tests on all platforms (e.g.: http://45.33.8.238/mac/7170/step_11.txt) Please revert immediately and please run tests locally before committing next time. Comment Actions I have added the missing 'REQUIRES', and -mtriple:s to the tests which was not needed when I tested locally. Sorry for the noise. |
Hmm. In the SoftFloat case, we also should be setting HasVector to false here. Maybe that even explains the incorrect assembler code you're seeing.
(Maybe it would be best after all to reset the HasVector flag globally, then we wouldn't have the problem of forgetting all these cases ...)