This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] EarlyCSE support for constrained intrinsics, default FP environment edition
ClosedPublic

Authored by kpn on Apr 6 2021, 8:21 AM.

Details

Summary

EarlyCSE cannot distinguish between floating point instructions and constrained floating point intrinsics that are marked as running in the default FP environment. Said intrinsics are supposed to behave exactly the same as the regular FP instructions. Teach EarlyCSE to handle them in that case.

Diff Detail

Event Timeline

kpn created this revision.Apr 6 2021, 8:21 AM
kpn requested review of this revision.Apr 6 2021, 8:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 8:21 AM

early returns?

llvm/lib/IR/IntrinsicInst.cpp
201

return false;

207

return false;

kpn updated this revision to Diff 335647.Apr 6 2021, 1:25 PM

Address review comments: use early return.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp
120

Should doesNotAccessMemory be returning true for these? Then the existing code would pick them up.

1072

I'm confused why you need this, I think the caller is already checking it (see comment at callsite).

1217

Isn't this the only call to handleBranchCondition? It's already guarded by canHandle(CondInst).

kpn added inline comments.May 18 2021, 11:44 AM
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
120

There are a lot of constrained intrinsics, but I wanted to be certain that I'm restricting the handling here to just these. I worry that if 'readonly' is set somewhere else then it could get changed in the future without anyone realizing that the behavior here would change. Thus the explicit list here.

Also, depending on the exception handling metadata argument, these constrained intrinsics may cause traps and they may change the FP status bits/register. In the default environment we assume no traps, and we assume nobody is looking at the FP status bits. But the default environment is communicated via the metadata argument.

The metadata arguments should work correctly without extra attributes like 'readonly' needed. And I can't swear the argument won't be changed to be stricter without touching the attributes. That would result in a miscompile, and it seems safer to just rely on the metadata argument and call it a day.

1072

I wouldn't have done it without a reason, but I can't find a repro now. It might be related to further work and thus should be in a later ticket. I'll remove it.

kpn updated this revision to Diff 346233.May 18 2021, 11:45 AM

Update for review comments.

JosephTremoulet accepted this revision.May 19 2021, 6:58 AM

LGTM assuming you clang-format the patch.

This revision is now accepted and ready to land.May 19 2021, 6:58 AM
This revision was landed with ongoing or failed builds.May 20 2021, 11:41 AM
This revision was automatically updated to reflect the committed changes.