Please avoid using UndefValue::get whenever possible as we are trying to get rid of undef. Please use PoisonValue. Thank you!

In D128501#3613420, @efriedma wrote:

No, you can still link those. There's no ABI change nor any interference at IR level.

The scenario I was thinking of with -ffine-grained-bitfield-accesses is something like the following:

File A:

struct X { int a : 8; int b : 24; };
void f(struct X*);
int g() {
    struct X x;
    x.a = 10;
    f(&x);
    return x.a;
}

File B:

struct X { int a : 8; int b : 24; };
void f(struct X* x) {
    x->b = 10;
}

If both files are compiled -ffine-grained-bitfield-accesses, the fields don't overlap. If both files are compiled with -fno-fine-grained-bitfield-accesses, the assignment in file A freezes both fields of "x". If file A is compiled with -ffine-grained-bitfield-accesses, but file B is not, f() corrupts the field "a", so g() returns poison (if I'm not missing anything?).

In D128317#3612910, @arsenm wrote:

I don't agree with this change. I definitely don't agree with changing the MIR reduction. I would say most of the value I've received from bugpoint and llvm-reduce is in finding bugs from mishandling undefs

In D128501#3608846, @efriedma wrote:

Under this scheme, is it illegal to link together object files built with -ffine-grained-bitfield-accesses and object files built with -fno-fine-grained-bitfield-accesses?

One more comment: it would be great if you could update LangRef to mention the special case of fneg. Thanks!

In rGd3919a8cc5031fd3d8c1ce3188abc0d7551306fb#1098667, @dcandler wrote:

That fneg gets handled differently came up during the review, https://reviews.llvm.org/D116952#3417339 and I found it similarly didn't get flushed on Arm.

In rGd3919a8cc5031fd3d8c1ce3188abc0d7551306fb#1098615, @dcandler wrote:

That's interesting. Is test_float_fneg_pzero_f32_out() the only one to have a problem? All the fneg tests do the same thing because fneg should not be affected by the denormal-fp-math attribute, so the result should always be the input negated.

0xB800000000000000 is just 0x3800000000000000 with the sign changed, which is what I'd expect from fneg no matter what the attrinute is set to. That said, I did notice a mistake with the tests: I didn't correctly vary the attributes on some of the other fneg tests. They should be

test_float_fneg_pzero_out() #1
test_float_fneg_psign_out() #2
test_float_fneg_pzero_in() #3
test_float_fneg_psign_in() #4

...but that wouldn't explain test_float_fneg_pzero_f32_out() failing, especially if the others pass.

In D116952#3603700, @shchenz wrote:

Hi @nlopes, thanks for providing the useful info. However I am still not very clear about how to deal with our internal failure after this patch.

define i1 @foo() denormal-fp-math=positive-zero,positive-zero {
  %_add = fadd double 0.000000, 0.000000, exceptions=ignore
  %_res = fcmp une double %_add, 0.000000
  ret i1 %_res
}
=>
define i1 @foo() noread nowrite nofree willreturn denormal-fp-math=positive-zero,positive-zero {
  ret i1 1
}

The alive result is very confusing. I don't understand why 0.000000 + 0.000000 != 0x000000 when denormal-fp-math=positive-zero, could you help to explain?

In D116952#3601819, @shchenz wrote:

In D116952#3601629, @nlopes wrote:

In D116952#3600716, @shchenz wrote:

define zeroext i1 @foo() #0 {
  %_add = fadd fast double 1.264810e-321, 3.789480e-321
  %_res = fcmp fast une double %_add, 5.054290e-321
  ret i1 %_res
}

attributes #0 = { "denormal-fp-math"="positive-zero" }

llc 1.ll -mtriple=powerpc64le-unknown-linux-gnu

Hi, this patch causes mis-compile for above case, now %_res is true while before this patch it is false. We can not handle the denormal constantFP in fcmp? Will the denormal constantFP be in other opcodes as well?

Alive says that LLVM is correct:

