Page MenuHomePhabricator

void (Bill Wendling)
Mind Taker

Projects

User does not belong to any projects.

User Details

User Since
Dec 3 2012, 10:22 AM (314 w, 5 d)

Sent here from the planet Zvddw, Bill's mission is to conquer the world by staying at home and occasionally (read: always) playing poker.

In his spare time, he works on LLVM-related things.

Recent Activity

Thu, Dec 13

void added a comment to D55616: Emit ASM input in a constant context.

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

Thu, Dec 13, 4:12 PM
void updated the diff for D55616: Emit ASM input in a constant context.

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

Thu, Dec 13, 4:11 PM

Wed, Dec 12

void added a comment to D55616: Emit ASM input in a constant context.

Addressed Eli's comment.

Wed, Dec 12, 2:43 PM
void updated the diff for D55616: Emit ASM input in a constant context.

Don't look at the optimization level here.

Wed, Dec 12, 2:42 PM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

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

Wed, Dec 12, 2:12 PM
void added a comment to D55616: Emit ASM input in a constant context.

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

Wed, Dec 12, 2:00 PM
void created D55616: Emit ASM input in a constant context.
Wed, Dec 12, 2:00 PM
void updated subscribers of D55616: Emit ASM input in a constant context.
Wed, Dec 12, 2:00 PM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

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.

Wed, Dec 12, 11:19 AM

Mon, Dec 10

void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

I'm seeing a case where an inline assembly 'n' constraint is behaving differently in -O0 now. Previously we would evaluate the builtin_constant_p as part of the EvaluateAsInt call in CodeGenFunction::EmitAsmInput, but I think we're not doing that now. This causes us to fail later because the constraint wasn't satisfied since we were in -O0 and thus we didn't optimize the rest of the expression. The test case we have is a nasty mix of builtin_constant_p, builtin_choose_expr, and builtin_classify_type. I can see about sharing it if needed.

Mon, Dec 10, 9:09 PM

Wed, Dec 5

void abandoned D55311: Add support for OUTPUT_ARCH linker script command.
Wed, Dec 5, 3:57 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Sorry, I'm really confused. Could you explain again for me why you need this patch to link Linux kernel?

Wed, Dec 5, 3:49 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

What is the motivation to do this? We don't try too hard to implement every detail of the linker script because it's just too complicated and there is usually an easy workaround that works without a linker script.

The motivation is to support linking Linux, which uses linker scripts heavily. I added a comment by IanT from another thread about why this is a good addition.

Now I remember. We saw this bug earlier and decided to report an issue to linux kernel and not change the linker.
Bug reported here: https://bugzilla.kernel.org/show_bug.cgi?id=194091.

Wed, Dec 5, 3:23 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Looks like what you are trying to do is to generate a x86-64 executable from i386 object files. That's what we do not expect in lld. With this patch, in theory you can create AArch64 exectuables from x86-64 object files, but that doesn't make sense and likely to crash the linker because we assume that all object files are uniform in terms of target types.

I think this use case is too tricky to directly support in the linker. I'd probably use objcopy or something to transplant i386 code to x86-64 object file before passing it to the linker, so that we don't deal with the trickiness in the linker.

Wed, Dec 5, 3:20 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Even with this patch, don't you still have to make a change to lld so that it accepts a set of input files of different targets?

Wed, Dec 5, 3:02 PM
void added inline comments to D55311: Add support for OUTPUT_ARCH linker script command.
Wed, Dec 5, 2:12 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

What is the motivation to do this? We don't try too hard to implement every detail of the linker script because it's just too complicated and there is usually an easy workaround that works without a linker script.

Wed, Dec 5, 12:58 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

What is your use case? I wonder what is the reason to use OUTPUT_ARCH to override -m?

It was pointed out that the "-m" flag has slightly different semantics than what we would expect:

Wed, Dec 5, 12:57 PM
void added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Note: I tried to add as many BFD arch entries as I could muster from binutils. It's obvious not complete, so any further entries you think should be here let me know.

Wed, Dec 5, 12:41 AM
void created D55311: Add support for OUTPUT_ARCH linker script command.
Wed, Dec 5, 12:27 AM

