Page MenuHomePhabricator

[SystemZ] Support -msoft-float
ClosedPublic

Authored by jonpa on Jan 3 2020, 3:47 PM.

Details

Summary

First attempt of reusing code from other targets to recognize -msoft-float on SystemZ.

Seems to work, and tests are passing.

Diff Detail

Event Timeline

jonpa created this revision.Jan 3 2020, 3:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2020, 3:47 PM
jonpa updated this revision to Diff 236196.Jan 4 2020, 12:04 PM

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?

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.

jonpa updated this revision to Diff 238345.Jan 15 2020, 1:23 PM

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

uweigand added inline comments.Mon, Jan 27, 5:23 AM
clang/lib/Basic/Targets/SystemZ.h
119

Given that you no longer provide the +soft-float target feature, this check is never true, right?

On the other hand, this is really now only used to define the SOFT_FLOAT predefined macro, which we probably shouldn't have anyway, given that GCC does not provide that predefine either.

So I think all this can just go away.

clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
37

What's the point of the Invalid setting now? Just set the variable to Hard initially, and if there's no option, that's where it will stay ...

73

Having the override in LLVM itself should be OK, I don't think we need this here.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1115

I don't think a fatal error is the correct action here. We should simply not return an unsupported regclass. Just like we already don't return a regclass for "v" unless Subtarget.hasVector, we should similarly add a useSoftFloat guard for the "f" constraint.