define i1 @foo() denormal-fp-math=positive-zero,positive-zero {
  %_add = fadd double 0.000000, 0.000000, exceptions=ignore
  %_res = fcmp une double %_add, 0.000000
  ret i1 %_res
}
=>
define i1 @foo() noread nowrite nofree willreturn denormal-fp-math=positive-zero,positive-zero {
  ret i1 1
}
Transformation seems to be correct!

Alive 2 says "ERROR: Couldn't prove the correctness of the transformation"...

https://alive2.llvm.org/ce/z/GPLj4Z

In D116952#3600716, @shchenz wrote:

define zeroext i1 @foo() #0 {
  %_add = fadd fast double 1.264810e-321, 3.789480e-321
  %_res = fcmp fast une double %_add, 5.054290e-321
  ret i1 %_res
}

attributes #0 = { "denormal-fp-math"="positive-zero" }

llc 1.ll -mtriple=powerpc64le-unknown-linux-gnu

Hi, this patch causes mis-compile for above case, now %_res is true while before this patch it is false. We can not handle the denormal constantFP in fcmp? Will the denormal constantFP be in other opcodes as well?

Alive2 complains about one of the test cases. Could you please double check? Thanks

define float @test_float_fneg_pzero_f32_out() denormal-fp-math=ieee,ieee denormal-fp-math-f32=positive-zero,ieee {
  %result = fneg float 0.000000, exceptions=ignore
  ret float %result
}
=>
define float @test_float_fneg_pzero_f32_out() noread nowrite nofree willreturn denormal-fp-math=ieee,ieee denormal-fp-math-f32=positive-zero,ieee {
  ret float -0.000000
}
Transformation doesn't verify!

LGTM

Could you please explain what's the bug and why is this the right fix?

There's no tradition in sharing tests between GVN & NewGVN. Are you going to implement the functionality in both new & old GVN?

If possible, please replace all uses of undef with poison. Thank you!

If possible, please replace all uses of undef with poison. Thank you!

In rGc1b610307df22d12687bde26919e45752c33ab0b#1094754, @aeubanks wrote:

this patch does a bit more than just remove br i1 undef, could you make the description a bit more accurate in future changes?

That's a bug, yes. I won't comment on the fix as I don't know enough about SLP's code.

In D126895#3554064, @vporpo wrote:

I think that the issue boils down to whether: %zext = zext i8 poison to i32 is an i32 poison.

In D126895#3553958, @vporpo wrote:

The issue is that the input to SLP contains phis with an undef input when the input is unreachable. Then SLP converts this to poison which I think is not legal.

In D126895#3553868, @nikic wrote:

Using poison for values from unreachable blocks is legal and preferred.

LGTM, sounds like the right approach. Negative indexes are not that common anyway.

In D126550#3546219, @thakis wrote:

This seems to break all kinds of tests, see http://45.33.8.238/linux/77205/step_12.txt

Did you run tests locally before landing?

Please take a look and revert for now if it takes a while to fix.

We believe that there's a bug when the 2nd argument is 0.
See here: https://gcc.godbolt.org/z/9735rYbqP

Somehow on my computer I don't get the same expected result for one of the tests:

$ cat aa.ll
define i8 @sub_add_umin_commute_umin(i8 %x, i8 %y, i8 %z) {
  %a = add i8 %x, %y
  %m = call i8 @llvm.umin.i8(i8 %z, i8 %y)
  %s = sub i8 %a, %m
  ret i8 %s
}

In D123473#3493474, @fhahn wrote:

In D123473#3491746, @nlopes wrote:

In D123473#3485355, @nlopes wrote:

In D123473#3484702, @reames wrote:

I don't think we actually want to do that. Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.

You've a point. My concern is that it's surprising to have LLVM introducing a load in a function that's not supposed to do any load. I was thinking about I/O, but those accesses should be volatile. For kernel code it may not be ok, but probably they also use volatile to prevent the compiler messing around instead of relying on writeonly

While I think that's a great first data point, the coverage of writeonly functions is probably not too extensive unfortunately. Another interesting data point may be to (randomly?) add writeonly attributes to the existing tests and verify that.

