https://reviews.llvm.org/D62370 is created to update the test llvm/test/CodeGen/SystemZ/codegenprepare-splitstore.ll

LGTM. But please hold on for some days if someone else might have comments.

Address jinong's comments.

Gentle ping ... Thank you!

LGTM, except some minor comments. However, please hold on for some days, if others have comments.

FYI. This is the thread of the discussion. http://lists.llvm.org/pipermail/llvm-dev/2016-September/105291.html

That was discussed widely when https://reviews.llvm.org/D26149 is reviewed. This is the commit log saying something about the delay.

This optimization was discussed on llvm-dev some time ago in "Load combine pass" thread. We came to the conclusion that we want to do
this transformation late in the pipeline because in presence of atomic loads load widening is irreversible transformation and it might hinder other optimizations.

Gentle ping ...

ok. Use the hasValue instead of the implicit bool convert operator to make the code clear.

Update local variable name.

Address comments.

Use opt instead of llc to do the check.

In D61169#1481670, @dmgreen wrote:

Hello. I feel like this splitting just shouldn't be done for volatile stores.

@fhahn I have completed all the needed patches, and it would be great if you have the time to continue the review. This is the whole picture.
https://reviews.llvm.org/D61248 is the 1st patch to allow the schedule strategy to forward the schedule state between MBB.
https://reviews.llvm.org/D61249 is the 2nd patch to update the schedule strategy for SystemZ target to adapt with 1st patch.
https://reviews.llvm.org/D59480(this patch) is the 3rd patch to add the scheduled state data structure, so that, it could be kept somewhere.
https://reviews.llvm.org/D61250 is the last patch to forward the scheduled state 3rd patch added for PostGenericScheduler and enable it for PowerPC target.

Update the patch according to the reviewer comments.

I am ok now for the doc part change. Thank you.

Looks good, just one minor comments.
It seems that, we have already had the PowerPC section in the rst. It would be better to put them together.

PowerPC Language Extensions
------------------------------

It would be great if we could add some documentation for these new added builtins, to make more people to know what we have done. i.e. clang/docs/LanguageExtensions.rst

ok. I will prepare another two patches first, so that, we could continue the review.

Thank you for the review comments. I have addressed all the comments.

gentle ping ...

Address comments.

In D59480#1432943, @fhahn wrote:

On a first look, the patch looks useful! Any chance to provide the follow-up patch that uses it as well? that would be helpful to see the bigger picture. Also, are you seeing any benefits by preserving the state?

In D59480#1433808, @jonpa wrote:

I did something similar for the SystemZ PostRA scheduler. Is this patch taking that into consideration? I have not looked at this in detail, but it would be nice if the SystemZ implementation could use this or at least parts of it...

gentle ping ..

In D58378#1402373, @nemanjai wrote:

This can cause relocation overflows:

$ cat b.c 
double b[1LU << 33];
double foo() { return b[(1LU << _SH) - 1] ; }
void setfoo(double d) { b[(1LU << _SH) - 1] = d; }

$ cat main.c 
double foo();
void setfoo(double);
int main(void) {
  setfoo(445.2);
  return foo() == 445.2;
}

$ clang -O2 b.c main.c -D_SH=28
/tmp/b-9d97be.o: In function `foo':
b.c:(.text+0x8): relocation truncated to fit: R_PPC64_TOC16_HA against symbol `b' defined in COMMON section in /tmp/b-9d97be.o+7ffffff8
/tmp/b-9d97be.o: In function `setfoo':
b.c:(.text+0x28): relocation truncated to fit: R_PPC64_TOC16_HA against symbol `b' defined in COMMON section in /tmp/b-9d97be.o+7ffffff8
clang-9: error: linker command failed with exit code 1 (use -v to see invocation)

LGTM. However, please hold on until another reviewer accept this patch,

LGTM.

It seems that, there are more registers in ABI about the additional register than we present in this patch. It would be great if we would list all of them here or add some comments to indicate this at least.

LGTM.

Update the comments.

LGTM.

LGTM now.

In D54824#1308344, @nemanjai wrote:

In D54824#1306039, @steven.zhang wrote:

Personally, I don't like writing the test case using such script. Because, it will check all the instructions and make your test point unclear. And it hard code the register name, which make this test fragile。

This makes maintenance of tests easier since anyone can re-run the script. Of course, the test case is very strictly specified which will make it sensitive to changes in codegen/regalloc/scheduling. However, it is arguably easier for someone to re-run the script and for reviewers to carefully review every aspect of the change than it would be if only specific instructions are checked. There is some value in tests being sensitive to changes not necessarily related to what they are testing - we may discover unintended changes that are actually detrimental.

As to the point about what the test case is actually testing, I think that is a valid concern and a comment should explain that.

Thank you for the detail explanation. So, there are Pro & Cons for both and they are targeting for different purpose. For this case, we shouldn't use this script as we have clear testing point.

Personally, I don't like writing the test case using such script. Because, it will check all the instructions and make your test point unclear. And it hard code the register name, which make this test fragile。

You don't need create a review for the new added NFC test case. LGTM.

LGTM.

The patch will disable some valid transformation post RA(i.e. LXSDX --> LXSD if the def reg is valid for D-form). We can continue to check the dst Reg to see if it is valid, instead of disabling it completely.

This is for pre-RA and my fix is for Post RA. Maybe, we can extend my fix to cover the pre-RA case. I will re-submit a new fix to cover this test case. I think, we could abandon this fix.

@wuzish You could apply the commit access from community. I will commit the patch for you.

I will commit the patch for you.

I will commit the patch for you.

I have done the following tests with this patch:

1. LLVM check-all uniitests
2. LLVM test-suites
3. bootstrap build the clang with the clang including my fix, and then, run the llvm check-all and test-suites, and it passed.

I have updated the patch and we can continue the review now.

Clean up all the failures from llvm test-suite. We miss to check the RegMO operand for the ADDI.

Yes, you are right.
y = ADDI x, imm.
w = LFDX 0, y

It is done.

Remove the data layout and triple in the IR and specify it in the command line.

Add the triple for the new created test case toc-float.ll

Update the change basing on Nemanjai's comment. Thank you.

If I understand correctly, we may also need to handle the DFLOADf32/DFLOADf64 and store pair here, as this is done pre-RA.

There are two changes:

1. add a query to check if it is imm operand as Nemanjai suggested. Didn't find other places that could use this query.
2. add a new test case to address the global case.

Thank you for the comment. I will fix that. BTW, I cannot get the assertion from your case, but this case.(miss some options ?) It seems that, clang put the floating variable into the TOC with some condition, which is different from gcc/xlc. I will deliver some other change to fix this issue.