Page MenuHomePhabricator

[ARM] Add custom strict fp conversion lowering when non-strict is custom
ClosedPublic

Authored by john.brawn on Dec 6 2019, 9:21 AM.

Details

Summary

We have custom lowering for operations converting to/from floating-point types when we don't have hardware support for those types, and this doesn't interact well with the target-independent legalization of the strict versions of these operations. Fix this by adding similar custom lowering of the strict versions.

This fixes the last of the assertion failures in the CodeGen/ARM/fp-intrinsics test, with the remaining failures due to poor instruction selection.

Diff Detail

Event Timeline

john.brawn created this revision.Dec 6 2019, 9:21 AM

Looks sensible.

Is it worth un-xfailing the test, now that it no-longer crashes? Even if it's check the poor codegen (maybe with some FIXME's), we can show it improving over time. I would use that update script, but I'm not sure how verbose that test will be with it.

llvm/lib/Target/ARM/ARMISelLowering.cpp
5404

Maybe just if (IsStrict) return Tmp; else return Tmp.first; (or something like it), if that works.

16342–16343

Can we use std::tie(SrcVal, Chain) = makeLibCall(..) ?

john.brawn marked 2 inline comments as done.Dec 11 2019, 9:42 AM

Looks sensible.

Is it worth un-xfailing the test, now that it no-longer crashes? Even if it's check the poor codegen (maybe with some FIXME's), we can show it improving over time. I would use that update script, but I'm not sure how verbose that test will be with it.

Un-xfailing it isn't trivial as codegen is poor in an inconsistent manner, e.g. fptosi_f32 gets poor codegen when you have single-precision+double-precision but good codegen when you have single-precision only (despite it having nothing to do with double-precision) due to some weirdness in type legalization.

I could make the test more complicated to handle this, but I'd rather not.

llvm/lib/Target/ARM/ARMISelLowering.cpp
5404

Unfortunately not (Tmp is a pair of SDValue), though doing a similar std::tie thing makes it more explicit what's happening so I'll do that.

16342–16343

Yes, I'll do that.

Update according to comments.

dmgreen accepted this revision.Dec 13 2019, 12:57 AM

Thanks

I would prefer if there were some tests in here. I looked at that file though, just running the update script on it. There are a lot of pushes and pops and softfloat movs and conflicting asms and whatnot. Perhaps you are right that is makes it more complex than would be good for it. (But.. I always dislike tests that just say ; CHECK-SP: vcvt.s32.f32, it is so easy to miss poor codegen. There could be a thousand instructions around that vcvt!)

As we know the test will be enabled later, and at this point I have certainly ran it enough to know this works, LGTM. If you want to improve the test (now or later), that's great. If you don't want to, I'm not going to be stubborn about it.

This revision is now accepted and ready to land.Dec 13 2019, 12:57 AM
This revision was automatically updated to reflect the committed changes.