Page MenuHomePhabricator

nlopes (Nuno Lopes)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 3 2013, 11:31 AM (456 w, 6 d)

Recent Activity

Sun, Jul 3

nlopes committed rG53dc0f107877: [NFC] Switch a few uses of undef to poison as placeholders for unreachble code (authored by nlopes).
[NFC] Switch a few uses of undef to poison as placeholders for unreachble code
Sun, Jul 3, 6:34 AM · Restricted Project, Restricted Project
nlopes committed rG022bd92c78ed: [LowerMatrixMultiplication] Switch dummy values from undef to poison [NFC] (authored by nlopes).
[LowerMatrixMultiplication] Switch dummy values from undef to poison [NFC]
Sun, Jul 3, 4:32 AM · Restricted Project, Restricted Project

Sat, Jul 2

nlopes added a comment to D128839: [DirectX backend] Add createHandle BufferLoad/Store DXIL operation.

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

Sat, Jul 2, 3:24 PM · Restricted Project, Restricted Project

Fri, Jul 1

nlopes added a reverting change for rG47e6f98f84ac: [LowerMatrixMultiplication] Switch dummy values from undef to poison [NFC]: rG7c4f45f87a94: Revert [LowerMatrixMultiplication] Switch dummy values from undef to poison….
Fri, Jul 1, 3:54 PM · Restricted Project, Restricted Project
nlopes added a reverting change for rG3e701bcd2a6a: attempt to fix aarch64 build bot: rG7c4f45f87a94: Revert [LowerMatrixMultiplication] Switch dummy values from undef to poison….
Fri, Jul 1, 3:54 PM · Restricted Project, Restricted Project
nlopes committed rG7c4f45f87a94: Revert [LowerMatrixMultiplication] Switch dummy values from undef to poison… (authored by nlopes).
Revert [LowerMatrixMultiplication] Switch dummy values from undef to poison…
Fri, Jul 1, 3:54 PM · Restricted Project, Restricted Project
nlopes committed rG3e701bcd2a6a: attempt to fix aarch64 build bot (authored by nlopes).
attempt to fix aarch64 build bot
Fri, Jul 1, 3:44 PM · Restricted Project, Restricted Project
nlopes committed rG47e6f98f84ac: [LowerMatrixMultiplication] Switch dummy values from undef to poison [NFC] (authored by nlopes).
[LowerMatrixMultiplication] Switch dummy values from undef to poison [NFC]
Fri, Jul 1, 3:32 PM · Restricted Project, Restricted Project

Thu, Jun 30

nlopes committed rG373571dbb4b5: [NFC] Switch a few uses of undef to poison as placeholders for unreachble code (authored by nlopes).
[NFC] Switch a few uses of undef to poison as placeholders for unreachble code
Thu, Jun 30, 3:02 PM · Restricted Project, Restricted Project
nlopes committed rG0586d1cac285: [NFC] Switch a few uses of undef to poison as placeholders for unreachble code (authored by nlopes).
[NFC] Switch a few uses of undef to poison as placeholders for unreachble code
Thu, Jun 30, 1:47 PM · Restricted Project, Restricted Project

Mon, Jun 27

nlopes added a comment to D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible.

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?).

Mon, Jun 27, 1:25 PM · Restricted Project, Restricted Project
nlopes added a comment to D128317: stop llvm-reduce from introducing undefs.

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

Mon, Jun 27, 10:46 AM · Restricted Project, Restricted Project

Sun, Jun 26

nlopes committed rGb79575d45662: PHINode::removeIncomingValue: use poison as a placeholder instead of undef when… (authored by nlopes).
PHINode::removeIncomingValue: use poison as a placeholder instead of undef when…
Sun, Jun 26, 10:51 AM · Restricted Project, Restricted Project
nlopes committed rG6ef9a2ad01c0: [LICM] Use poison to replace unreachable values instead of undef [NFC] (authored by nlopes).
[LICM] Use poison to replace unreachable values instead of undef [NFC]
Sun, Jun 26, 6:56 AM · Restricted Project, Restricted Project
nlopes committed rG3fa2411dc56f: [LoopSimplifyCFG] use poison when replacing dead instructions instead of undef… (authored by nlopes).
[LoopSimplifyCFG] use poison when replacing dead instructions instead of undef…
Sun, Jun 26, 6:16 AM · Restricted Project, Restricted Project
nlopes committed rGd46fa1fc58b4: [ArgumentPromotion] use poison when replacing dead instructions instead of… (authored by nlopes).
[ArgumentPromotion] use poison when replacing dead instructions instead of…
Sun, Jun 26, 5:44 AM · Restricted Project, Restricted Project

