This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably not poison
ClosedPublic

Authored by craig.topper on Jul 8 2020, 3:41 PM.

Details

Summary

Follow up from the transform being removed in D83360. If X is probably not poison, then the transform is safe.

Still plan to remove or adjust the code from ConstantFolding after this. Also need to audit the partial undef vector handling in InstSimplify just below this code.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 8 2020, 3:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2020, 3:41 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper marked an inline comment as done.Jul 8 2020, 3:46 PM
craig.topper added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4124

Should I be passing CxtI and DT here? I based this off the code in SimplifyInsertElementInst, but maybe i should have based it off of SimplifyFreezeInst?

nlopes added inline comments.Jul 9 2020, 1:13 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4124

Yes, you can; it's correct to do so.
The only issue is that SimplifySelectInst() doesn't seem to receive the select instruction as argument to be passed as CxtI.

Use CxtI and DT if they are passed correctly in the SimplifyQuery

reames accepted this revision.Jul 9 2020, 11:29 AM

LGTM

And thank you for doing this.

This revision is now accepted and ready to land.Jul 9 2020, 11:29 AM
This revision was automatically updated to reflect the committed changes.

I'd erroneously made this comment on the revision (where I think nobody will see it) rather than here, so copying it here:

A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?

A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?

Yes, it may hide some warnings.
But that's life. Optimizations do hide programming errors. This is not the only one for sure. But it's ok as you are checking the code that will run. If the compiler can "fix" bugs automatically, then developers don't need to worry about those. As long as you keep running these checks continuously to track changes in the compiler like this one :)

Replying to my own question, as I was able to test this sooner than I expected: Yes, it looks like the new MSAN warnings remain after this revision. Excellent! I think that proves that this was a useful fix. :)

Yes, it may hide some warnings.
But that's life. Optimizations do hide programming errors. This is not the only one for sure. But it's ok as you are checking the code that will run. If the compiler can "fix" bugs automatically, then developers don't need to worry about those. As long as you keep running these checks continuously to track changes in the compiler like this one :)

Fair point, and thanks for the perspective! :) Even though I seem to have gotten "lucky" (or unlucky?) with this case, I'm sure that point will be relevant sooner or later.