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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

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



================
Comment at: clang/lib/Driver/ToolChains/Arch/SystemZ.cpp:12
 #include "clang/Driver/Options.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"
----------------
Still needed?


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1109
     const TargetRegisterInfo *TRI, StringRef Constraint, MVT VT) const {
+
   if (Constraint.size() == 1) {
----------------
Unneeded whitespace change?


================
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.");
+
----------------
jonpa wrote:
> 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.
Please keep the test and just add the hasVector guard for the {v} constraints.


================
Comment at: llvm/lib/Target/SystemZ/SystemZSubtarget.cpp:39
+  if (HasSoftFloat)
+    HasVector = false;
+
----------------
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.


================
Comment at: llvm/lib/Target/SystemZ/SystemZTDC.cpp:313
 bool SystemZTDCPass::runOnFunction(Function &F) {
+  if (F.getFnAttribute("use-soft-float").getValueAsString() == "true")
+    return false;
----------------
jonpa wrote:
> 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.
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)
```



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

https://reviews.llvm.org/D72189