Differential D35691
csu: test: explicitly add libm as build parameter Authored by alfredo on Jul 1 2022, 3:21 PM. Tags None Referenced Files
Subscribers
Details CSU tests build fails with '/usr/lib/libgcc_s.so: undefined reference to fma' MFC after: 2 days
Diff Detail
Event TimelineComment Actions It's been a while since I tried getting powerpcspe to build, but there's something going on with fma that I don't completely understand. I remember there were many more failures due to missing fmas. It might be better to attempt to stop llvm from emitting fmas altogether? That said, linking libgcc with libm might open up another can of worms. I am not sure if this is the right fix at all. Comment Actions This might be related to https://reviews.llvm.org/D77558 . I haven't added a reduced test case, so it's not been approved and merged. Comment Actions Oh, only for tests? I thought this was for the actual csu itself (crt1.o) which set off giant alarm bells in my head. I would suggest refining the commit log to avoid similar explosions for other people. I probably don't care about the test needing this though I would make it specific to the arch (perhaps using Makefile.powepc64 or the like even?) That said, if Justin's upstream fix is the real fix that would be even better. Comment Actions I agree with john: The tag line is far too scary and is misleading as to what this change does. Please reward the commit message. Other than that: lgtm Comment Actions If libgcc_s.so needs libm now then the change I submitted to LLVM is the right route, but if it's just the test, and libgcc_s.so does not itself need libm, then this is fine. I don't have my beefy hardware yet (still in storage for my move), so can't check myself. Comment Actions As far as I remember, several libs including libgcc now 'suddenly' required (variations of) fma routines, since they were expanded as libcalls. I think the upstream llvm change is much better: if powerpcse doesn't support fma at all, then it shouldn't be emitted by llvm if at all possible. (And a libcall does not really make sense, since it won't give better performance; it will have to go through the motions of calculating it with separate multiplies and adds...) I think what @jhibbits is talking about is upstream llvm only wanting to accept that change if a 'good enough' regression test is added too. Comment Actions Thanks for the reviews! I modified the patch to apply the change only to powerpscpe and changed the description/commit message. Please let me know if you're fine with the patch now. Comment Actions I just updated the review upstream, adding the needed test, so I think it should be good to submit in the next several days. I think that should make this change redundant at that point. Comment Actions Nice, thanks! Comment Actions I think it's fine, if this is the only fix needed to unbreak the powerpcspe build. Just add a note somewhere that it needs revisited and probably reverted after whatever change brings in https://reviews.llvm.org/D77558 (likely a cherry-pick of it specifically, unless a new release is imminent). |