[llvm-dev] RFC: Killing undef and spreading poison

[Nuno Lopes via llvm-dev]

>> Right, a load touching a single uninitialized bit results in poison.
>> The trick is that on the first bitfield write, all the remaining 
>> untouched fields become initialized (with an arbitrary value, though). 
>> Essentially we are making the adjacent bitfields undef.
>>
>> So if you have:
>> struct foo a;
>> a.x = 2;
>> a.y = 3;
>>
>> IR becomes:
>> %a = alloca foo
>> %x = load %a
>> %x2 = freeze %v  ; %x2 not poison
>> %x3 = bitmasking  ; %x3 not poison
>> store %a, %x3
>>
>> %y = load %a
>> %y2 = freeze %y  ;  not needed; %y is not poison
>> etc..
>
> Oh, okay, that works.  I should have thought that through a bit more 
> carefully.

I must say I got scared about bitfields a few weeks ago as well :)


> Sort of related testcase:
>
> struct S { int x : 24; };
> S s = { 2 };
>
> Gives:
>
> @s = local_unnamed_addr global { i8, i8, i8, i8 } { i8 2, i8 0, i8 0, i8 
> undef }, align 4
>
> When undef goes away, clang will need to be patched to generate zero here? 
> More generally, will struct padding in globals be poison, or some 
> arbitrary value?

Padding can safely become poison.


> Instcombine currently transforms "memcpy(dst, src, 4)" to "load i32 src; 
> store i32 dst".  I guess that's illegal under this model?How about the 
> related transform "memcpy(dst, src, 4)" to "load <4 x i8> src; store <4 x 
> i8> dst"?  (SROA also performs similar transforms.)

The first is illegal, you're right (unless you know that they aren't poison, 
of course).  The second version with vectors is correct.
The problem is that codegen for vector loads/stores doesn't seem optimal 
ATM. We've seen really bad cases (e.g., http://llvm.org/PR30615)


> Have you given any thought to how auto-upgrade for bitcode files would 
> work?  I guess in general, you can upgrade an old "load i32" to "bitcast 
> (freeze (load <4 x i8>)) to i32", but that's really awkward.

Ah, good point, I forgot about that.  I was assuming auto-upgrade only 
needed to change undef into "freeze poison", but you're right that load 
semantics are also changing and would need patching as well.
It actually needs to be "load <32 x i1>" because undef is per bit and one 
bit being poison cannot taint the whole byte.
Of course we could have a few optimizations for common patterns such as 
(load (alloca)) -> (freeze (load (alloca)), but ATM I don't see any way 
around the pattern you suggest.

Thanks,
Nuno