Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vector load/store instruction large displacement bug #3531

Merged
merged 1 commit into from Feb 7, 2019

Conversation

NigelYiboYu
Copy link
Contributor

@NigelYiboYu NigelYiboYu commented Jan 30, 2019

Fix vector load/store instruction large displacement bug

Fix a problem in VRS, VSI, and VRV vector-storage instruction
formats to handle large displacements correctly.

The fix consists of two parts: an instruction selection phase change and
a binary encoding change. Both of these changes aim to use a scratch register
to hold the base+displacement value if a vector load/store instruction has a
displacement that exceeds the 12-bit signed integer range.

Signed-off-by: Nigel Yu yunigel@ca.ibm.com

@fjeremic
Copy link
Contributor

We really ought to add Tril testing for such cases. This would have been caught long ago. @Leonardo2718 do we have a timeline when we may support processor detection for Tril testing? Such a test for this bug would have required z13 machines.

@NigelYiboYu NigelYiboYu changed the title Fix vector load/store instruction large displacement bug WIP: Fix vector load/store instruction large displacement bug Jan 31, 2019
@NigelYiboYu
Copy link
Contributor Author

I'm currently working on a more complete fix. Marking this WIP.

@Leonardo2718
Copy link
Contributor

@fjeremic

@Leonardo2718 do we have a timeline when we may support processor detection for Tril testing? Such a test for this bug would have required z13 machines.

#3539 🙂

@NigelYiboYu NigelYiboYu force-pushed the fix-vector-large-disp branch 2 times, most recently from 8784f91 to f8042f4 Compare February 5, 2019 15:38
return new (INSN_HEAP) TR::S390VRSaInstruction(cg, op, n, targetReg, sourceReg, mr, mask4);
if (mr != NULL) preced = mr->enforceSSFormatLimits(n, cg, preced);

if (preced)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment on the use of two different constructors here because I can't find a better place to put it. We have to use two constructors here for a lot of instruction formats because the root OMR::Instruction has two constructors and they use different preceding instructions. We can misplace an instruction if the preceding instruction is not used properly.

@NigelYiboYu NigelYiboYu changed the title WIP: Fix vector load/store instruction large displacement bug Fix vector load/store instruction large displacement bug Feb 5, 2019
compiler/z/codegen/S390GenerateInstructions.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fjeremic fjeremic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comments.

Fix a problem in VRS, VSI, and VRV vector-storage instruction
formats to handle large displacements correctly.

The fix consists of two parts: an instruction selection phase change and
a binary encoding change. Both of these changes aim to use a scratch register
to hold the base+displacement value if a vector load/store instruction has a
displacement that exceeds the 12-bit signed integer range.

Signed-off-by: Nigel Yu <yunigel@ca.ibm.com>
@fjeremic
Copy link
Contributor

fjeremic commented Feb 6, 2019

@genie-omr build all

@fjeremic
Copy link
Contributor

fjeremic commented Feb 7, 2019

AppVeoyr failed due to infrastructure reasons.

@fjeremic fjeremic merged commit c785081 into eclipse:master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants