In D38479#1507435, @phosek wrote:

We (Fuchsia) would like to see this landed as well so we can start using this in our kernel.

In D60942#1500749, @joerg wrote:

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.

Yes, that's correct. The problem is that with -O0, we never run SimplifyCFG, so those conditional branches make it straight to the backend. As discussed with Richard on IRC, the core issue is that for -O0, the IR generation for __builtin_constant_p is different from the constant folding as part of AST/EvalConstant.cpp. The latter is used by EmitIfStmt and friends to decide whether the BBs should be created in first place and currently fails evaluation for bcp.

On the backend side:

• The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

With -O0, the intrinsic is never generated and no SimplifyCFG is run either (see above).

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.

In D60942#1477583, @joerg wrote:

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.

Remove errant space.

In D61719#1496270, @t.p.northover wrote:

Could you add a test?

Add testcase.

This is the doc saying dword == xword:

In D58260#1484879, @efriedma wrote:

In D58260#1483021, @void wrote:

Right. My point is that it's equivalent to passing a pointer around, except that the pointer cannot be dereferenced anywhere except the originating function. (For the purposes of what Nick's trying to do, "originating function" includes code that's inlined into a function.) As long as IPSCCP and the like don't violate that constraint, passing the pointer around isn't going to cause semantic changes to the program. (A function that tries to jump to a block address that's not within that function is already undefined and still should be.)

Oh, you're saying that if we're inlining a function that contains an indirectbr, but we aren't inlining the indirectbr, we can allow inlining as long as we as the blockaddress still points to the original function. That's abstractly reasonable, but it probably breaks the Linux kernel's _THIS_IP_ macro (and therefore, I'm guessing, isn't what gcc does).

In D58260#1482993, @efriedma wrote:

But should it?

It seems like a reasonable optimization, depending on the code structure.

That implies that a blockaddress that's propagated to other functions aren't used by anything other than arithmetic and comparison instructions.

The important thing is that it can also be returned back to the original function, either directly, or indirectly through memory.

In D58260#1474884, @efriedma wrote:

that's a case I don't think can be done in C even with GNU C extensions

It can't be written explicitly in C, sure... but optimizations like IPSCCP can propagate blockaddress constants to other functions.

This looks like a nice cleanup. Accepting unless Peter has any objections.

In D60943#1474926, @rsmith wrote:

In D60943#1474899, @void wrote:

Here's the motivating bug report: https://bugs.llvm.org/show_bug.cgi?id=41027

Thanks, that's illuminating. OK, if we want to support that code, then there's really not much validation we can do in the frontend for constraints that require immediate constants. Is "n" really special in this regard, or is it just the first one that we've encountered?

Here's the motivating bug report: https://bugs.llvm.org/show_bug.cgi?id=41027

In D60943#1474091, @joerg wrote:

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

On the more meaty parts of this change, I think further iterations will be necessary in-tree to extend this to the other constraints.

This relies upon D60943.

Add testcase.

Fix testcase to get the correct error message.

Fix test. Use "e" instead of "n".

Put constraint string check in the correct place.

In D56571#1467358, @craig.topper wrote:

I think things don't work right unless you disable the integrated assembler. I'm not sure why though.

My opinion doesn't carry as much weight as others who are more familiar with the front-end code, but LGTM.

Move checks over to the main LL file, not Input file.

@pcc: Ping? :)

Another ping. Anyone? :-)

Friendly ping. :-)

In D56508#1397173, @rupprecht wrote:

In D56508#1397156, @void wrote:

That should be fixed by D57842, which I'm planning to submit soon -- I want to poke a few more test cases first.

This fails if the archives are nested. E.g.:

Ping? :-)

In D57267#1390484, @riccibruno wrote:

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

LGTM, but may want to wait for other reviewers.

Move test to proper place and add "-verify" flag.

I encountered another ICE here: https://github.com/ClangBuiltLinux/linux/issues/320#issuecomment-455456506

There appears to be an ICE with this patch: https://github.com/ClangBuiltLinux/linux/issues/6#issuecomment-454675962

I like these changes. Could you add the documentation for the new instruction?

Apologies - the value seems to indeed overflow, but I'm still very confused how this was affected by this change.

Implement Rui's suggestion for how to output the ELF files.

Add check that the object file is still valid after using "obj-path".

In D56046#1341515, @MaskRay wrote:

The filenames of Task=0 and different. Is this preferable?

t.o t2.o were compiled with -module-summary

ld.lld --plugin-opt=obj-path=t3.o -shared t.o t2.o => creates t2.o0 t2.o1 t2.o2

gold --plugin=~/llvm/Release/lib/LLVMgold.so --plugin-opt=thinlto --plugin-opt=obj-path=t3.o -shared t.o t2.o => creates t2.o t2.o1 t2.o2

Don't append "0" to the object filename.

Add testcase.

In D56046#1341491, @ruiu wrote:

Replicating the gold behavior sounds fine, but does this patch work? This change seems to leave Buf uninitialized while Buf will be used later. Also, in gold LTO plugin, -obj-path overwrites -save-temps, but this change doesn't seem to replicate that behavior. Could you add tests?

Ping? :-)

In D55616#1335587, @vitalybuka wrote:

Looks like it's broken by this patch

clang: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/lib/AST/ExprConstant.cpp:11055: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext &, SmallVectorImpl<clang::PartialDiagnosticAt> *) const: Assertion `Result && "Could not evaluate expression"' failed.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/9281/steps/check-clang%20msan/logs/stdio

Fix how prefixes are used in the testcase.

Reduce duplication by using multiple check prefixes.

Ping? :-)

In D55616#1329301, @jyknight wrote:

This seems like a good start, but not complete.

"n" and "i" both should require that their argument is a constant expression. For "n", it actually must be an immediate constant integer, so setRequiresImmediate() should be used there. For "i", you may use an lvalue constant as well. The way we seem to indicate that, today, is with if (!Info.allowsRegister() && !Info.allowsMemory()). However the code in that block does not today *require* that the evaluation as a constant succeed...and it should.

It should also only require that the result is an integer when requiresImmediateConstant().

Additionally, the Sema code needs to have the same conditions as the CodeGen code for when a constant expression is required, but it doesn't. (before or after your change).

The n constriant *requires* a known integer. Therefore, enforce this in both
Sema and CodeGen by setting the "requires immediate" flag and evaluating to a
known integer instead of random "int"..

Addressed Eli's comment.

Don't look at the optimization level here.

In D54355#1328786, @jyknight wrote:

In D54355#1328557, @void wrote:

The issue is that "n" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of __builtin_constant_p shouldn't be delayed (it being compiled at -O0). I suspect that this issue will also happen with the "i" constraint.

Indeed. We _do_ actually already know that in clang too, however -- clang already knows that "n" requires an immediate, and does constant expression evaluation for it, e.g. to expand constexpr functions. Grep for "requiresImmediateConstant". I guess it's just not hooked up to the __b_c_p correctly, though.

(Note, as a workaround, you could use | instead of ||, or put the expression as the value of an enumeration constant).

As a side note, the number of ways to evaluate a constant expression are legion in clang. They should really be unified...