I'm a bit puzzled by the need for this change. The bump allocator already has logic to do power-of-two scaling for the allocation, so I wonder why it doesn't work properly here.

Adjust based on comments.

Why go back to the large tables for crc32? Just because JamCRC had that bug doesn't mean it should persist.

I wonder if we should actually enumerate evil here, i.e. give the situations in which inlining actually fails. As mentioned on IRC, I wonder if we shouldn't aim for the stronger semantics and at least warn by default of any situation that prevents always_inline from doing its job.

I'd make the argument that it should be strlcpy if present. That covers a lot of existing systems too. I'm not sure this optimisation should be done for freestanding mode though.

2nd ping. Chandler, care to check this please?

Ping.

Ping

LGTM

Can you declare a volatile variable and set that from f instead?

Switch the pass to use two rounds. The first round will collect all relevant intrinsics in RPO, the second one will translate them accordingly.

Apply feedback.

Chandler, are you OK with getting the InstructionSimplify.h part in now, so that it can be merged into 9.0 and the rest follow separately?

It would be interesting to get an actual backtrace. It should not be a DWARF register number, but it might be from a direct caller of the unw interface.

At least for freestanding environments, it would be useful to separate nonnull completely from deferencable. GCC has a separate flag for it, which might also be a reasonable idea.

Hook up a second instance of the pass after the Float To Int pass for optimized builds. This is after the initial loop transforms, so it can profit from some unrolling, but it is before vectorization. The late run of the pass should is kept for now and ensures any potentially added variants are still dropped before SDAG.

It would be nice to also default to this for -ffreestanding.

Simplify the pass logic. First round will update the predecessor links and note if any block is orphaned, a second round will remove unreachable blocks if necessary.
The pass can be further refined to use and update DominatorTree and AssumptionCache incrementally, but this should be functionally complete now. It will not handle more complex cases like orphaned loops, but I don't think those are commonly used with is.constant or objectsize conditions either.
The pass will now scan every BB once, but fall back to the start of a BB of recursive removal invalidates the iterator. This seems to be the strictest form I can manage.

Please read the discussion. The general consensus was and likely is that this is at least somewhat bogus in the standard at best and the cost of not artificially breaking code is much higher than the benefit. Yes, GCC made different choices, but that's no excuse.

Take a look at the list archives for the discussion about attribute nonnull and nullable a while ago. The short version is that the library interface specification might have a wildcard clause that NULL pointers are invalid unless explicitly allowed, but it works perfectly fine on any sensible implementation except the ds9k. When the combination of glibc annotations and aggressive gcc optimizations started, it pointlessly broke a lot of code for no good reason.

Thanks, this looks good enough within the limitations of the framework.

Looking a bit more into the details. Chandler, you've originally suggested going with the LowerAtomic route and that actually does create code that fails the SDAG lowering if the pass is skipped, e.g. on ARM.
The second part is currently overlapping with the CodeGenPrepare pass. I can cleanup the implementation somewhat by reusing the same functionality as that pass is using OR I could factor out a minimal branch for doing the constant folding optimization from CodeGenPrepare as a general branch that is included in the pass chain for -O0, e.g. instead of the more general CodeGenPrepare pass. The main difference is that the non-optimized build would not get any recursive folding from PHI-simplification, but I think that's fine for the original use case. It would also not get the block merging, but again, that seems to be fine for the constraints.

For the first part, I was actually asked to do that. I don't mind either way.

Generalize slightly to also cover llvm.objectsize which has very similar constraints.

The combination of D60942, D06943 and D65280 solves the problems for me on all targets I have.

Update PHI nodes in disconnected block.

Replace boolean argument to replaceAndRecursivelySimplify with optional vector of un-modified instructions. This simplifies the API change significantly and allows other potential use cases. Redo the restart handling. After a successful simplification step, restart the current BB only, but always do another full pass of the function.

Avoid goto. Create new BranchInst instead of modifying in-place. Update tests to reflect changes. Move most of the x86 is-constant test to generic.

LGTM otherwise.

With the exception of the small improvements outlined, this looks good to me.

I understand, but the current version just doesn't work anyway to delay it.

You lost the changes to lib/Sema/SemaStmtAsm.cpp to actually do the delaying for immediate operands?

I think MMX code is obscure enough at this point that it doesn't matter much either way. Even less across DSO boundaries. That's why I don't really care either way.

The patch still needs to be generalized to handle all ICE constraints, but it is not blocked by the review for handling the is.constant intrinsic.

In D60942#1500742, @void wrote:

In D60942#1500730, @joerg wrote:

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

On the frontend side:

• __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
• The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

It does not fix the issues on our side, but pushes them to a different place. It is still an improvement, but the problem is not solved yet.

I'm in the process of testing this, but feedback will take a bit.

In D59816#1442909, @evgeny777 wrote:

don't bother with the 1KB table in both binary and source, just recompute it on the first use

Is single threaded usage guaranteed?

I'd recomment copying the version from libarchive (https://github.com/libarchive/libarchive/blob/master/libarchive/archive_crc32.h):

• don't bother with the 1KB table in both binary and source, just recompute it on the first use.
• at least in the past unrolling the inner loop somewhat helped a lot

LGTM from my perspective at least.

For it to be really useful for the majority of bugs, it would be nice to figure out automatically how to get the preprocessing step done and filter out the # lines afterwards. That part alone significantly cuts down the creduce time.

Why do you exclude clang? I would expect LLVM at this point to not support visibility attributes on XCOFF either?

Well, that was a sample to illustrate the point. A full working (and now failing) example is:

The other problem is that we don't use the CFG machinery to prune dead branches. Consider the x86 in/out instructions: one variant takes an immediate, the other a register. The classic way to deal with that is something like

Can you include a patch for something like (int *)0xdeadbeeeeeef on amd64? That's a valid value for "n", but clearly too large for int. Thanks for looking at this, it is one of the two large remaining show stoppers for the asm constraint check.

Placing .bss.rel.ro before .data doesn't make sense. It forces the content of .bss.rel.ro to embedded into the binary. I also don't really understand the motivation here. Memory mappins on the kernel side are quite cheap.