Sat, Jun 25

nlopes added a comment to D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible.

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?

Sat, Jun 25, 4:55 AM · Restricted Project, Restricted Project

Fri, Jun 24

nlopes added inline comments to D114650: [SCEV] Construct SCEV iteratively..
Fri, Jun 24, 10:05 AM · Restricted Project, Restricted Project

Thu, Jun 23

nlopes added a comment to rGd3919a8cc503: [ConstantFolding] Respect denormal handling mode attributes when folding….

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

Thu, Jun 23, 6:23 AM · Restricted Project, Restricted Project
nlopes updated subscribers of rGd3919a8cc503: [ConstantFolding] Respect denormal handling mode attributes when folding….

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.

Thu, Jun 23, 6:22 AM · Restricted Project, Restricted Project
nlopes added a comment to rGd3919a8cc503: [ConstantFolding] Respect denormal handling mode attributes when folding….

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.

Thu, Jun 23, 3:30 AM · Restricted Project, Restricted Project
nlopes added a comment to D116952: [ConstantFolding] Respect denormal handling mode attributes when folding instructions.

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?

Thu, Jun 23, 2:12 AM · Restricted Project, Restricted Project

Wed, Jun 22

nlopes added a comment to D116952: [ConstantFolding] Respect denormal handling mode attributes when folding instructions.
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

Wed, Jun 22, 9:26 AM · Restricted Project, Restricted Project
nlopes added a comment to D116952: [ConstantFolding] Respect denormal handling mode attributes when folding instructions.
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?

Wed, Jun 22, 7:02 AM · Restricted Project, Restricted Project
nlopes raised a concern with rGd3919a8cc503: [ConstantFolding] Respect denormal handling mode attributes when folding….

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!
Wed, Jun 22, 7:00 AM · Restricted Project, Restricted Project

Fri, Jun 17

nlopes added inline comments to D127960: [InstCombine] Push freeze through recurrence phi.
Fri, Jun 17, 1:03 AM · Restricted Project, Restricted Project

Thu, Jun 16

nlopes accepted D127960: [InstCombine] Push freeze through recurrence phi.

LGTM

Thu, Jun 16, 11:45 AM · Restricted Project, Restricted Project
nlopes added inline comments to D127960: [InstCombine] Push freeze through recurrence phi.
Thu, Jun 16, 6:44 AM · Restricted Project, Restricted Project
nlopes added a comment to D127942: [NewGVN] add context instruction for SimplifyQuery.

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

Thu, Jun 16, 1:01 AM · Restricted Project, Restricted Project

Mon, Jun 13

nlopes added a comment to D124228: [NFC][NewGVN][LoadCoercion] Add new tests for future commit.

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

Mon, Jun 13, 2:57 AM · Restricted Project, Restricted Project
nlopes added a comment to D124068: [NFC][NewGVN][LoadCoercion] Add tests for future commit..

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

Mon, Jun 13, 2:49 AM · Restricted Project, Restricted Project
nlopes added a comment to D127492: [X86][SLP] Basic test coverage for llvm.powi.

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

Mon, Jun 13, 2:47 AM · Restricted Project, Restricted Project
nlopes committed rG5a132499fb39: [NFC] Remove straight UB from SROA tests (authored by nlopes).
[NFC] Remove straight UB from SROA tests
Mon, Jun 13, 12:59 AM · Restricted Project, Restricted Project

Sun, Jun 12

nlopes added a comment to rGc1b610307df2: [NFC] Remove 'br i1 undef' from SROA tests.

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

