Page MenuHomePhabricator

Add constrained intrinsics for some libm-equivalent operations
ClosedPublic

Authored by andrew.w.kaylor on Apr 20 2017, 3:54 PM.

Details

Summary

This patch adds constrained versions of some libm-equivalent floating point operations for which non-constrained intrinsics currently exist.

The purpose of the constrained intrinsics is to force the optimizer to respect the restrictions that will be necessary to support things like the STDC FENV_ACCESS ON pragma without interfering with optimizations when these restrictions are not needed. This is a follow-up to D27028.

There are additional libm-equivalent operations which may need to be handled in a future patch. In addition, it will be necessary to update the direct manipulation of libm function calls to prevent unwanted optimizations when FP environment access is enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.
docs/LangRef.rst
12927

Probably doesn't matter to the output, but why are there two spaces after metadata on the second line?

include/llvm/CodeGen/ISDOpcodes.h
267

Should this say "functions" rather than "operators"?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6550

LLVM style is to have "case" aligned with "switch"

6551

Use llvm_unreachable instead of assert(false). No return required if you use that.

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
950–951

Why not just merge this function call into the 'if'?

lib/IR/IntrinsicInst.cpp
149

This unreachable shouldn't be necessary with the default in the switch.

lib/IR/Verifier.cpp
4352

Don't unary intrinsics only have 3 operands and binary have 4? Is there a hidden one I'm missing?

docs/LangRef.rst
12927

Editing error?

include/llvm/CodeGen/ISDOpcodes.h
267

It probably should say 'intrinsics'

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
950–951

The original intention was that this value would be used again after the Select() call to decide when we needed to do more work to a selected node. I suspect that's not going to happen here after all so I can probably get rid of this value and the comment below the Select().

lib/IR/IntrinsicInst.cpp
149

I agree, but I have a vague memory that I was getting a compiler warning here. I'll look into that.

lib/IR/Verifier.cpp
4352

Yeah, I should be using getNumArgOperands() here instead. The extra operand is the function.

andrew.w.kaylor marked 6 inline comments as done.

Addressed review comments
Updated to latest source base

Fixed LangRef spacing issues in more places

craig.topper added inline comments.May 24 2017, 2:34 PM
docs/LangRef.rst
13313

environment is misspelled.

13353

environment is misspelled

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6551

Lowercase mutate to match the function name?

Addressed review feedback

This revision is now accepted and ready to land.May 25 2017, 10:09 AM
This revision was automatically updated to reflect the committed changes.