Sat, Dec 1

void committed rC348071: Correct indentation..
Correct indentation.
Sat, Dec 1, 1:09 AM
void committed rL348071: Correct indentation..
Correct indentation.
Sat, Dec 1, 1:09 AM
void committed rC348070: Specify constant context in constant emitter.
Specify constant context in constant emitter
Sat, Dec 1, 12:33 AM
void committed rL348070: Specify constant context in constant emitter.
Specify constant context in constant emitter
Sat, Dec 1, 12:33 AM

Fri, Nov 30

void committed rL348032: Revert r348029. I was git-ing and jumped the gun..
Revert r348029. I was git-ing and jumped the gun.
Fri, Nov 30, 12:47 PM
void committed rC348032: Revert r348029. I was git-ing and jumped the gun..
Revert r348029. I was git-ing and jumped the gun.
Fri, Nov 30, 12:47 PM
void committed rL348029: We're in a constant context in the ConstantEmitter..
We're in a constant context in the ConstantEmitter.
Fri, Nov 30, 12:43 PM
void committed rC348029: We're in a constant context in the ConstantEmitter..
We're in a constant context in the ConstantEmitter.
Fri, Nov 30, 12:43 PM
void added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

Actually - as Peter suggested there, this is in fact fixed by a more recent version of gold. The gold I use with llvm regression tests (GNU gold (GNU Binutils 2.30.51.20180214) 1.16) fixes this issue:

softirq.o: early_irq_init: PREEMPTED_IR
irqdesc.o: early_irq_init: PREVAILING_DEF_REG

$ nm test-object.o | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001a0 W arch_early_irq_init
0000000000000190 W arch_probe_nr_irqs
$

My installed version of gold (1.15) has the bug. So please do use a more recent version of gold to fix this issue.

Fri, Nov 30, 11:13 AM
void added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

Fri, Nov 30, 11:10 AM

Thu, Nov 29

void added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

Thu, Nov 29, 5:52 PM
void added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

Hi @piramam,

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

This patch fixed a longstanding break with ThinLTO + PGO + LLD. If the current issue is easy to fix, I'd prefer to fix forward rather than revert this patch. Can you share details on what fails - error message or crash?

But ultimately, this is @tejohnson's call to revert or fix forward.

Thu, Nov 29, 5:49 PM
void reopened D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

This change is breaking the Linux ThinLTO build. I'll work to get you a test case. Could you revert this patch until we can resolve the issue?

Thu, Nov 29, 5:32 PM
void accepted D54964: Add test about __builtin_constant_p.
Thu, Nov 29, 10:17 AM

Sun, Nov 25

void committed rC347531: A "constexpr" is evaluated in a constant context. Make sure this is reflected.
A "constexpr" is evaluated in a constant context. Make sure this is reflected
Sun, Nov 25, 6:14 PM
void committed rL347531: A "constexpr" is evaluated in a constant context. Make sure this is reflected.
A "constexpr" is evaluated in a constant context. Make sure this is reflected
Sun, Nov 25, 6:14 PM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Fixed in r347531.

Sun, Nov 25, 6:14 PM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Doh! I'll take a look at it.

Sun, Nov 25, 4:28 PM

Sat, Nov 24

void committed rC347512: isEvaluatable() implies a constant context..
isEvaluatable() implies a constant context.
Sat, Nov 24, 2:49 AM
void committed rL347512: isEvaluatable() implies a constant context..
isEvaluatable() implies a constant context.
Sat, Nov 24, 2:49 AM

Thu, Nov 22

void committed rC347480: A __builtin_constant_p() returns 0 with a function type..
A __builtin_constant_p() returns 0 with a function type.
Thu, Nov 22, 3:01 PM
void committed rL347480: A __builtin_constant_p() returns 0 with a function type..
A __builtin_constant_p() returns 0 with a function type.
Thu, Nov 22, 3:01 PM
void committed rC347446: The result of is.constant() is unsigned..
The result of is.constant() is unsigned.
Thu, Nov 22, 1:34 AM
void committed rL347446: The result of is.constant() is unsigned..
The result of is.constant() is unsigned.
Thu, Nov 22, 1:34 AM

