Page MenuHomePhabricator

[ValueTracking] Add UndefOrPoison/Poison-only version of relevant functions
Needs ReviewPublic

Authored by aqjune on Tue, Jul 21, 8:39 AM.

Details

Reviewers
nikic
jdoerfert
Summary

This patch adds isGuaranteedNotToBePoison,
getGuaranteedNonUndefOrPoisonOp, and programUndefinedIfUndefOrPoison.

isGuaranteedNotToBePoison will be used at D75808. The latter two functions are
needed for isGuaranteedNotToBePoison.

Diff Detail

Unit TestsFailed

TimeTest
110 mswindows > lld.MachO/invalid::no-filelist.s
Script: -- : 'RUN: at line 2'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-mc.exe -filetype=obj -triple=x86_64-apple-darwin C:\ws\w64\llvm-project\premerge-checks\lld\test\MachO\invalid\no-filelist.s -o C:\ws\w64\llvm-project\premerge-checks\build\tools\lld\test\MachO\invalid\Output\no-filelist.s.tmp.o

Event Timeline

aqjune created this revision.Tue, Jul 21, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 21, 8:39 AM
aqjune marked an inline comment as done.Tue, Jul 21, 8:42 AM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4847–4848

This is a fix to a bug that caused returning false more often. Since it was a small change, embedded here

nikic added inline comments.Tue, Jul 21, 9:36 AM
llvm/include/llvm/Analysis/ValueTracking.h
583

As these functions are the same, I would just rename getGuaranteedNonPoisonOp to getGuaranteedNonUndefOrPoisonOp.

llvm/lib/Analysis/ValueTracking.cpp
4808

If PoisonOnly=true, does this actually end up returning true for UndefValue in all cases? It's not completely obvious to me. Maybe if (isa<UndefValue>(C)) return PoisonOnly;?

4816

As a followup, we may want to recursively check the elements instead.

4847–4848

I'd prefer to land this separately with the corresponding test case, otherwise it gets lost in the rest of the patch.

4855

To make sure I get the logic here: Undef is a bit-wise concept and programUndefinedIfUndefOrPoison() tells us whether we get UB for a full undef operand, while isGuaranteedNotToBeUndefOrPoison() tells us whether there can be any undef bits, so we can apply this only for the i1 case. Is that right?

aqjune marked an inline comment as not done.Tue, Jul 21, 10:30 AM
aqjune added inline comments.
llvm/include/llvm/Analysis/ValueTracking.h
583

I think splitting these two is good (even if their contents are actually the same) because users like PoisonChecking want to only consider poison values. What do you think?

llvm/lib/Analysis/ValueTracking.cpp
4808

You're right, it shouldn't fall through. Will fix this

4816

Sounds good!

4847–4848

Okay, will make a separate patch for this

4855

Yes, exactly.

aqjune updated this revision to Diff 279704.Tue, Jul 21, 9:53 PM

Fix undef case

aqjune marked 2 inline comments as done.Tue, Jul 21, 9:54 PM
aqjune updated this revision to Diff 280706.Sat, Jul 25, 11:55 PM

clang-format

aqjune updated this revision to Diff 281299.Tue, Jul 28, 11:08 AM

Rebase,
Remove getGuaranteedNonUndefOrPoisonOp since getGuaranteedNonUndefOrPoisonOp and getGuaranteedNonPoisonOp are equivalent

aqjune added inline comments.Tue, Jul 28, 11:10 AM
llvm/include/llvm/Analysis/ValueTracking.h
583

Leave getGuaranteedNonPoisonOp only & left a comment at previous getGuaranteedNonUndefOrPoisonOp's uses saying that non-poison op and non-undef op are equivalent

I think I'm fine with this, though it's a lot. @nikic ?

llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp
296

Interesting that we need these.