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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


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



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


================
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:
> 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)
> ```
> 
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.



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

https://reviews.llvm.org/D72189