In D28141#4316637, @rnk wrote:

In D28141#4143347, @wxiao3 wrote:

@rnk do you have plan to bring this back?

As far as I can tell, the issue is currently fixed, see the usage of .section .rdata and .long directives from llc here: https://gcc.godbolt.org/z/1znoroP74

In D144701#4157971, @mstorsjo wrote:

Thanks for the updates!

On second look, I'm a bit undecided and divided whether it's better to have the arch switch, or just keep it as it was (or include aarch64 in the switch too?). I guess it's fine with me either way. (And the aarch64 and arm cases should have their own testcases already, I believe.)

For the testcase, can you include i386 or i686 too, since that's at least covered in the switch?

restrict the change only to x86_64

In D144701#4157624, @mstorsjo wrote:

Note that this behaviour isn't entirely strictly uniform across all COFF targets. See D57277, where the AArch64 target was changed to move jumptables on COFF to the text section. This change was then later reversed in D113576 when the AArch64/COFF target supported the relevant relocations.

Before this change, COFF on x86_32 and aarch64 put the jumptables in rdata, while x86_64 and arm (use thumbv7 in triples passed to llc) put them in the text section (in the case of arm, the jumptable is emitted inline in the function and not after it). This patch seems to change the output of x86_64, but doesn't affect the others. I think it would be good to clarify this aspect in the commit message and wording. I'm mildly concerned about how it affects other targets when this is universal code for COFF, but at least on a quick test it didn't make any difference for the arm case (the only one where the jump tables would be left in the text section).

The testcase here seems a bit brittle; if I run this testcase with an aarch64-pc-win32 triple, I don't get any jumptable generated at all, since it can mostly be optimized out. See e.g. llvm/test/CodeGen/AArch64/win64-jumptable.ll for a less brittle testcase.

1. only affect x86
2. less brittle testcase

RUN to check the added option

@rnk do you have plan to bring this back?

LGTM.

Address Phoebe's comments.

Address Simon's comments.

Address Phoebe's comments.

update the comments.

Address comments from Craig.

In D106408#3021110, @rampitec wrote:

I still consider this a good experiment as it has uncovered couple RA bugs in the process.

Obviously 0.01 there is a pure guesstimate.

To fix one of my local performance regressions, I need to set it to 0.1 at least.
But tuning the value seems to be meaningless. Because:
Without your patch, isReallyTriviallyReMaterializableGeneric return true means that the VR is definitely ReMaterializable.
With your patch, isReallyTriviallyReMaterializableGeneric return true means that the VR is probably ReMaterializable.
I don't think the change is consistent with this function's orignal design goal.
Moreover, your patch conflicts with our target implementation: X86InstrInfo::isReallyTriviallyReMaterializable (llvm/lib/Target/X86/X86InstrInfo.cpp). E.g., for most LEA instructions, our implementation will return false. While your patch will always return true if LEA is using virtual registers. Your patch makes our target implementation useless anymore.

There is some internal benchmark performance regression after the patch landed.
The story is as follows:
There is a virtual register: %5 which lives across many basic blocks (some are inside loops).
Before this patch is landed, its weight is: 2.299834e-03 as below:

The broken antideps number looks good but how about the final impact to runtime performance?
Could you please provide SPEC2017 performance data such as rate (like https://reviews.llvm.org/D85368)?

Can you provide any performance data for SPEC? SPEC2017 is better than SPEC2006.

LGTM. But I suggest you waiting for one or two days to see if @qcolombet or others object.

@RKSimon I'm busy with other stuff and my colleague: LiuChen3 will help finish the work.

Can anyone provide me some small reproducers code for the issue tripped over by Chromium / Skia?

Yes, there is a test to ensure that Darwin defaults to the old behaviour in "test/CodeGen/x86_32-m64.c".

I have created a patch for you: https://reviews.llvm.org/D63473
Is it ok?

Please make sure all Chromium for 32-bit Linux libraries are following System V ABI (i.e., m64 is passed on mmx register). I suspect that there are some hand written assembly code in your libraries which is not following the ABI.

Thanks for the comments!
Updated for landing.

Thanks for the suggestions!
I have updated it.

Thanks for the suggestions!
I have updated the patch accordingly.

Consider other Systems (e.g Darwin, PS4 and FreeBSD) don't want to spend any effort dealing with the ramifications of ABI breaks (as discussed in https://reviews.llvm.org/D60748) at present, I only fix the bug for Linux. If other system wants the fix, the only thing needed is to add a flag (like "IsLinuxABI" ) to enable it.

Hi all,

Thanks for the information!
We have reverted the patch and will resubmit it when we have a complete fix.

Ok for merge now?

No, in most normal operation for x86_32, LLVM does the wrong thing. A simple example to show the ABI bug:

Hi all,

Yes, the ABI bug will cause SEGV in Linux where a lot of libraries are built by GCC.
I have restricted the fix to Linux only in the latest revision.

Any other comments?

Ok, I have excluded Darwin and PS4 for the changes.
The fix mainly targets at Linux so that we can compile a project with parts by GCC and parts by LLVM given that they follow the same ABI.

Dear reviewers, any comments?