Thank you for taking a look!
Addressing nits.

While there, extract runOneConfiguration() out of runBatch().

Simplify loops in computeBatch().

@gchatelet thank you for taking a look!
Is this better?

In D142292#4072244, @lebedev.ri wrote:

In D142292#4072222, @efriedma wrote:

My primary concern here is compile-time; in general, the more complex expressions we model, the more we have to worry about SCEV going out of control in complex cases.

True, that seems to be The concern for SCEV.

In D142292#4072222, @efriedma wrote:

My primary concern here is compile-time; in general, the more complex expressions we model, the more we have to worry about SCEV going out of control in complex cases.

And more refactorings.

@Meinersbur please help with polly stuff. I don't expect that i guessed right,
and i certainly didn't update the test correctly. They have way too much undef in them...

Peeled off all refactorings, diff is much simpler now.

@RKSimon thank you for taking a look!
Addressing nits.

@RKSimon thank you for taking a look!
Addressing nits.

In D142292#4071665, @nikic wrote:

What I didn't really understand are your examples in the patch description. The referenced godbolt examples don't contain select instructions, and the input program also doesn't look like it should involve selects, so I'm a bit unclear on the relation these examples have to the patch.

I think non-refactoring patches that say what they do, but don't say why they do what they do, should be automatically reverted.
That TLDR is my general motivation. Sure, they no longer have select's because they got canonicalized into min/max, but that isn't true for everything.

Please file a bug if there isn't one already.

Rebased, NFC.

Rebased, NFC.
Now that D141776 has landed, this can proceed.

@RKSimon thank you for the review!

(will only proceed after D140271.)

In D142254#4070376, @goldstein.w.n wrote:

In D142254#4070259, @RKSimon wrote:

This feels like we could easily make this a generic DAG fold with a suitable TLI callback to help determine the preferred CondCode (or just signed/unsigned).

What where you thinking the API would be? I can see it being easy to represent "prefered signed/unsigned" but have trouble seeing a clean and generic
API for only if LT/GT but NOT for LE/GE.

In D140880#4069399, @foad wrote:

That being said, i don't think this can be just set, at best it should be hidden under an opt-in flag.

I don't think there's much point doing this at all if it's opt-in. If you already know that you're suffering from a slow development cycle due to PIE, you might as well just type cmake -DCMAKE_EXE_LINKER_FLAGS=-no-pie. The idea of doing by default was to fix the problem for developers who didn't even know they had the problem.

You probably want https://cmake.org/cmake/help/v3.16/module/CheckPIESupported.html
That being said, i don't think this can be just set, at best it should be hidden under an opt-in flag.

Yay!

ping

In D142123#4066460, @aaron.ballman wrote:

In D142123#4066447, @lebedev.ri wrote:

In D142123#4066351, @njames93 wrote:

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

Shall we remove abseil checks?
Shall we remove libc++-specific checks?
Shall we remove webkit checks?
Shall we remove backwards-compatibility checks?

Nothing is ever useful for everyone. Much like -Weverything,
enabling all checks comes with an explcit caveat that
one needs to disable the checks that are not applicable for the codebase.

+1 to having this check.

We're not asking for it to be useful to everyone; we are asking for justification for the current proposed form because there are problems with what's proposed. We're trying to figure out what the correct approach is (fwiw, I'd be opposed to what's proposed as-is but would be fine with a tweaked proposal).

In D142123#4066351, @njames93 wrote:

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

In D142038#4065083, @RKSimon wrote:

I'm sorry if I've caused you an issue here, but you did ask on D141877 that I show you my alternative approach, and I did my best to get something to you as fast as possible, despite me explaining that I'm stretched at the moment.

The level of refactoring that you employ in your version is more than is needed to add support for the SSE shifts to fix the regressions on D141778, and I don't agree that generalizing the code so much is necessary, especially as it affects understand-ability.

Maybe if you proposed refactoring patches that weren't part of a dependency chain for a simple improvement then it'd be easier to have a conversation.

Makes sense to me.

In D136651#4063818, @friss wrote:

In D136651#4063632, @lebedev.ri wrote:

In D136651#4063597, @friss wrote:

In D136651#4060071, @lebedev.ri wrote:

Docs still missing.

Sorry @lebedev.ri I missed your earlier comment about this. What format of doc would you like to see? A more elaborate comment in the tool's source or something else?

Right now it's impossible to discover that tool unless you already know it exists (or it's mentioned in some xcode doc, i guess)
It should have it's own description in it's --help, and ideally an .rst documentation page in docs.

I have no issue adding a more detailed help text. The documentation page seems somewhat overkill, but I guess I could be convinced otherwise. You need a fairly special build environment for this to make a difference and I'm not sure it's broadly applicable. You also need good build system integration to make it practical.

In D136651#4063597, @friss wrote:

In D136651#4060071, @lebedev.ri wrote:

Docs still missing.

Sorry @lebedev.ri I missed your earlier comment about this. What format of doc would you like to see? A more elaborate comment in the tool's source or something else?

Rebased, NFC.
Same regression remains, and given that my attempts to improve other things aren't being well-received, i'm not sure if i should bothed.

ping

ping

ping

I don't consider this code to be better, and if it is considered to be,
i may need to reevaluate trying to contribute to this backend.

I guess i don't see why this is better than the current version of my patches?
Also, this has insufficient test coverage.

Thanks!

Rebased.

Rebased.

Use less smart functions.
@RKSimon is this what you had in mind?

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165311, @srj wrote:

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165287, @srj wrote:

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165275, @lebedev.ri wrote:

Reproducer please. That simply means the assertion should be replaced with return undef;.

I'll get you one tomorrow.

Just run the enclosure thru llc and the failure should be apparent.

bad.ll72 KBDownload

Ok, so. I looked really hard, and essentially i'm not sure we can just change those attributes from implying readonly/readnone.
Things kinda just fall apart

In rGf8d9097168b73e3befd94ac4e16cbc5f944afcec#1165266, @srj wrote:

This change has injected a failure in Halide's CUDA testing code -- our correctness_predicated_store_load test now fails with

correctness_predicated_store_load: .../DAGCombiner.cpp:23378: llvm::SDValue combineShuffleOfSplatVal(llvm::ShuffleVectorSDNode*, llvm::SelectionDAG&): Assertion `MinNonUndefIdx < NumElts && "Undef shuffle?"' failed.

It's not clear to me whether this is indicative of a a defect in this change (vs a pre-existing bug in Halide that was surfaced by this change) -- advice welcome, but a revert pending investigation would be welcome.

(But since the same IR was passing the validation checks inside LLVM prior to this change, it seems likely that there's a subtle defect in this change.)

Adding more docs as discussed. Further suggestions welcomed.

@RKSimon just so we can save everyone's time, can you please explain how you request this to look?

Ok, if we must not unconditionally emit the memory attributes, then let's not.
Please stamp? :)