This is an archive of the discontinued LLVM Phabricator instance.

[X86] Use default attributes for intrinsics
ClosedPublic

Authored by nikic on Oct 28 2022, 3:37 AM.

Details

Summary

This adds the default attributes (nocallback, nosync, nofree, willreturn) to some X86 intrinsics. This will be needed to avoid optimization regressions in the future.

Due to the number of intrinsics, this patch focuses just on the IntrNoMem intrinsics up to the AVX2 section.

Diff Detail

Event Timeline

nikic created this revision.Oct 28 2022, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2022, 3:37 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
nikic requested review of this revision.Oct 28 2022, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2022, 3:37 AM

This adds the default attributes (nocallback, nosync, nofree, willreturn) to some X86 intrinsics. This will be needed to avoid optimization regressions in the future.

Can you give us more details on what these optimizations are?

nikic added a comment.Oct 28 2022, 4:02 AM

This adds the default attributes (nocallback, nosync, nofree, willreturn) to some X86 intrinsics. This will be needed to avoid optimization regressions in the future.

Can you give us more details on what these optimizations are?

We currently implicitly assume that all readnone intrinsics are also willreturn. Once we stop doing that, any intrinsics not marked as willreturn will no longer be subject to DCE.

In principal I think this patch is fine, do we have any thorough documentation on these attribute kinds? I just want to ensure we're not going to have a problem with a few cases (e.g. the round intrinsics which can be dependent on MSRs etc.)

nikic added a comment.Oct 28 2022, 5:05 AM

In principal I think this patch is fine, do we have any thorough documentation on these attribute kinds?

These attributes are documented at https://llvm.org/docs/LangRef.html#function-attributes, with the exception of nocallback, which only has a pending patch at https://reviews.llvm.org/D131628. But I don't think callbacks are relevant for any of these intrinsics.

I just want to ensure we're not going to have a problem with a few cases (e.g. the round intrinsics which can be dependent on MSRs etc.)

This shouldn't be a problem for the newly added attributes, but if you actually want to model that dependence, then the existing intrinsic definitions for these are incorrect (they cannot be IntrNoMem and need to read InaccessibleMemOnly instead, and intrinsics that modify MSRs need to write InaccessibleMemOnly, basically).

RKSimon accepted this revision.Oct 28 2022, 5:29 AM

LGTM - thanks - the round dependency shouldn't be a problem - our isel patterns should have the relevant MXCSR use predicates

This revision is now accepted and ready to land.Oct 28 2022, 5:29 AM

I just want to ensure we're not going to have a problem with a few cases (e.g. the round intrinsics which can be dependent on MSRs etc.)

It seems we have already using IntrInaccessibleMemOnly + IntrWillReturn for constrained intrinsics https://github.com/llvm/llvm-project/blame/main/llvm/include/llvm/IR/Intrinsics.td#L764
For other X86 instrinsics, it should be not a problem for those embedded rounding control ones. But maybe we need to take care of a few legacy rounding intrinsics, e.g., https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/IntrinsicsX86.td#L680-L691

@nikic IIUC, intrinsics without IntrNoMem don't need such change, right?

Second thought, almost all the X86 FP intrincics that may affected by MXCSR (including the embedded ones but the rounding argument is _MM_FROUND_CUR_DIRECTION) have the same problem. It is a reason why we have to define constrained intrinsics.
However, we don't have constrained target specific intrincics. So this should be a problem for user to use intrinsics in strict FP mode. +@andrew.w.kaylor for acknowledge.

pengfei accepted this revision.Oct 28 2022, 6:38 AM

Third thought, we may need noreturn for those intrinsics that may raise exceptions under maytrap or strict cases. But I think we don't have a good way to switch the attributes with compiler options. Anyway, it is an irrelevant problem. And this patch LGTM.

nikic added a comment.Oct 28 2022, 7:17 AM

Third thought, we may need noreturn for those intrinsics that may raise exceptions under maytrap or strict cases. But I think we don't have a good way to switch the attributes with compiler options. Anyway, it is an irrelevant problem. And this patch LGTM.

Good point. The fact that the constrained FP intrinsics are currently marked as IntrWillReturn does seem incorrect to me. This can probably lead to miscompiles if a guaranteed-to-trap constrained FP intrinsic is followed by undefined behavior for example -- in this case, the undefined behavior will be propagated backwards past the trapping instruction. Unless I misunderstood something about the semantics of these intrinsics, we shouldn't be marking them as willreturn, and instead infer willreturn on call-sites for fpexcept.ignore (and maybe fpexcept.maytrap, I'm not clear on the precise constraints there.) Or as an alternative to inferring, handle them specially in Instruction::willReturn().

@nikic IIUC, intrinsics without IntrNoMem don't need such change, right?

IntrReadOnly also needs to be adjusted to enable DCE. Other intrinsics can't be DCEd anyway, but can still benefit in other ways. E.g. lack of willreturn means that any "guaranteed-to-execute" optimizations will stop at the intrinsic, and lack of "nocallback" means that GlobalsAA will make pessimistic assumptions about access to internal globals. So it's best to use default attributes wherever possible, but IntrNoMem/IntrReadOnly are the most important ones.

This revision was automatically updated to reflect the committed changes.