This is an archive of the discontinued LLVM Phabricator instance.

[llvm-reduce] Add reduce operands pass
ClosedPublic

Authored by swamulism on Aug 29 2021, 7:40 PM.

Details

Summary

Add reduction to set operands to default null values

Diff Detail

Event Timeline

swamulism created this revision.Aug 29 2021, 7:40 PM
swamulism requested review of this revision.Aug 29 2021, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2021, 7:40 PM
swamulism edited the summary of this revision. (Show Details)Aug 29 2021, 7:42 PM
swamulism added reviewers: lebedev.ri, aeubanks.

I'm not sure if we should be using undef, poison, or the null value for reductions. I'll ask llvm-dev for opinions.

llvm/test/tools/llvm-reduce/remove-operands.ll
2

we should add tests for things like making sure we don't crash on a br since basic blocks don't have a null value

llvm/tools/llvm-reduce/DeltaManager.cpp
53

reduceOperandsDeltaPass for consistency

llvm/tools/llvm-reduce/deltas/ReduceOperands.cpp
32

can we list types that don't have a null value instead? seems better than the current approach since the list is much smaller

I'll run this patch over a larger file to see if we're missing some types in this check (as in we're trying to get the null value of some type that doesn't have a null value)

63

these are always used together, can we create something like canReduceOperand()?

swamulism updated this revision to Diff 370829.Sep 5 2021, 3:49 PM
swamulism marked an inline comment as done.

[llvm-reduce] Update reduce operand test and fix pass name

swamulism marked an inline comment as done.Sep 5 2021, 3:50 PM
swamulism added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperands.cpp
32

I would rather keep it a list of types that work rather than types that don't work. Since if a new type is added that is not nullable in the future this pass would crash.

But it would probably bet better for this logic to live in the same place as Constant::getNullValue

swamulism marked an inline comment as done.Sep 6 2021, 7:15 PM
aeubanks added inline comments.Sep 7 2021, 2:34 PM
llvm/tools/llvm-reduce/deltas/ReduceOperands.cpp
32

yeah, can you add a Constant::hasNullValue?

83

VariableCount doesn't really make sense, maybe just Count is good enough

Wouldn't it be better to leave the question of with what to replace to a separate patch, and stick to the current consistent practice here?

Wouldn't it be better to leave the question of with what to replace to a separate patch, and stick to the current consistent practice here?

I don't think it matters too much if we've agreed in the thread that null is better, and we do a followup patch soon where we add a flag and consolidate what we replace things with into a helper method controlled by the flag.

Wouldn't it be better to leave the question of with what to replace to a separate patch, and stick to the current consistent practice here?

I don't think it matters too much if we've agreed in the thread that null is better, and we do a followup patch soon where we add a flag and consolidate what we replace things with into a helper method controlled by the flag.

But did we agree on null?
And i'm not really sure what "I don't think it matters too much" means - this either does introduce inconsistency or it does not.

Wouldn't it be better to leave the question of with what to replace to a separate patch, and stick to the current consistent practice here?

I don't think it matters too much if we've agreed in the thread that null is better, and we do a followup patch soon where we add a flag and consolidate what we replace things with into a helper method controlled by the flag.

But did we agree on null?

Reading through the thread, there was a vague consensus that using undef was not ideal and null is probably better in most cases. https://groups.google.com/g/llvm-dev/c/ofvHCsXrLN8

And i'm not really sure what "I don't think it matters too much" means - this either does introduce inconsistency or it does not.

It does introduce inconsistency, but I don't think the inconsistency matters. We'll end up either having undef or null in the reduced repro, either one is probably for most cases.

May i propose that we do that either after this patch, or before this patch, but not overlap the two?
I am not seeing quite seeing from where the motivation for the latter comes from.

Sure I suppose we can split up this patch
@swamulism can you do that?

Sure I suppose we can split up this patch
@swamulism can you do that?

Sure I can add Constant::hasNullValue and remove ReduceOperands::typeIsNullable in a separate patch

Sure I suppose we can split up this patch
@swamulism can you do that?

