Do you have any comments on the regbankselect part?

Hi Krzysztof,

FWIW, LGTM now.

Hi Krzysztof,

Hi Reid,

Hi Konstantin,

The %1 assignment is dead but not marked as such. I'm not 100% sure whether it is allowed to omit dead flags.

Any chance you could add a test case?

Hi Geoff,

FYI, I've revert the original commit in r325421.

Hi Aditya,

Thanks all for the great feedbacks!

Hi Amara,

(though it may require the interface to be changed slightly since we'll need the opcode)

Hi Geoff,

BTW, while writing the RFC, I realized that we could potentially generated incorrect code if we were silently downgrade a post-r317488 bitcode with a pre-r317488 compiler. (I.e., running fast math optimizations whereas we only wanted reassoc)

That's http://lists.llvm.org/pipermail/llvm-dev/2018-February/121114.html

I think we have to create a new version of the IR since the bits changed meaning (we can't just flip 'on' new bits).

Hi Sanjay,

For the review itself, I leave this to @aprantl. I was just providing insights on the shrink-wrapping question :).

LGTM

Hi Roman.

LGTM

LGTM

One question below.

LGTM, coordinate with the x86 changes and you should be good to go!

While I was here, I notice that selectCopy was not using getRegClassForTypeOnBank. Could you double check this is intended?

LGTM with the nitpicks mentioned by Krzysztof.

LGTM

Just two remarks:

• So far shrink-wrapping was solely an analysis pass, i.e., it didn't modify the input code
• Ideally I'd like we postpone creating new blocks until we decided where we are going to insert the code, otherwise we'll end up with blocks to clean-up later on

Ah makes sense.

Let's leave the patch as is.

Alright, LGTM then.
Please also patches the two other places I mentioned assuming you can have a test case for them.

Got a chat with Roman off-line and turns out the problem I mentioned is worked around by everything single target, so indeed no test cases!
LGTM then

Couple of nits on the test case.

Basically, I am fine either way (with or without the cast), but we would need to be consistent.

Out-of-curiosity do you actually see codegen differences with that patch?

LGTM

LGTM with couple of nits below.

LGTM

Hi Carlos,

LGTM too. The code is probably simpler to understand like this and if compile-time turns out to be a problem we can revisit.
I wonder though if removing this copies upfront wouldn't have helped tablegen current matching table (because it does not look through COPYs right now).

Sounds like a good thing to do.

In D41876#972584, @volkan wrote:

In D41876#972547, @bogner wrote:

In D41876#971616, @volkan wrote:

In D41876#971552, @qcolombet wrote:

Could this be extended to reduce any isel related bug?

Yes, it can be extended to reduce the other ISel problems by adding a regex and the specific arguments for llc.

I'm guessing the answer is no since you'd have to update the regex as well, but would it make sense to allow llc arguments to be passed through from the command line or something like that?

I thought we could have a regex, argument list pair for each error type.

But that does not imply that there is no correspondence to user code before the frame setup is complete.

Should I submit autogenerated tests that are not FP/G_COPY/G_TRUNC related, but that still need to be updated for D37775 as a separate patch? Should I have a review for it or can I just submit?

Hi Alexander,

Mostly looks good to me.
We probably want to make two separate patches:

• One with the pattern matching capabilities
• One with the new pass

Hi David,

Could you upload the patch with the full context?

Hi Geoff,

LGTM

Looks mostly good to me.

I don't expect you have a test case that exposed the problem, right?
(In particular for Cyclone at least WriteAdr is the same as WriteI).

Hi Diego,

Hi Marina,

Hi Volkan,

How about checking for EnableGlobalISelAbort.getNumOccurences() > 0 to check for an command line override, otherwise doing what the target wants via the isGlobalISelAbortEnabled() override?

Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.

One more comment: Do you have an idea how we could make sure that the combiners are making progress?
Basically, I would like we think about two cases that bit us in the past:

1. Two combines undo each others
2. Two combines keep piling up (like expand op1 into op2 & op3 and expand op2 into op1 & op3 would keep having the IR growing)

Hi Aditya,

Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.

Hi Amara,

Hi Philip,

LGTM

LGTM