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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/SystemZ.cpp:74
+    Features.push_back("-vector");
+  else
+    if (Arg *A = Args.getLastArg(options::OPT_mvx, options::OPT_mno_vx)) {
----------------
uweigand wrote:
> Having the override in LLVM itself should be OK, I don't think we need this here.
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.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1115
+       (Constraint[1] == 'f' || Constraint[1] == 'v'))))
+    report_fatal_error("Inline asm is using an fp/vector reg with soft-float.");
+
----------------
uweigand wrote:
> 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.
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.


================
Comment at: llvm/lib/Target/SystemZ/SystemZTDC.cpp:313
 bool SystemZTDCPass::runOnFunction(Function &F) {
+  if (F.getFnAttribute("use-soft-float").getValueAsString() == "true")
+    return false;
----------------
uweigand wrote:
> It seems it would be preferable to avoid the duplicate check by using the Subtarget,
> e.g. something like F.getSubtarget().hasSoftFloat().
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.


================
Comment at: llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp:174
+                       ? FSAttr.getValueAsString().str()
+                       : TargetFS;
+
----------------
uweigand wrote:
> 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.
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.



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

https://reviews.llvm.org/D72189