Sure I can add Constant::hasNullValue and remove ReduceOperands::typeIsNullable in a separate patch

I meant the part about reducing to undef vs null. Constant::hasNullValue() should be in this patch

Sure I suppose we can split up this patch
@swamulism can you do that?

Sure I can add Constant::hasNullValue and remove ReduceOperands::typeIsNullable in a separate patch

I meant the part about reducing to undef vs null. Constant::hasNullValue() should be in this patch

Also no, that can be separated - place it before Constant::getNullValue() / base it on that function,
and change Constant::getNullValue() to first assert that said new function says there is such a constant.

But, i'm still not sold on null everywhere. E.g. div-by-zero is ub, gep inbounds of null w/ non-zero offset is null, etc.

Sure I suppose we can split up this patch
@swamulism can you do that?

Sure I can add Constant::hasNullValue and remove ReduceOperands::typeIsNullable in a separate patch

I meant the part about reducing to undef vs null. Constant::hasNullValue() should be in this patch

Also no, that can be separated - place it before Constant::getNullValue() / base it on that function,
and change Constant::getNullValue() to first assert that said new function says there is such a constant.

But, i'm still not sold on null everywhere. E.g. div-by-zero is ub, gep inbounds of null w/ non-zero offset is null, etc.

Isn't it the same for undef? Since we could arbitrarily decide to replace all undefs with null. Using undef should always be at least the same amount of ub as using null.

swamulism added a comment.EditedSep 7 2021, 4:13 PM

Also no, that can be separated - place it before Constant::getNullValue() / base it on that function,
and change Constant::getNullValue() to first assert that said new function says there is such a constant.

But, i'm still not sold on null everywhere. E.g. div-by-zero is ub, gep inbounds of null w/ non-zero offset is null, etc.

I was thinking to change getNullValue to return nullptr when there is no null value instead of crashing.
Then make hasNullValue to something like the following so the code only needs to be updated in one place when future types are added.

bool Constant::hasNullValue(Type *Ty) {
  return getNullValue(Ty) != nullptr;
}

Also for the null vs undef discussion, llvm-reduce should not reduce using this delta pass if it doesn't pass the interestingness test.
Is the worry that this pass will make the the reduction worse (longer or harder to read)?

swamulism updated this revision to Diff 371214.Sep 7 2021, 4:21 PM

Add Constant::hasNullValue

