This is an archive of the discontinued LLVM Phabricator instance.

DAG: Implement soften float for ffrexp
ClosedPublic

Authored by arsenm on Jul 5 2023, 5:07 PM.

Details

Summary

Fixes #63661

Diff Detail

Event Timeline

arsenm created this revision.Jul 5 2023, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 5:07 PM
arsenm requested review of this revision.Jul 5 2023, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 5:07 PM
alexfh added a comment.Jul 5 2023, 5:53 PM

Thanks for the quick fix! However, I'm not convinced this is a trivial fix for https://github.com/llvm/llvm-project/issues/63661, thus it may introduce other problems and make reverting to a known good state even more complex. As I mentioned on https://reviews.llvm.org/rG003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d#1223579, I'd prefer a revert to green before this fix gets tested and the commits starting from 003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d are recommitted.

Thanks for the quick fix! However, I'm not convinced this is a trivial fix for https://github.com/llvm/llvm-project/issues/63661, thus it may introduce other problems and make reverting to a known good state even more complex. As I mentioned on https://reviews.llvm.org/rG003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d#1223579, I'd prefer a revert to green before this fix gets tested and the commits starting from 003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d are recommitted.

Wanted to note that this fixed boost issue on rv64 btw.

arsenm added a comment.Jul 5 2023, 5:58 PM

Thanks for the quick fix! However, I'm not convinced this is a trivial fix for https://github.com/llvm/llvm-project/issues/63661, thus it may introduce other problems and make reverting to a known good state even more complex. As I mentioned on https://reviews.llvm.org/rG003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d#1223579, I'd prefer a revert to green before this fix gets tested and the commits starting from 003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d are recommitted.

This is a trivial patch extending new functionality to just another case. The revert will be highly disruptive to my downstream uses that already exist and I will not be doing it. There is absolutely no reason to revert 003b58f65bdd5d9c7d0c1b355566c9ef430c0e7d. You could make an argument for reverting b15bf305ca3e9ce63aaef7247d32fb3a75174531, but that's more trouble than it's worth given here is the patch

alexfh added a comment.Jul 5 2023, 6:19 PM

FWIW, the clang crash is gone with this patch. I'm trying to verify the other problem (https://reviews.llvm.org/rGb15bf305ca3e9ce63aaef7247d32fb3a75174531#1223188).

alexfh added a subscriber: bgraur.Jul 5 2023, 6:24 PM

FWIW, the clang crash is gone with this patch. I'm trying to verify the other problem (https://reviews.llvm.org/rGb15bf305ca3e9ce63aaef7247d32fb3a75174531#1223188).

And unfortunately, this patch doesn't fix the problem @bgraur mentioned in https://reviews.llvm.org/rGb15bf305ca3e9ce63aaef7247d32fb3a75174531#1223188.

alexfh accepted this revision.Jul 5 2023, 6:26 PM

LG based on my previous comment and the assertion that this is a "trivial fix".

This revision is now accepted and ready to land.Jul 5 2023, 6:26 PM