This is an archive of the discontinued LLVM Phabricator instance.

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
12557 ↗(On Diff #96040)

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

include/llvm/CodeGen/ISDOpcodes.h
254 ↗(On Diff #96040)

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

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6533 ↗(On Diff #96040)

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

6534 ↗(On Diff #96040)

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

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
946 ↗(On Diff #96040)

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

lib/IR/IntrinsicInst.cpp
149 ↗(On Diff #96040)

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

lib/IR/Verifier.cpp
4331 ↗(On Diff #96040)

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

docs/LangRef.rst
12557 ↗(On Diff #96040)

Editing error?

include/llvm/CodeGen/ISDOpcodes.h
254 ↗(On Diff #96040)

It probably should say 'intrinsics'

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
946 ↗(On Diff #96040)

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 ↗(On Diff #96040)

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
4331 ↗(On Diff #96040)

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 ↗(On Diff #99810)

environment is misspelled.

13353 ↗(On Diff #99810)

environment is misspelled

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6551 ↗(On Diff #99810)

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.