llvm/lib/Target/SystemZ/SystemZTDC.cpp
313 ↗(On Diff #238345)

It seems it would be preferable to avoid the duplicate check by using the Subtarget,
e.g. something like F.getSubtarget().hasSoftFloat().

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp
179

So this change actually enables another feature: per-function CPU / feature selection.

Now that is of course an interesting feature that we actually want to have as well, but it seems like this really ought to be a separate patch, and in any case there should be test cases to verify that feature.

jonpa updated this revision to Diff 240924.Tue, Jan 28, 10:02 AM
jonpa marked 9 inline comments as done.

Updated per review - see inline comments.

jonpa added inline comments.Tue, Jan 28, 10:05 AM
clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
73

OK, removed, but now the driver produces (clang -msoft-float -mvx):

clang -cc1 ... "-target-feature" "+vector" "-msoft-float" "-mfloat-abi" "soft"

, and the generated function gets these attributes:

attributes #0 = { ... "target-features"="+transactional-execution,+vector,+vector-enhancements-1" ... "use-soft-float"="true" }

You are right however, this doesn't matter - the output is soft float.

Updated soft-float-03.ll test to check that these function attributes works as expected in the backend.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1115

Ah, yes, that's better.

I found out that the check for '{v}' is not wrapped by a guard against hasVector() as 'v' has, but it fails during SelectionDAGBuilder. A separate patch might be to give a better message here to the user in case of compiling without vector support / soft-float.

Updated tests and removed the test for {v}, since it fails with an assert.

llvm/lib/Target/SystemZ/SystemZTDC.cpp
313 ↗(On Diff #238345)

Yeah, right - as I wrote before I didn't find a way to get the Subtarget which was disapointing but still true... This is an IR pass, and I didn't see it being done anywhere.

This would really be preffered, since -mattr=soft-float passed to llc would not be handled as it is.

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp
179

ok, removed it from this patch.

I had to change soft-float-02.ll to use -mattr=soft-float instead of a function attribute after removing this.

174 ↗

	(On Diff #238345)

ok, removed it from this patch.

I had to change soft-float-02.ll to use -mattr=soft-float instead of a function attribute after removing this.

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

clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
12

Still needed?

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1109

Unneeded whitespace change?

1115

Please keep the test and just add the hasVector guard for the {v} constraints.

llvm/lib/Target/SystemZ/SystemZSubtarget.cpp
39

I just realized there is another place where this check is duplicated: UsesVectorABI in SystemZTargetMachine.cpp. This now also needs to check for the soft-float feature: with -msoft-float, GCC also falls back to the 16-byte vector alignment, so we must match that for ABI compatibility. Note that at that point, we only check the global features, not per-function features.

llvm/lib/Target/SystemZ/SystemZTDC.cpp
313 ↗(On Diff #238345)

This is particularly annoying given that this patch now no longer supports "use-soft-float" anywhere else.

I believe you can get at the Subtarget via the TargetTransformInfo, which should be available to IR passes. Look at how other passes do it, something like:

const TargetTransformInfo &TTI =
    getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);

Then you'll have to register that you'll be using the TargetTransformInfoWrapperPass by providing a getAnalysisUsage override, something like:

void getAnalysisUsage(AnalysisUsage &AU) const override {
  AU.addRequired<TargetTransformInfoWrapperPass>();
}

And you may also have to add:

INITIALIZE_PASS_DEPENDENCY(TargetTransformInfoWrapperPass)
jonpa updated this revision to Diff 241568.Thu, Jan 30, 1:29 PM
jonpa marked 5 inline comments as done.

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

Done.
All llc invocations use -mattr=soft-float instead of relying on the function attributes, as must be 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.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1115

Sorry for being unclear: even with the hasVector guard for {v}, it fails with an assert, unlike the case with {f}.

Since this is currently not handled with -vector, I thought this should be a separate patch...?

llvm/lib/Target/SystemZ/SystemZSubtarget.cpp
39

In order to do this, I added back the SoftFloat flag in clang/lib/Basic/Targets/SystemZ.h in order to call resetDataLayout() when it is set. (There is a check in clang/lib/CodeGen/BackendUtil.cpp that indicates that it is necessary to set the v128 alignment also here).

"+soft-float" is pushed to Features in systemz::getSystemZTargetFeatures() so that SoftFloat will be set per above, and so that UsesVectorABI() can check for it and return true if found. Furthermore, this is actually needed I think, since we are not looking at the function attributes anymore.

Added a test for this in clang/test/CodeGen/target-data.c.

llvm/lib/Target/SystemZ/SystemZTDC.cpp
313 ↗(On Diff #238345)

Thanks for helping me out :-)

It seemed to work so far as to get the TTI, but unfortunately it seems I would somehow need the TTIImpl pointer to get to the SystemZ implementation, but it is private and inaccessible. The TTI class hierarchy is very complex, so I chose to instead simply not add the SystemZTDCPass at all in SystemZPassConfig::addIRPasses(), since the TM is there available.

This should work fine as long as the user is passing an option to use soft-float. In other words, this will not work with function attributes, I think.

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

Done.
All llc invocations use -mattr=soft-float instead of relying on the function attributes, as must be done.

Ok, good.

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?

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

clang/lib/Basic/Targets/SystemZ.h
125

Eh, I guess that should be "HasVector && !SoftFloat".

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1176

This should have "&& Subtarget.hasVector()" here. Shouldn't that fix the problem with {v}?

If it does not, I agree this can wait for another patch.

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp
62

No, VectorABI = false then.

Also, should you look for "soft-float" and "-soft-float", just as is done for "vector" above?

201–202

This can be just getTM<SystemZTargetMachine>() instead of the manual cast.

jonpa updated this revision to Diff 241792.Fri, Jan 31, 12:32 PM
jonpa marked 6 inline comments as done.

If soft-float, then we have *no* VectorABI!

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.

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

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.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1176

For some reason, the compilation then aborts with an assert if that constraint has been specified with either -vector or soft-float, so I think it should wait for a separate patch.

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp
62

With the clang driver, it seems that
-mhard-float after -msoft-float works to cancel the first argument.
-mno-soft-float is not recognized here / handled (by any target) (warning: unused)
-mno-hard-float is not even allowed (error)

With the features arguments (-target-feature / -mattr), it however works the same as with the 'vector' feature, so it makes sense to handle it the same way.

If soft-float, then we have *no* VectorABI!

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.

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

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

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.

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.

clang/lib/CodeGen/TargetInfo.cpp
9908

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

jonpa updated this revision to Diff 242204.Mon, Feb 3, 3:04 PM
jonpa marked an inline comment as done.

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
uweigand accepted this revision.Tue, Feb 4, 1:38 AM

LGTM, thanks!

This revision is now accepted and ready to land.Tue, Feb 4, 1:38 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Feb 4, 7:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

jonpa added a comment.Tue, Feb 4, 8:08 AM

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.

I have added the missing 'REQUIRES', and -mtriple:s to the tests which was not needed when I tested locally. Sorry for the noise.