Thanks!

In D57149#1401627, @mattd wrote:

I do see the point of ensuring the sanity of StackProtector, and I'm not opposed to patching that up (as you have done here in this patch); however, I do see benefit in preventing the dead code from reaching the StackProtector in the first place. I'm not sure how others feel.

In D58200#1397672, @uabelho wrote:

Hi!

Nice to see this fix get in because we've seen the need for such a change for our out-of-tree target as well.

However, we've added a FixedRegisters argument to tryAssign and trySplit too.
It was a while since we did that change locally but would it make sense to push that into trunk too?

Ping!

Nice to see this fix get in because we've seen the need for such a change for our out-of-tree target as well.

Thanks for fixing!

In D57149#1387325, @mattd wrote:

Hi @uabelho

The test case in your patch definitely causes a problem, but that's because the -start-before/-stop-after is used. If I run that same test through llc, the UnreachableBlockElim pass kicks-in and drops the dead code.
I took a look at the code in your Janurary 24th comment. I was unable to get the same dead block to surface, but most likely I had the wrong flag set. How did you build the sample in that comment?

Managed to drop the new testcase when I rebased >.<

Is this ok or is it better to go down the CGP route instead? If so, can we really guarantee that all dead code is always removed when we get to StackProtector so we don't end up with this problem anyway?

Rebased.

In D57221#1384651, @mkazantsev wrote:

Aside from refactoring that we can make as follow-up if needed (yet I don't really think it does), does it look OK? :)

Thanks!

The fix makes sense to me but please let others have an opinion too.

We're having issues with this as well, we don't have any libunwind so this change causes
/usr/bin/ld: cannot find -lunwind

I don't know this code at all.

It solves the problem I saw, thanks!

In D57149#1369777, @rnk wrote:

An alternative way to handle this would be to remove unreachable code. I'm surprised unreachable code reaches this pass in a normal pipeline. I thought CGP removed it in normal operation, even at O0. However, removing unreachable code might invalidate the domtree (unless our utility for that updates it?).

Fixed typo, reformulated a piece of a comment.

Not sure who should review this, I added the three last persons that I think did non-NFC changes to StackProtector.cpp.

Is this a proper way of solving the problem? Is it ok to require DT?

In D57018#1366287, @serge-sans-paille wrote:

@uabelho top-of-tree should be fine now.

Just saw build-bots failed the same way

We've been having issues with std::is_trivially_copyable too, and it seems not even this patch solves the problems for us.

The problem I saw goes away with this patch and I haven't seen any new failures with the limited testing I've done on the patch. Thanks!
I don't know this code though, so I don't know if this is the way the problem should be solved.

Great, thanks!

If I change what I think is two typos, then the patch compiles for me and solves the issue I had.

Hi,
A late question about this change. I notice that this change sometimes gives me additional DIFiles in the clang output compared to before.
E.g. if I have a file

I don't know this code but at least it makes the reproducer I had compile succesfully now. Thanks!

I've verifed that it solves the problem I reported. I've done a little bit of testing with the patch and didn't see anything obviously broken yet at least.

I submitted this for markus since he doesn't have commit access yet.

I've got a post-review comment about the use of SCHED_IDLE vs the needed gcc version.

In D53370#1267950, @efriedma wrote:

LGTM.

It looks like you didn't touch LibCallSimplifier::optimizeLog, but I guess that's not necessary (it probably shouldn't be using emitUnaryFloatFnCall anyway).

Was it something like this you had in mind Eli?

Hi,
A late comment about a problem I've noticed with this change.

We are using the Scalarizer in our out-of-tree target and I've run some tests with the patch without problems so I think it's ok.
Please wait a day before submitting though in case someone who really knows this code objects, but if not I think it's ok to push.

In D52540#1247832, @sheredom wrote:

Should I incorporate the test case from the bugzilla into this commit? Given that its a slightly different failure case that is also fixed by my change, seems like its worth ensuring it doesn't accidentally regress again.

Ping?

In D52540#1247778, @sheredom wrote:

In D52540#1247681, @uabelho wrote:

Is this the same problem as described in https://bugs.llvm.org/show_bug.cgi?id=28911 ?

I wasn't seeing the exact same failure as that bug describes - it was failing in Scalarizer::finish in the test included.

But I did pull the example from the bugzilla and tested it and it no longer crashes with my scalarizer fix.

Is this the same problem as described in https://bugs.llvm.org/show_bug.cgi?id=28911 ?

Alright, I'll commit this as is in a little bit then. Thanks!

In D52410#1243309, @arsenm wrote:

An instruction just needs to def one or the other and the live out would be vcc, since it needs to visit the super register

I would like to create a testcase exposing that we actually fix a bug when we use regsOverlap() instead of MCSubRegIterator, but then I think I need help.

I have a post-review question about this change.

Ping

I'm not sure if I'm allowed to LGTM this but it does look good to me and it solves PR38855 so thumbs up.

Update testcase

This indeed solves the problem I reported in PR38674 but I don't know IndVarSimplify enough to say if this is the right solution or not.

Found the problem when testing my out of tree target, but it's exposed also with the attached testcase.