Sun, Jun 12, 1:09 PM · Restricted Project, Restricted Project
nlopes committed rG571ae1abebb6: fix test expected output (fixes arm buildbot failure) [NFC] (authored by nlopes).
fix test expected output (fixes arm buildbot failure) [NFC]
Sun, Jun 12, 11:29 AM · Restricted Project, Restricted Project
nlopes committed rG4dd1bffc9dac: [clang][CodeGen] Switch a few placeholders from UndefValue to PoisonValue (authored by nlopes).
[clang][CodeGen] Switch a few placeholders from UndefValue to PoisonValue
Sun, Jun 12, 11:08 AM · Restricted Project, Restricted Project
nlopes committed rGc1b610307df2: [NFC] Remove 'br i1 undef' from SROA tests (authored by nlopes).
[NFC] Remove 'br i1 undef' from SROA tests
Sun, Jun 12, 7:30 AM · Restricted Project, Restricted Project

Fri, Jun 10

nlopes committed rGe5c5f92e1282: [InstCombine] switch synthetic unreachable to use undef instead of poison (NFC) (authored by nlopes).
[InstCombine] switch synthetic unreachable to use undef instead of poison (NFC)
Fri, Jun 10, 1:54 PM · Restricted Project, Restricted Project
nlopes committed rG952e06939380: [NFC] remove 'br undef' from InstCombine test cases (authored by nlopes).
[NFC] remove 'br undef' from InstCombine test cases
Fri, Jun 10, 7:29 AM · Restricted Project, Restricted Project
nlopes committed rGeb8cbb3ad796: [NFC] Add 3 more -inseltpoison.ll test variations (authored by nlopes).
[NFC] Add 3 more -inseltpoison.ll test variations
Fri, Jun 10, 6:07 AM · Restricted Project, Restricted Project

Jun 6 2022

nlopes added inline comments to D127119: [SLP]Fix undef handling in gather function..
Jun 6 2022, 1:19 PM · Restricted Project, Restricted Project
nlopes updated subscribers of D127119: [SLP]Fix undef handling in gather function..
Jun 6 2022, 12:05 PM · Restricted Project, Restricted Project
nlopes added inline comments to D127119: [SLP]Fix undef handling in gather function..
Jun 6 2022, 11:46 AM · Restricted Project, Restricted Project

Jun 5 2022

nlopes added inline comments to D127073: [SLP] Treat undef as any other constant.
Jun 5 2022, 1:39 PM · Restricted Project, Restricted Project
nlopes added inline comments to D127073: [SLP] Treat undef as any other constant.
Jun 5 2022, 11:41 AM · Restricted Project, Restricted Project

Jun 4 2022

nlopes added inline comments to D126939: [SLP] Avoid converting undef to poison when gathering..
Jun 4 2022, 12:59 AM · Restricted Project, Restricted Project

Jun 3 2022

nlopes added a comment to D126939: [SLP] Avoid converting undef to poison when gathering..

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

Jun 3 2022, 2:22 AM · Restricted Project, Restricted Project

Jun 2 2022

nlopes added a comment to D126895: [SLP] Phi inputs that come from an unreachable block should be undef..

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

Jun 2 2022, 11:49 AM · Restricted Project, Restricted Project
nlopes requested changes to D126895: [SLP] Phi inputs that come from an unreachable block should be undef..

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.

Jun 2 2022, 11:15 AM · Restricted Project, Restricted Project
nlopes added a comment to D126895: [SLP] Phi inputs that come from an unreachable block should be undef..

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

Jun 2 2022, 10:51 AM · Restricted Project, Restricted Project

Jun 1 2022

nlopes added inline comments to D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
Jun 1 2022, 2:04 AM · Restricted Project, Restricted Project

May 31 2022

nlopes accepted D126687: [InstCombine] Fix inbounds preservation when swapping GEPs (PR44206).

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

May 31 2022, 5:59 AM · Restricted Project, Restricted Project

May 30 2022

nlopes committed rG8c55de9ee7f6: fix tests after my commit 80b3dcc045f8ea6e5e532d8891bbf1305bce89e8 (authored by nlopes).
fix tests after my commit 80b3dcc045f8ea6e5e532d8891bbf1305bce89e8
May 30 2022, 11:44 AM · Restricted Project, Restricted Project
nlopes added a comment to D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.

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.

