In D47925#1131851, @gipri wrote:

Great! Sorry but I do not have merge access, can you land this for me? Thanks!

lgtm

Thanks for the patch!

I'm adding Craig Topper as a reviewer because he knows the vector selection DAG stuff better than I do. The constrained FP handling looks OK to me.

The reason that I originally implemented this the way I did, mutating the strict nodes to their non-constrained equivalents, was that I thought it would require too much duplication in the .td files to implement all the pattern matching for the strict nodes. The original plan was to find some other way to communicate the "strict" state to the target after instruction selection, but I never found a way to do that.

At some point we should create a document that describes the entire flow of FP instructions through the instruction selection process. To be honest I don't remember how it all works, and that makes it difficult to review changes like this. It would also be nice to verify that we all have the same understanding of how it works. I don't mean to volunteer you to produce the entire document, but would you mind giving me a rough outline? I'm still concerned about the case that is not chained.

lgtm

Thank you for your patience in addressing my concerns.

I'm OK with this.

In D44817#1046470, @dberlin wrote:

Sigh.
(a quick scan of things named Map in LLVM doesn't find any other obvious cases of this).

I wonder if we shouldn't have a debug/expensive checks mode where it moves all the memory on find and construct to make all these situations fail obviously and instantly so it could be found by bots.

As long as you're OK with the fact that this will probably be replaced in the near future (and please add a comment indicating that above the setOptPassGate function), I'm OK with this being added. I do prefer OptPassGate as a name for the generalized functionality.

In D44464#1041373, @yrouban wrote:

In D44464#1039091, @andrew.w.kaylor wrote:

I would prefer not to overload the behavior of OptBisect. ...

Andrew, would it be ok to create an NFC extracting a pure virtual class OptPassGate from OptBisect? Then I could make use of the OptPassGate interface to implement context specific pass gates without referring to OptBisect.

In D44464#1037846, @fedor.sergeev wrote:

I would like to stress that we consider OptBisect to be more than just "bisector", but rather the only
available way of skipping optimization passes with an arbitrarily complex control pattern.

I have some concerns about this patch, mostly centered around what the intended use cases are.

Thanks for taking the initiative on this!

In D40524#969410, @RKSimon wrote:

Cheers - did you create the bug report in the end? I can't seem to find it.

Looks good!

BTW, thank you very much for this patch!

lgtm

After thinking through this particular situation a bit more with regard to the STRICT_EXP node, I think what you've chosen to do here is probably correct. I'm not entirely certain what the exp_finite implementation does, but I would expect that with regard to rounding it will produce the same result as the normal function as long as the input is finite. Similarly, I think that the exception behavior of exp_finite should be the same as the non-finite version as long as the input is finite. If the input is non-finite, then I would expect that the appropriate exception was raised or status flag set by whatever produced the value. I don't think either exp or exp_finite will produce an exception for non-finite values. We'll get the wrong answer with exp_finite of course, but the user signed up for that when using the fast math flags.

In D41338#964520, @spatel wrote:

In D41338#960487, @hfinkel wrote:

In D41338#958611, @efriedma wrote:

Does this make sense for ISD::STRICT_FEXP (the strict version of the node)?

I would guess strict and fast math don't really mix...

I agree.

Sorry for the delay. Let me address this comment first. I would agree too, but I was informed that there's a clang customer in the gaming world that wants to compile with -ffast-math and enable div-by-zero FP exceptions as a way to sanitize their data (at least in development builds).

I assume that we're still a long way from realizing this dream (optimized FP + some subset of FP exceptions enabled) in LLVM, but if there's no correctness issue with allowing this transform, then I think we should treat these nodes the same in this patch.

In D40524#965331, @RKSimon wrote:

@andrew.w.kaylor What is happening with this patch, will you be able to commit it for Jan 3 to fix PR35240? What about the high 8-bit register issue - is there a bugzilla (and repro) for this?

In D40524#940330, @a.elovikov wrote:

I'm not sure if the following is possible/legal but it fails even with this patch:

body:             |
  bb.0:
    successors:
    liveins: %ch, %bl

    %cl = MOV8rr %bl, implicit-def %cx, implicit killed %ch, implicit-def %eflags
    ; CHECK-NOT: MOV32rr
    RETQ %cx

If that's a valid testcase than "a little more checking to do." (isLive before this patch) has to be fixed. If it's invalid - we'd need an assert for that too (or some mir-verify?). However, that should not be a part of this change, probably. Hence, just asking what's your opinion about that testcase.

Death to the curlies!

Updated the test case.

-Refactored getSuperRegDestIfDead() to consolidate the liveness checking logic.
-Added a test case.

In D40524#936926, @MatzeB wrote:

FWIW At a first glance this feels to me as we should have the fixes in isLive() instead as that seems to roughly be "LivePhysRegs::contains() with tweaks" where the LivePhysRegs::contains() would check super registers.

It isn't clear to me how your custom lowering interacts, if at all, with existing table-driven selection patterns. One of the goals in the implementation up to this point has been to have the instruction selection fall back on existing pattern matching as much as possible so that we don't need to duplicate all of the cases that are currently handled. Can you explain to me how this applies in the AMDGPU case?

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