Wed, Nov 21

void committed rCTE347419: Update call to EvaluateAsInt() to the new syntax..
Update call to EvaluateAsInt() to the new syntax.
Wed, Nov 21, 12:48 PM
void committed rL347419: Update call to EvaluateAsInt() to the new syntax..
Update call to EvaluateAsInt() to the new syntax.
Wed, Nov 21, 12:48 PM
void committed rL347418: Update call to EvaluateAsInt() to the new syntax..
Update call to EvaluateAsInt() to the new syntax.
Wed, Nov 21, 12:48 PM
void committed rLLDB347418: Update call to EvaluateAsInt() to the new syntax..
Update call to EvaluateAsInt() to the new syntax.
Wed, Nov 21, 12:48 PM
void committed rL347417: Re-Reinstate 347294 with a fix for the failures..
Re-Reinstate 347294 with a fix for the failures.
Wed, Nov 21, 12:48 PM
void committed rC347417: Re-Reinstate 347294 with a fix for the failures..
Re-Reinstate 347294 with a fix for the failures.
Wed, Nov 21, 12:48 PM

Tue, Nov 20

void committed rCTE347366: Update EvaluateAsInt to the new syntax..
Update EvaluateAsInt to the new syntax.
Tue, Nov 20, 3:27 PM
void committed rL347366: Update EvaluateAsInt to the new syntax..
Update EvaluateAsInt to the new syntax.
Tue, Nov 20, 3:27 PM
void committed rC347364: Reinstate 347294 with a fix for the failures..
Reinstate 347294 with a fix for the failures.
Tue, Nov 20, 3:27 PM
void committed rLLDB347365: Update call to EvaluateAsInt() to the new syntax..
Update call to EvaluateAsInt() to the new syntax.
Tue, Nov 20, 3:27 PM
void committed rL347365: Update call to EvaluateAsInt() to the new syntax..
Update call to EvaluateAsInt() to the new syntax.
Tue, Nov 20, 3:27 PM
void committed rL347364: Reinstate 347294 with a fix for the failures..
Reinstate 347294 with a fix for the failures.
Tue, Nov 20, 3:27 PM
void committed rL347294: Use is.constant intrinsic for __builtin_constant_p.
Use is.constant intrinsic for __builtin_constant_p
Tue, Nov 20, 12:56 AM
void committed rC347294: Use is.constant intrinsic for __builtin_constant_p.
Use is.constant intrinsic for __builtin_constant_p
Tue, Nov 20, 12:56 AM
void closed D54355: Use is.constant intrinsic for __builtin_constant_p.
Tue, Nov 20, 12:56 AM

Mon, Nov 19

void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Should be okay now. PTAL. :-)

Mon, Nov 19, 3:20 PM
void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

Remove ODR violation.

Mon, Nov 19, 2:54 PM

Sun, Nov 18

void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

No function pointers

Sun, Nov 18, 5:32 PM
void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

Don't re-wrap a ConstExpr.

Sun, Nov 18, 1:57 AM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Ping? I really don't want this review to go on forever.

Sun, Nov 18, 1:45 AM

Nov 15 2018

void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Ping? :-)

Nov 15 2018, 11:26 PM

Nov 13 2018

void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

ImpCast.

Nov 13 2018, 9:48 PM
void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

Fix overflow flag.

Nov 13 2018, 6:25 PM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

I think this is ready now. PTAL.

Nov 13 2018, 5:58 PM
void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

Updated to address comments.

Nov 13 2018, 5:56 PM
void added inline comments to D54355: Use is.constant intrinsic for __builtin_constant_p.
Nov 13 2018, 2:51 PM
void added inline comments to D54355: Use is.constant intrinsic for __builtin_constant_p.
Nov 13 2018, 1:11 PM
void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

Remove some extraneous checks.

Nov 13 2018, 1:09 PM
void added a comment to D54356: Convert CheckICE into a statment visitor.

Okay. I'll revert this then.

I don't think we necessarily need the change in the other patch that's based on this one, but I still think this refactoring is an improvement :)