In D123473#3485355, @nlopes wrote:

In D123473#3484702, @reames wrote:

I don't think we actually want to do that. Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.

You've a point. My concern is that it's surprising to have LLVM introducing a load in a function that's not supposed to do any load. I was thinking about I/O, but those accesses should be volatile. For kernel code it may not be ok, but probably they also use volatile to prevent the compiler messing around instead of relying on writeonly

In D112025#3488141, @sepavloff wrote:

In D112025#3485583, @nlopes wrote:

Is the second argument required to be a constant? If so, it would be great to document that. Thanks!

Fixed in https://github.com/llvm/llvm-project/commit/d9b5544e0f99656ac18e39daf31fa8231ea0d9ef.
Thanks!

Is the second argument required to be a constant? If so, it would be great to document that. Thanks!

In D123473#3484702, @reames wrote:

I don't think we actually want to do that. Instead, I think we're making a mistake by considering the load to be immediate UB as opposed to simply returning poison in this case.

Sounds great, thanks! We should avoid introducing ptr2int at all costs.

Well, I think this is ok. It's not regressing behavior vs undef store removal. So if this is important for Rust, let's maybe keep it,
Fingers crossed that the GSoC student will make good progress on the uninitialized memory thing.

@regehr If you have time, it would be nice to fuzz LICM to check if this assertion can be triggered (after the patch lands).
It needs a function with the writeonly attribute + a pointer as input (or a global variable), a loop with a store to said pointer, a phi, and that's it (i.e., the test cases here as seed). Then some random stuff to make it crash.
Thanks!

LGTM. It's better than keeping freeze around. If we get concrete examples later of non-ideal "best" constants we can improve.

In D123991#3466373, @aqjune wrote:

In D123991#3464364, @nlopes wrote:

Well, here we are discussing the semantics of LLVM IR. It's ok for the semantics of SDAG to be different, as long as it's a refinement of LLVM IR's.

I think any valid compilation of LLVM IR should be considered here.
There might be another compiler for LLVM IR that does not use SDAG internally. If padding is poison, it is valid for the other compiler to lower padding to non-zero bits, which will invalidate the transformations in various machines mentioned above.

I'm surprised the assertion is not an if. But if it doesn't trigger, then LGTM.

LGTM, I think it reads well.

In D123991#3464417, @nikic wrote:

In D123991#3464364, @nlopes wrote:

Well, here we are discussing the semantics of LLVM IR. It's ok for the semantics of SDAG to be different, as long as it's a refinement of LLVM IR's.
Padding at IR level is not observable normally. So we can define it as poison.

The only way to observe padding is through store/load of different types. But we specify that the location of padding is non-deterministic in the case of vectors. I don't see a reason for padding in integer types to be different.

The SDAG can happily assume that the padding is zero because stores at SDAG level enforce that. But we don't need to offer that semantics at IR level.

The only advantage of defining padding as zero would be, as the examples show, fold load+zext into a larger load. But for this to work we would need to define the memory layout, and I don't know if that's consistent across backends. And is it worth it?

I don't think the SDAG semantics would be a refinement of the IR semantics though. In SDAG, a non-byte-size load performs an implicit assert that the padding bits are zero, while the IR semantics (as specified here) would allow that padding to be poison. Presumably this is where the "the result is undefined if the value was not originally written using a store of the same type" wording that LangRef uses right now comes from, as that allows making a consistent choice between load and store.

Well, here we are discussing the semantics of LLVM IR. It's ok for the semantics of SDAG to be different, as long as it's a refinement of LLVM IR's.
Padding at IR level is not observable normally. So we can define it as poison.

Not sure it's sufficient to restrict to the callers. Essentially we want to assert that these functions don't have state, so any read/writes they do is to locally allocated memory. No other function in the program can access that memory.

In D123473#3462792, @nikic wrote:

Maybe a better solution would be to clarify the wording for writeonly in langref to be clear that the attribute only considers writes to memory visible to its callers. Then we can retain writeonly even if loads to local objects are introduced. I doubt considering reads to function-local objects for writeonly adds any value.