May 30 2022, 11:39 AM · Restricted Project, Restricted Project
nlopes committed rG80b3dcc045f8: [Support] Make report_fatal_error respect its GenCrashDiag argument so it… (authored by nlopes).
[Support] Make report_fatal_error respect its GenCrashDiag argument so it…
May 30 2022, 11:19 AM · Restricted Project, Restricted Project
nlopes closed D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
May 30 2022, 11:19 AM · Restricted Project, Restricted Project
nlopes added inline comments to D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
May 30 2022, 11:14 AM · Restricted Project, Restricted Project
nlopes added inline comments to D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
May 30 2022, 12:07 AM · Restricted Project, Restricted Project

May 29 2022

nlopes updated the diff for D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
May 29 2022, 1:21 PM · Restricted Project, Restricted Project
nlopes added inline comments to D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
May 29 2022, 10:43 AM · Restricted Project, Restricted Project

May 27 2022

nlopes requested review of D126550: Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace.
May 27 2022, 8:14 AM · Restricted Project, Restricted Project

May 26 2022

nlopes added inline comments to D125438: [InstCombine] NEW Baseline tests for InstCombine optimization to merge GEP instructions with constant indices.
May 26 2022, 8:42 AM · Restricted Project, Restricted Project

May 22 2022

nlopes added inline comments to D125438: [InstCombine] NEW Baseline tests for InstCombine optimization to merge GEP instructions with constant indices.
May 22 2022, 1:59 PM · Restricted Project, Restricted Project

May 20 2022

nlopes updated subscribers of D112025: Intrinsic for checking floating point class.

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

May 20 2022, 1:44 AM · Restricted Project, Restricted Project

May 19 2022

nlopes committed rG5fc9449c962a: [DeadArgElim] Use poison instead of undef as placeholder for dead arguments (authored by nlopes).
[DeadArgElim] Use poison instead of undef as placeholder for dead arguments
May 19 2022, 10:03 AM · Restricted Project, Restricted Project, Restricted Project
nlopes closed D125983: [DeadArgElim] Use poison instead of undef as placeholder for dead arguments.
May 19 2022, 10:03 AM · Restricted Project, Restricted Project, Restricted Project
nlopes requested review of D125983: [DeadArgElim] Use poison instead of undef as placeholder for dead arguments.
May 19 2022, 8:46 AM · Restricted Project, Restricted Project, Restricted Project
nlopes added inline comments to D125896: [LoopUnroll] Freeze tripcount rather than condition.
May 19 2022, 6:24 AM · Restricted Project, Restricted Project

May 10 2022

nlopes raised a concern with rG394c683d4063: [InstCombine] sub(add(X,Y),umin(Y,Z)) --> add(X,usub.sat(Y,Z)).

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
}
May 10 2022, 3:48 AM · Restricted Project, Restricted Project

May 9 2022

nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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.

May 9 2022, 8:01 AM · Restricted Project, Restricted Project

May 4 2022

nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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

May 4 2022, 10:53 AM · Restricted Project, Restricted Project

May 3 2022

nlopes added a comment to D112025: Intrinsic for checking floating point class.

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!

May 3 2022, 5:51 AM · Restricted Project, Restricted Project

May 2 2022

nlopes added a comment to D112025: Intrinsic for checking floating point class.

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

May 2 2022, 6:12 AM · Restricted Project, Restricted Project
nlopes added a reviewer for D123473: [LICM] Only create load in pre-header when promoting load.: aqjune.
May 2 2022, 4:02 AM · Restricted Project, Restricted Project
nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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.

May 2 2022, 4:01 AM · Restricted Project, Restricted Project

Apr 29 2022

nlopes added inline comments to D124677: [ConstantFold] Don't convert getelementptr to ptrtoint+inttoptr.
Apr 29 2022, 12:06 PM · Restricted Project, Restricted Project
nlopes accepted D124677: [ConstantFold] Don't convert getelementptr to ptrtoint+inttoptr.

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

Apr 29 2022, 9:10 AM · Restricted Project, Restricted Project
nlopes accepted D124173: [InstCombine] Remove memset of undef value.

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.