Nov 13 2018, 1:06 PM
void abandoned D54356: Convert CheckICE into a statment visitor.

This isn't necessary. We can assume it's in a constant context because it's checking for an ICE.

Nov 13 2018, 1:02 PM
void added a comment to D54356: Convert CheckICE into a statment visitor.

Okay. I'll revert this then.

Nov 13 2018, 12:54 PM
void added a comment to D54356: Convert CheckICE into a statment visitor.

This code is called in over 90 places, so it's hard to tell if they all are in a constant context. Though I suppose that what this code is supposed to check for would work the same in a constant context as without one. I can revert this if you want, but to be honest the original function was terrible--it's huge and hard to understand what's going on. As for adding new expressions, this isn't the only place where a StmtVisitor is used. One still needs to go through all of those visitors to see if they need to add their expression.

Nov 13 2018, 12:41 PM
void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Ping?

Nov 13 2018, 10:35 AM
void added a comment to D54356: Convert CheckICE into a statment visitor.

Ping?

Nov 13 2018, 10:35 AM

Nov 9 2018

void added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

This has D54356 integrated into it. D54356 should be reviewed and submitted first, even though it's out of order.

Nov 9 2018, 2:24 PM
void updated the diff for D54355: Use is.constant intrinsic for __builtin_constant_p.

Adding ConstantExpr visitor.

Nov 9 2018, 2:18 PM
void added a parent revision for D54355: Use is.constant intrinsic for __builtin_constant_p: D54356: Convert CheckICE into a statment visitor.
Nov 9 2018, 2:15 PM
void added a child revision for D54356: Convert CheckICE into a statment visitor: D54355: Use is.constant intrinsic for __builtin_constant_p.
Nov 9 2018, 2:15 PM
void created D54356: Convert CheckICE into a statment visitor.
Nov 9 2018, 2:15 PM
void created D54355: Use is.constant intrinsic for __builtin_constant_p.
Nov 9 2018, 2:08 PM

Nov 8 2018

void committed rCTE346467: Remove unused c'tor..
Remove unused c'tor.
Nov 8 2018, 6:06 PM
void committed rL346467: Remove unused c'tor..
Remove unused c'tor.
Nov 8 2018, 6:05 PM
void committed rCTE346461: Ignore implicit things like ConstantExpr..
Ignore implicit things like ConstantExpr.
Nov 8 2018, 5:35 PM
void committed rL346461: Ignore implicit things like ConstantExpr..
Ignore implicit things like ConstantExpr.
Nov 8 2018, 5:35 PM
void committed rC346458: Use correct parameter name in comment..
Use correct parameter name in comment.
Nov 8 2018, 5:07 PM
void committed rL346458: Use correct parameter name in comment..
Use correct parameter name in comment.
Nov 8 2018, 5:07 PM
void committed rL346455: Compound literals, enums, et al require const expr.
Compound literals, enums, et al require const expr
Nov 8 2018, 4:44 PM
void committed rC346455: Compound literals, enums, et al require const expr.
Compound literals, enums, et al require const expr
Nov 8 2018, 4:44 PM
void closed D53921: Compound literals, enums, et al require const expr.
Nov 8 2018, 4:44 PM
void closed D53921: Compound literals, enums, et al require const expr.
Nov 8 2018, 4:44 PM
void added a comment to D53921: Compound literals, enums, et al require const expr.

I'm using "arcanist" for the first time, and I don't want to mess things up. I'll address the comments here in the next patch.

Nov 8 2018, 4:28 PM
void added inline comments to D53921: Compound literals, enums, et al require const expr.
Nov 8 2018, 3:37 PM

Nov 2 2018

void updated the diff for D53921: Compound literals, enums, et al require const expr.
Nov 2 2018, 12:12 AM

Nov 1 2018

void updated the diff for D53921: Compound literals, enums, et al require const expr.
Nov 1 2018, 7:44 PM
void updated the diff for D53921: Compound literals, enums, et al require const expr.

I believe this patch should address most, if not all, of your concerns. PTAL.

Nov 1 2018, 5:15 PM
void updated the diff for D53921: Compound literals, enums, et al require const expr.
Nov 1 2018, 11:50 AM