These are already accepted semantics for memory attributes, see e.g. the code in checkFunctionMemoryAccess(), which ignores local memory. The wording in LangRef is "If a writeonly function reads memory visible to the program", which is a bit unclear, but probably the intent of "visible to the program" here is "visible to the caller".

sorry, never mind, Alive2's counterexample seems wrong. I'll dig up further.

Alive2 complains about this commit:

define <2 x i32> @modulo2_vec(<2 x i32> %x) {
%0:
  %rem.i = srem <2 x i32> %x, { 2, 2 }
  %cmp.i = icmp slt <2 x i32> %rem.i, { 0, 0 }
  %add.i = select <2 x i1> %cmp.i, <2 x i32> { 2, 2 }, <2 x i32> { 0, 0 }
  %ret.i = add nsw <2 x i32> %add.i, %rem.i
  ret <2 x i32> %ret.i
}
=>
define <2 x i32> @modulo2_vec(<2 x i32> %x) {
%0:
  %rem.i = srem <2 x i32> %x, { 2, 2 }
  %1 = and <2 x i32> %rem.i, { 2, 2 }
  %ret.i = add nsw <2 x i32> %1, %rem.i
  ret <2 x i32> %ret.i
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

In D123473#3442779, @fhahn wrote:

In D123473#3442753, @nlopes wrote:

I think the patch is not the full fix, as far as I understand. It's a good improvement, perf & correctness wise, though, but doesn't fix #51248.

There's one missing case: the load is needed and the function can't load memory (writeonly, readnone, etc?). In that case the optimization should bail out.

I think at the moment, the load will only be generated if the source already had a load that is guaranteed to execute. So hoisting that in a write only function should be fine, as it is already UB.

I think the patch is not the full fix, as far as I understand. It's a good improvement, perf & correctness wise, though, but doesn't fix #51248.

In D121614#3394190, @asl wrote:

@nlopes I think we need to better frame the requirement then, ask for CVs (this is always done though) and maybe even organize some kind of interviews? Strongly favouring ones with prior contributions is a good thing IMO

I don't know how to frame this nicely, but I think it's important to set the expectations right: what kind of support can students expect from mentors? Mentors aren't their babysitter nor have the responsibility to fix the bugs for them. Nor fix compilation errors. I would like to have this written down given some bad experiences in the past.
Mentor's time is expensive and scarce, so use it wisely. The mentor is there to guide only, not to teach how to code.

In D121243#3368365, @nlopes wrote:

For this transform only -- because we are guaranteed to repeat the values in each fcmp -- I was expecting that we could 'or' the relevant flags. But there's a surprising corner case with true and false fcmp predicates according to Alive2:
https://alive2.llvm.org/ce/z/Nffn3L

The behavior -- blocking poison via predicate? -- does not seem to be documented in the LangRef.

oops, that seems to be a bug in Alive2.
We convert fcmp true to true, but that is wrong as we lose the fast-math flags. Let me fix that.

For this transform only -- because we are guaranteed to repeat the values in each fcmp -- I was expecting that we could 'or' the relevant flags. But there's a surprising corner case with true and false fcmp predicates according to Alive2:
https://alive2.llvm.org/ce/z/Nffn3L

The behavior -- blocking poison via predicate? -- does not seem to be documented in the LangRef.

Let me come back the usefulness question: which frontend can mark loops as finite but can't mark functions as willreturn?

This concept of sub-objects also shows up in the 'restrict' stuff. The difference is that with 'restrict' the aliasing constraints are dynamic: the promise is that e.g. two (dynamic) accesses in the original program don't alias. Here the constrains are static.

In D120000#3357413, @craig.topper wrote:

Anyway, this transformation works by creating some artificial bitcasts that you expect to be carried over to the backend so the lowering can share the calls to the TLS function.

This strategy seems very brittle to me. If some later optimization decides to remove the bitcast, your optimization will stop working. It's very likely that will be the case once opaque pointers take over.

Is this much different than the artificial bitcast we use in ConstantHoisting?