Apr 29 2022, 1:27 AM · Restricted Project, Restricted Project

Apr 26 2022

nlopes updated subscribers of D123473: [LICM] Only create load in pre-header when promoting load..

@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!

Apr 26 2022, 2:30 PM · Restricted Project, Restricted Project
nlopes accepted D123962: [InstCombine] fold freeze of partial undef/poison vector constants.

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

Apr 26 2022, 6:10 AM · Restricted Project, Restricted Project

Apr 24 2022

nlopes added a comment to D123991: [LangRef] Clarify load/store of non-byte-sized types.

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.

Apr 24 2022, 7:50 AM · Restricted Project, Restricted Project
nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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

Apr 24 2022, 7:39 AM · Restricted Project, Restricted Project

Apr 22 2022

nlopes accepted D124124: [LangRef] Limit read/writeonly attrs to memory visible to caller.

LGTM, I think it reads well.

Apr 22 2022, 4:35 AM · Restricted Project, Restricted Project

Apr 21 2022

nlopes added a comment to D123991: [LangRef] Clarify load/store of non-byte-sized types.

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.

Apr 21 2022, 6:49 AM · Restricted Project, Restricted Project
nlopes added a comment to D123991: [LangRef] Clarify load/store of non-byte-sized types.

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.

Apr 21 2022, 4:50 AM · Restricted Project, Restricted Project

Apr 20 2022

nlopes added a comment to D124124: [LangRef] Limit read/writeonly attrs to memory visible to caller.

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.

Apr 20 2022, 2:59 PM · Restricted Project, Restricted Project
nlopes added inline comments to D123473: [LICM] Only create load in pre-header when promoting load..
Apr 20 2022, 2:10 PM · Restricted Project, Restricted Project
nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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".

Apr 20 2022, 2:08 PM · Restricted Project, Restricted Project

Apr 14 2022

nlopes accepted rG5e9022483960: [InstCombine] SimplifyDemandedUseBits - remove lshr node if we only demand….

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

Apr 14 2022, 10:22 AM · Restricted Project, Restricted Project
nlopes raised a concern with rG5e9022483960: [InstCombine] SimplifyDemandedUseBits - remove lshr node if we only demand….

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
Apr 14 2022, 10:04 AM · Restricted Project, Restricted Project

Apr 11 2022

nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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.

Apr 11 2022, 7:44 AM · Restricted Project, Restricted Project
nlopes added a comment to D123473: [LICM] Only create load in pre-header when promoting load..

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.

Apr 11 2022, 7:29 AM · Restricted Project, Restricted Project

Mar 19 2022

nlopes added a comment to D121614: Add GSOC landing page with instructions for GSOC students..

@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

Mar 19 2022, 8:40 AM · Restricted Project

Mar 14 2022

nlopes added a comment to D121614: Add GSOC landing page with instructions for GSOC students..

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.

Mar 14 2022, 3:12 PM · Restricted Project

Mar 10 2022

nlopes committed rWbd394f67c84f: add GSoC 2022 load undef project (authored by nlopes).
add GSoC 2022 load undef project
Mar 10 2022, 3:36 AM · Restricted Project

Mar 8 2022

nlopes added a comment to D121243: [InstCombine] Preserve FMF in foldLogicOfFCmps..

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.

Mar 8 2022, 2:15 PM · Restricted Project, Restricted Project
nlopes added a comment to D121243: [InstCombine] Preserve FMF in foldLogicOfFCmps..

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.

Mar 8 2022, 2:06 PM · Restricted Project, Restricted Project

Mar 6 2022

nlopes added a comment to D118168: [LLVM] Introduce llvm.loop.finite metadata to represent loops which are known to iterate a finite number of times.

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

Mar 6 2022, 4:32 AM · Restricted Project, Restricted Project
nlopes added a comment to D115274: [IR][RFC] Memory region declaration intrinsic.

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.

Mar 6 2022, 4:27 AM · Restricted Project, Restricted Project

Mar 3 2022

nlopes added a comment to D120000: [1/3] TLS loads opimization (hoist).

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?

Mar 3 2022, 10:21 AM · Restricted Project, Restricted Project