This is enabled only when -Os is specified?

LGTM

@reames
I'm a little bit concerned about the performance impact of this patch. Do you have any data for the patch? Is there any performance penalty and how much code size is reduced?

The patch LGTM. I'd like to see the patch land before the llvm release.

Ping @ruiu

@ruiu,
Can we submit the patch?

In D71442#1789052, @pengfei wrote:

Ping.

I've gone as far as writing a rough draft of textual assembler support. If folks agree this is helpful, I'll abandon this patch, and post one in that direction. Any high level suggestions as to syntax? I see two major options, but definitely welcome suggestions w.r.t. naming/semantics/etc...

Option 1

.boundary_align 4
jmp foo

and

.boundary_align 4
.bundle_lock
test ...
jcc foo
.bundle_lock

Option 2

Option 1

.boundary_align 4
jmp foo
.end_boundary_align

and

.boundary_align 4
test ...
jcc foo
.end_boundary_align

p.s. If anyone has a better name than "boundary align" please suggest it. That's not a good name; it's just a placeholder.

In D70157#1773180, @reames wrote:

Recording something so I don't forget it when we get back to the prefix padding version. The write up on the bundle align mode stuff mentions a concerning memory overhead for the feature. Since the basic implementation techniques are similar, we need to make sure we assess the memory overhead of the prefix padding implementation. See https://www.chromium.org/nativeclient/pnacl/aligned-bundling-support-in-llvm for context. I don't believe this is likely to be an issue for the nop padding variant.

I'm currently preparing the patches on the clang side in addition to an update to cfe-dev. Please stay tuned and we would really appreciate any feedback there.

In the original RFC, we sketched the C/C++ support we envisioned using builtins. A simple example that loads two 4x4 matrixes, multiplies them, adds a third matrix to the result and stores the it can be found in the code below. Our initial proposal is quite stripped down and intended to be exposed to end users via something like a C++ matrix wrapper class.

typedef float m4x4_t __attribute__((matrix_type(4, 4)));
 
 
void f(m4x4_t *a, m4x4_t *b, m4x4_t *c, m4x4_t *r) {
  *r = __builtin_matrix_add(__builtin_matrix_multiply(*a, *b), *c);
}

Interesting. Do you have any patch for the C/C++ frontend? What does the C/C++ code look like?

In D67210#1659439, @RKSimon wrote:

rL371078 should fix this

In D67210#1659230, @RKSimon wrote:

Please abandon this, it isn't a valid solution to the issue (raised at PR43227). I have a WIP fix that will address this correctly.

@anton-afanasyev, I have no objection. Thank you for the effort to improve the performance.

Update the patch corresponding to Reid's comments.

@anton-afanasyev
Hi,
Do you have any performance data for the patch? I'd like to know what benchmark has performance gain with your patch. https://reviews.llvm.org/D64394 fixed perlbench regression, but I wonder what the performance gain do we achieve with the 2 patch?

@anton-afanasyev
Hi,
Did you look into the SPEC cpu2017/500.perlbench_r issue? There is some significant performance drop on X86 with the patch. I ask you to revert the patch first, and when the SPEC2017 regression is fixed, we can submit the patch again. How do you think?

In D60437#1462731, @hans wrote:

Okay, let's not worry too much about the test case, the patch seems obviously good.

Do you have commit access, or would you like me to commit for you?

In D60437#1462660, @hans wrote:

In D60437#1462644, @LuoYuanke wrote:

In D60437#1462379, @hans wrote:

In D60437#1462279, @LuoYuanke wrote:

In D60437#1461210, @hans wrote:

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

Thank you for the suggestion. I disable the code of xmm0 - xmm7 mapping and there is no case failure in llvm/tests/DebugInfo/COFF/. However when I disable general register (rax -r15), some of the test cases (.e.g test/DebugInfo/COFF/types-basic.ll) fail. There is pretty much code for the failed test case which I'm investigating. To create a small test case, we may need to cause much pressure on register allocator for mm0 - mm7 with __m64 variable in c code. However I have not figured out how to created such case.
Nevertheless, does anybody know why the mm0-mm7 registers are not mapped. Is there any reasons on it?

I'm guessing it's just an oversight.

Would it be possible to use inline assembly to trigger use of these registers in some way?

If this turns out to be hard, I think we can just check in your patch as is, it seems like an obvious fix.

In D60437#1462379, @hans wrote:

In D60437#1462279, @LuoYuanke wrote:

In D60437#1461210, @hans wrote:

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

In D60437#1461210, @hans wrote:

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

@jyknight
I don't have permission to commit the patch. Could you help to commit it? Thanks.

In D60039#1458242, @jyknight wrote:

If you change to condition on GLIBC instead of linux, I think this is fine to commit (although it still seems unfortunate to me that we're carrying an abandoned codebase which depends on weird broken stuff like this...)

Fix the issue only when GLIBC is defined.

Use getcontext() only on linux platform.

In D60039#1453614, @jyknight wrote:

I would question whether it's actually worth it to attempt to keep software working that seems to have been abandoned for a decade as part of the llvm test suite? Should this code just be removed, instead of patched?

I'd note that not all platforms support getcontext -- at least Windows and OpenBSD don't. I don't actually know if this is expected to work on those platforms, but it definitely won't after this change.

In D60039#1450876, @craig.topper wrote:

I believe this is saving rbp but mangling it while doing so in a reversible way.

0x00007ffff6f93fa3 <+3>:     mov    %rbp,%rax
0x00007ffff6f93fa6 <+6>:     xor    %fs:0x30,%rax
0x00007ffff6f93faf <+15>:    rol    $0x11,%rax
0x00007ffff6f93fb3 <+19>:    mov    %rax,0x8(%rdi)

In D60039#1450808, @craig.topper wrote:

Are sure RBP isn't in the jmp buffer? There's a macro used by setjmp called PTR_MANGLE that can xor the pointer with another variable which can obscure the value. It gets xored again in longjmp to demangle it. But in either case its not a good idea to examine the contents of jmp_buf so I think this is the right fix.

Hi
What's the status for __float128 support? Has it already been finished?

@rnk
Do you agree to just fix the " X86ISD::WrapperRIP TargetGlobalTLSAddress" issue in this patch? Do you have some other concern of comments for the patch?

Add "FIXME" for tls compiled with pic but without pie option.

Fix typo of comments.

Hi ruiu

I think SPLT has better compatibility compared to change the original PLT entry. Given the platform is old, and use the old dynamic linker, the new library that build with SPLT can still be linked by dynamic linker. I am not sure if it still works if we change the struct of PLT entry.
The second issue is GNU toolchain has already adopt SPLT, if we want to change the design, we need also influence the GNU community to revise the design and all their libraries which are built with CET enabled toolchain.

Hi rnk,
Could you help to review? Thanks

Thanks. I abandon the review and create a new review at https://reviews.llvm.org/D58336.

Hi Johnson,

Add context for the patch.

The case will randomly fail if we test it with command "
while llvm-lit test/tools/gold/X86/cache.ll ; do true; done". It is because the llvmcache-foo file is younger than llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6. But due to timestamp precision reason their timestamp is the same. Given the same timestamp, the file prune policy is to remove bigger size file first, so mostly foo file is removed for its bigger size. And the files size is under threshold after deleting foo file. That's what test case expect.