Page MenuHomePhabricator

[SLP] Don't allow Div/Rem as alternate opcodes
ClosedPublic

Authored by a.elovikov on Jan 14 2020, 3:08 PM.

Details

Summary

We don't have control/verify what will be the RHS of the division, so it might
happen to be zero, causing UB.

Diff Detail

Event Timeline

a.elovikov created this revision.Jan 14 2020, 3:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 3:08 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
ABataev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
427–429

Do you really need it here? It does not check anything for casts.

vporpo added a subscriber: vporpo.Jan 14 2020, 3:40 PM
vporpo added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
414–415

Does this cover the case where VL[0] is invalid? Maybe add a test for it?

a.elovikov marked an inline comment as done.Jan 14 2020, 3:44 PM
a.elovikov added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
427–429

If that helper routine was checking for casts, I would have placed the call to it into the condition on line 426, similar to the check on line 414. But since it doesn't, there is no reason to do that there. On the other hand, if later some cast would become invalid, that condition on line 426 must be changed and the assert states exactly that.

In other words, this assert verifies exactly that - the check does not list casts as invalid.

a.elovikov marked an inline comment as done.Jan 14 2020, 3:45 PM
a.elovikov added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
414–415

I believe the second function in the LIT test verifies that - I have urem in the index 0 there.

ABataev added inline comments.Jan 14 2020, 3:58 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
386

What about float point ops?

a.elovikov marked an inline comment as done.Jan 14 2020, 4:18 PM
a.elovikov added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
386

FP subclasses of llvm::Instruction rely on default floating point environment, so shouldn't throw. If FP operation can produce a signaling NaN it should be represented as constrained intrinsic (via CallInst), which I doubt is handled in this file and should probably bailout for other reasons earlier. At least, I don't see calls to TLI->isFunctionVectorizable in this file.

As such, I'm not aware of any existing issues in this regard. I'll be happy to update the code if you can point me which ones aren't valid and why.

ABataev added inline comments.Jan 14 2020, 4:26 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
386

I was just asking if you're sure about it - fine.

This (and D72739) should be in 10.0 - please can you raise a bug and block https://bugs.llvm.org/show_bug.cgi?id=44555

Ping. I don't see any comments that require action on my side.

This revision is now accepted and ready to land.Jan 21 2020, 9:33 AM
This revision was automatically updated to reflect the committed changes.