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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: clang/lib/Basic/Targets/SystemZ.h:118
         HasVector = true;
+      else if (Feature == "+soft-float")
+        SoftFloat = true;
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/Arch/SystemZ.cpp:38
+  if (ABI == systemz::FloatABI::Invalid)
+    ABI = systemz::FloatABI::Hard;
+
----------------
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 ...


================
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)) {
----------------
Having the override in LLVM itself should be OK, I don't think we need this here.


================
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.");
+
----------------
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.


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


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


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

https://reviews.llvm.org/D72189