swamulism marked 3 inline comments as done.Sep 7 2021, 4:22 PM
aeubanks added inline comments.Sep 8 2021, 11:33 AM
llvm/lib/IR/Constants.cpp
32 ↗(On Diff #371214)

remove

388 ↗(On Diff #371214)

if we're going to return nullptr here there's no need for Constant::hasNullValue
also can you update the documentation saying that it'll return null for not supported types, and move it into Constant.h

swamulism updated this revision to Diff 371460.Sep 8 2021, 3:24 PM

Add documentation to getNullValue and remove hasNullValue

swamulism updated this revision to Diff 371478.Sep 8 2021, 5:15 PM

Add missing newline

lebedev.ri resigned from this revision.Sep 9 2021, 5:02 AM

My point is either not being heard, or is being intentionally ignored.

I honestly don't understand what you're trying to say. I thought your point was that making everything null instead of undef would be bad because some operations may have UB when using null (e.g. your null GEP example). But that argument doesn't make sense since undef is more undefined than null. div by undef is still UB.

Or are you trying to say that we should be doing the operand reduction pass in the first place on some instructions?

@aeubanks I believe @lebedev.ri is asking you to use undef for the initial patch and then switch undef to 0 consistently for all reductions in llvm-reduce. Right now everything else uses undef, and this place is going to use 0.

@aeubanks I believe @lebedev.ri is asking you to use undef for the initial patch and then switch undef to 0 consistently for all reductions in llvm-reduce. Right now everything else uses undef, and this place is going to use 0.

When I said

Sure I suppose we can split up this patch
@swamulism can you do that?

I was expecting a separate patch to add a helper method to choose what value to replace things with given a type.
Since replacing most operands with undef seems not ideal when it comes to reducing, I'd rather that patch come first, but I wouldn't object to making this patch use undef first then having the patch to choose what value to reduce to coming after.

Separately, after rereading @lebedev.ri 's comments, I was getting the impression that maybe I misunderstood the comments about not wanting to replace every operand of every instruction with null (or undef). Perhaps you meant that we should skip replacing the pointer operand of a GEP or the second operand of a div with null/undef?

@aeubanks I believe @lebedev.ri is asking you to use undef for the initial patch and then switch undef to 0 consistently for all reductions in llvm-reduce. Right now everything else uses undef, and this place is going to use 0.

Precisely. I have no clue how else i need to reword my message to get my thought across,

Sorry for my bad communication, I was fine with doing what you wanted. That's what I meant with

Sure I suppose we can split up this patch
@swamulism can you do that?

Sure I can add Constant::hasNullValue and remove ReduceOperands::typeIsNullable in a separate patch

I meant the part about reducing to undef vs null. Constant::hasNullValue() should be in this patch

swamulism updated this revision to Diff 372602.Sep 14 2021, 5:59 PM

Change from reducing to null to reducing to undef

swamulism updated this revision to Diff 372620.Sep 14 2021, 7:38 PM

Update incorrect docs

almost there

llvm/test/tools/llvm-reduce/no-replace-intrinsic-callee-with-undef.ll
6

this doesn't contain any operand bundles, so the pass shouldn't be necessary

llvm/test/tools/llvm-reduce/remove-invoked-functions.ll
1

I think the following is cleaner:

diff --git a/llvm/test/tools/llvm-reduce/remove-invoked-functions.ll b/llvm/test/tools/llvm-reduce/remove-invoked-functions.ll
index e1224b99e176..0d3f8f87fd4a 100644
--- a/llvm/test/tools/llvm-reduce/remove-invoked-functions.ll
+++ b/llvm/test/tools/llvm-reduce/remove-invoked-functions.ll
@@ -1,4 +1,4 @@
-; RUN: llvm-reduce --delta-passes=functions,function-bodies,arguments,attributes --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
+; RUN: llvm-reduce --test FileCheck --test-arg --check-prefixes=CHECK-ALL,CHECK-INTERESTINGNESS --test-arg %s --test-arg --input-file %s -o %t
 ; RUN: cat %t | FileCheck --check-prefixes=CHECK-ALL,CHECK-FINAL %s
 
 ; CHECK-INTERESTINGNESS: define i32 @maybe_throwing_callee(
@@ -22,8 +22,8 @@ declare void @thrown()
 define void @caller(i32 %arg) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 ; CHECK-ALL: bb:
 bb:
-; CHECK-INTERESTINGNESS: %i0 = invoke i32
-; CHECK-FINAL: %i0 = invoke i32 bitcast (i32 ()* @maybe_throwing_callee to i32 (i32)*)(i32 %arg)
+; CHECK-INTERESTINGNESS: %i0 = invoke i32 {{.*}}@maybe_throwing_callee
+; CHECK-FINAL: %i0 = invoke i32 bitcast (i32 ()* @maybe_throwing_callee to i32 (i32)*)
 ; CHECK-ALL: to label %bb3 unwind label %bb1
   %i0 = invoke i32 @maybe_throwing_callee(i32 %arg)
           to label %bb3 unwind label %bb1
llvm/tools/llvm-reduce/deltas/ReduceOperands.cpp
17

do we need this?

19

do we need this?

23

undef

37

we actually can replace token values with an undef

e.g.

declare void @llvm.foo(token)

define void @f() {
  call void @llvm.foo(token undef)
  ret void
}

verifies

swamulism updated this revision to Diff 372856.Sep 15 2021, 7:53 PM

Address comments

swamulism marked 6 inline comments as done.Sep 15 2021, 7:57 PM

Thanks for looking through this!

aeubanks accepted this revision.Sep 15 2021, 9:20 PM
This revision is now accepted and ready to land.Sep 15 2021, 9:20 PM
This revision was landed with ongoing or failed builds.Sep 17 2021, 12:33 PM
This revision was automatically updated to reflect the committed changes.