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.

In D27028#614098, @arsenm wrote:

Correct handling of ftz is required for correctness, so it isn't appropriate to be an optimization hint placed with the fast math flags

Fixed typos and style issues.
Added FIXME comments as requested.

In D27028#613949, @mehdi_amini wrote:

In D27028#613926, @andrew.w.kaylor wrote:

In D27028#613867, @mehdi_amini wrote:

Having the llvm_unreachable right after the StringSwitch should achieve the same thing.

I'm not sure I understand what you're suggesting here. If I do this:

return StringSwitch<RoundingMode>(RoundingArg)
  .Case("round.dynamic",    rmDynamic)
  .Case("round.tonearest",  rmToNearest)
  .Case("round.downward",   rmDownward)
  .Case("round.upward",     rmUpward)
  .Case("round.towardzero", rmTowardZero);
llvm_unreachable("Unexpected rounding mode argument in FP intrinsic!");

the llvm_unreachable statement is purely unreachable because the implicit default from StringSwitch will assert or dereference a null pointer. The llvm_unreachable in this case effectively becomes a comment.

Not really: in an optimized build it means that the default case is unreachable. The assertion does not exist there, and the optimizer can drop the nullptr dereference.

In D27028#613867, @mehdi_amini wrote:

Having the llvm_unreachable right after the StringSwitch should achieve the same thing.

I see. I still think it's worth adding UnreachableDefault() instead of just a comment explaining that other values will assert/crash. I'm checking the legal values in the verfifier, so this shouldn't be an issue.

I re-wrote the ISel code to introduce a pseudo-instruction for the strict variants of FP operations, which is mutated directly to a normal FP node just before instruction selection. I removed the SDNodeFlag extensions I had added in my original patch because they aren't needed in this implementation. Some variant of that code will likely need to be re-introduced at some point, particularly to handle FTZ rounding modes.

In D27028#604699, @arsenm wrote:

It's not handled in general llvm. We currently have a subtarget feature for f32/f64 denormal support in the default rounding mode

Is flush-to-zero currently handled with function attributes?

In D27028#604603, @hfinkel wrote:

In D27028#604594, @arsenm wrote:

As I mentioned at the meeting I think these need a way to control whether denormals are flushed or not

You mean that there should be a way to control this on a per-operation basis, or there should be some way to represent that the user might be changing some thread state that controls how this is done?

In D27028#604172, @hfinkel wrote:

Based on our conversation at the dev meeting, here's how I thought this would work:

1. Introduce target-independent chain-carrying nodes to represent these operations. For argument's sake, STRICT_FADD, etc.
2. Since nothing in the SDAG knows about what these nodes do, there's no problem with optimizations doing bad things.
3. You'd do something minimal in SelectionDAGISel::DoInstructionSelection() around the call to:
   
   Select(Node);
   
   so that it would become:
   
   bool IsStrictFPOp = isStrictFPOp(Node); if (IsStrictFPOp) mutateStrictFPToFP(Node); // STRICT_FADD -> FADD, etc.
   
   Select(Node);
   
   if (IsStrictFPOp && !TLI->addStrictFPRegDeps(Node)) report_fatal_error("Could not add strict FP reg deps");
   
   and then you'd be done. Obviously this is somewhat hand wavy, but if there are complexities I'm overlooking, I'd like to understand them.

Removed IntrInaccessibleMemOnly from llvm.assume.

Ping.

I added the IntrInaccessibleMemOrArgMemOnly property, and updated the llvm.assume intrinsic to use IntrInaccessibleMemOnly.

In D26382#589550, @hfinkel wrote:

This will work, but we might want a separate tracking state for this (because otherwise we won't be able to infer readonly for any functions with FP operations, which is going to be very unfortunate). We might do this as a later enhancement, however.

I'm trying to understand where the data structured entry gets removed when the pass is not skipped. I tried downloading the reproducer from the PR you mentioned, but I can't seem to get this to happen with that file and the command line you listed above. I have seen this problem before, but I'm having trouble making it happen now.

The problem with adding more run lines is that if I understand this failure correctly, the test would fail if any of the run targets was missing from the build configuration. Long term, I think it would be nice if LIT had a way to selectively disable individual run lines based on the build configuration.

It looks like the explicit triples are being used in the tests in the non-arch-specific directory simply to force the tests to be run for both x86-64 and i386 targets. This is great for x86, but it means that these tests aren't run at all against other targets, even when another target is the default for the build. That seems bad.

I apologize for the delay. I forgot about this patch. I'll get it committed today.

In D14996#483565, @MatzeB wrote:

Any news on this?