This was committed as r313784. I put the wrong differential revision number in the comment for that check-in.

This is breaking buildbots for 32-bit targets because the offset in 'nullptr + int8_t_N' is being implicitly cast to an int. That makes the sizeof(offset) == sizeof(ptr) check turn out differently than my tests were assuming.

-Changed GNU idiom from extension to warning.
-Updated to ToT.

Does anything else need to be done for this to be ready to land?

Ping.

Fixed value-dependent argument in isNullPointerConstant checks.
Added check for C++ zero offset in subtraction.
Added value-dependent test cases.

Refactored the GNU idiom check to be shared by Sema and CodeGen.
Refined the checks so that nullptr+0 doesn't warn in C++.
Added the zero offset qualification in the warning produced with C++.

Added warnings for null pointer arithmetic.

In D37042#850676, @hfinkel wrote:

I'd really like to see this done in some way such that we can issue a warning along with generating well-defined code. The warning can specifically state that the code is using an extension, etc.

LGTM

My intention here was to make this as strict/limited as possible while still handling the cases of interest. There are two different implementations I want to handle. You can see the first variation in the __BPTR_ALIGN definition being added here:

Can you revert the white space changes in the places you aren't otherwise modifying? In general, you shouldn't make formatting changes outside of the parts of the file your patch is modifying. It complicates the version control blame process without adding a lot of benefit.

I'm not sure why the svn attributes got attached to the file I added. I'll remove them before committing.

Can you put this off until the patch that Craig submitted in D36983 either lands or gets rejected? If that change goes through, you should be able to remove your modifications to X86ISelDAGToDAG.cpp.

In D36335#846018, @craig.topper wrote:

Do we want to give the target any chance to use FMSUB/FNMADD/FNSUB if any of the arguments are negated?

Could you also add a use of this new intrinsic to llvm/test/Verifier/fp-intrinsics.ll?

-Addressed review feedback
-Rebased

Addressed initial review feedback

Oops. I didn't mean to accept the revision yet.

It looks like a number of passes have this as a required analysis pass, and some assert that loops are in the simplified form. Can you verify that all passes which depend on this pass can also be skipped by opt-bisect?

Re-wrote the patch to move the isNoBuiltin checks into canConstantFoldCallTo and ConstantFoldCall.

Thanks for doing this!

In D33737#769328, @efriedma wrote:

The other alternative is that you could pass in an AttributeList (the result of getAttributes()) to the functions... that makes it more clear what exactly is getting used, but it's more complicated (and probably involves a bunch of refactoring to expose a function to check isNoBuiltin given a Function/AttributeList pair).

But either way, I would prefer a solution that involves checking for isNoBuiltin() in as few places as possible.

In D33737#769217, @efriedma wrote:

Would it be possible to pass in a CallSite to canConstantFoldCallTo, ConstantFoldCall, and SimplifyCall, to reduce the number of places we check this?

Addressed review feedback

Fixed LangRef spacing issues in more places

Addressed review comments
Updated to latest source base

lgtm

A couple of things came up as I was preparing this for commit. They're pretty straightforward issues, so I'll just fix them in my sandbox before committing.

lgtm

Still approved.

Looks good to me.

Looks good to me

Can you add tests for these changes? I think the attribute.ll and no-proto.ll tests in test/Transforms/InferFunctionAttrs would be the right place to add these. I'm not sure if the existing tests check all of the LibFunc calls or just a representative sample.

Ping.

I'm happy with this. Thanks for the improvements!

It looks like I need to rethink this.

OK, so maybe that puts me back in the position of needing to find a way to conditionally add the MXCSR use/def information only when the strict semantics are required, in which case there would be no significant advantage to splitting the register as I'm proposing here.

In D30661#693861, @efriedma wrote:

It's not just DCE which is problematic... we could also sink a floating-point operation past a read from the status register. I suppose you could prevent that particular problem by making reads from the status register write to the control bits, but that causes its own problems.

In D30661#693773, @efriedma wrote:

A subsequent patch will update floating point operations to add an implicit use of the control bits and an implicit def of the status bits

This seems kind of confusing... strict floating-point ops need to implicitly use and def the status bits, because the new value depends on the previous value. You can think of an FP operation as a logical OR acting on the status register. Many kinds of code motion are legal (e.g. you can reorder FP operations with each other, or hoist them out of loops). But if you omit the use, other optimizations won't work correctly; for example, dead code elimination will eliminate FP operations which have a visible effect on the status register.

Given that, I'm not sure what splitting the status register buys you; I guess it becomes easier to check whether an instruction modifies the control bits?

In D30662#693559, @ahatanak wrote:

Is it possible to add a test case (possibly in CodeGen)?

Is there anything left blocking this patch?

-Consolidated examination of StrictFP node opcodes.

-Combined uses of string literals for FP rounding mode and exception behavior.
-Removed extension to StringSwitch since it is no longer needed.
-Added documentation explaining that the rounding mode argument isn't used from the frem intrinsic.

In D28363#637147, @mkuper wrote:

(Out of curiosity - you mean r290726 actually has an observable effect on compile time?)

In D27028#635372, @DavidKreitzer wrote:

When frem has a meaningful result, it is always exact, so perhaps we should omit the rounding behavior argument? I think we still need a constrained frem intrinsic, though, to handle the exceptional behavior in cases such as "frem inf, x" and "frem x, 0".

In D27028#634214, @b-sumner wrote:

FWIW, rounding controls are needed for llvm.fma.*, llvm.fmuladd.*, and llvm.sqrt.*

Ping.

It looks to me like you are probably correct about the intended mailing list post, but it's still not particularly helpful. It would be nice to have a reference to a primary source, but this is the best I could find:

I have one nitpick about a typo in a comment, but otherwise this looks good to me.

Ping

In D25848#624177, @davide wrote:

I consider this reasonable, but I don't feel qualified enough to review as I'm not a LICM expert, so I'm CC:ing @danielcdh . I'm slightly worried about other passes hitting a similar issue (expecting some state to be freed and asserting in doFinalization())

What do you think of this change instead?

Index: lib/Transforms/Scalar/LICM.cpp
===================================================================
--- lib/Transforms/Scalar/LICM.cpp	(revision 289480)
+++ lib/Transforms/Scalar/LICM.cpp	(working copy)
@@ -124,8 +124,13 @@
   }

Sorry this is moving so slowly. I just reproduced the problem with the test file from your last comment. I'll take a closer look and see if I can understand what's going on.

I moved the isEHPad() check, as suggested.