Page MenuHomePhabricator

[PowerPC] Update alignment for ReuseLoadInfo in LowerFP_TO_INTForReuse
ClosedPublic

Authored by lkail on Apr 6 2020, 7:36 PM.

Details

Summary

In LowerFP_TO_INTForReuse, when emitting stfiwx, alignment of 4 is
set for the MachineMemOperand, but RLI(ReuseLoadInfo)'s alignment is not updated
for following loads.

It's related to failed alignment check reported in https://bugs.llvm.org/show_bug.cgi?id=45297

Diff Detail

Event Timeline

lkail created this revision.Apr 6 2020, 7:36 PM
lkail retitled this revision from [PowerPC] Update alignment for RLI in LowerFP_TO_INTForReuse to [PowerPC] Update alignment for ReuseLoadInfo in LowerFP_TO_INTForReuse.Apr 6 2020, 7:39 PM
lkail edited the summary of this revision. (Show Details)
lkail edited the summary of this revision. (Show Details)Apr 6 2020, 7:46 PM
steven.zhang added inline comments.Apr 6 2020, 8:05 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7945

You are setting the RLI.Alignment to uninitialized value if it is not i32Stack.

lkail marked an inline comment as done.Apr 6 2020, 8:17 PM
lkail added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7945

The default constructor of Align will set it to 1. This is consistent with Chain = DAG.getStore(DAG.getEntryNode(), dl, Tmp, FIPtr, MPI); which also will have alignment of 1 for the store. I don't have deep insight of if it's correct for the Chain to have alignment of 1 yet.

steven.zhang added inline comments.Apr 6 2020, 10:12 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7945

You shouldn't have the assumption of the value of the default arguments of the Alignment and the getStore(), as they can be changed. You should specify the explicit initial value of the Alignment. (i.e. Align Alignment(1)).

steven.zhang added inline comments.Apr 6 2020, 10:28 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7926–7927

Replace the Align(4) with Alignment.

7932

Pass the Alignment to the getStore() as what we have done for getMachineMemOperand().

lkail updated this revision to Diff 255591.Apr 6 2020, 10:48 PM

Use MaybeAlign to initialize default alignment explicitly.

lkail updated this revision to Diff 255594.Apr 6 2020, 11:01 PM

Address comment.

lkail marked 2 inline comments as done.Apr 6 2020, 11:04 PM
This comment was removed by lkail.
lkail updated this revision to Diff 255596.Apr 6 2020, 11:09 PM
lkail updated this revision to Diff 255608.Apr 7 2020, 1:10 AM

Discussed with @steven.zhang, it seems more appropriate to use Align directly and initialize it to the default alignment of the stored value.

daltenty added inline comments.Apr 7 2020, 8:39 AM
llvm/test/CodeGen/PowerPC/kernel-fp-round.ll
2

We run into this problem in 32/64-bit big endian as well when targeting pwr6+ (i.e. -mtriple=powerpc-unknown-unknown -mattr=-vsx -mcpu=pwr6), please add a run line for it.

lkail updated this revision to Diff 255879.Apr 7 2020, 7:04 PM

Add run lines for 32/64bit big endian on pwr6.

lkail marked an inline comment as done.Apr 7 2020, 7:05 PM
lkail updated this revision to Diff 255881.Apr 7 2020, 7:12 PM
steven.zhang accepted this revision.Apr 7 2020, 7:23 PM

LGTM now, but pls hold on for some days in case others have more comments.

This revision is now accepted and ready to land.Apr 7 2020, 7:23 PM
daltenty accepted this revision.Apr 8 2020, 8:37 AM

LGTM

sfertile accepted this revision.Apr 8 2020, 11:11 AM

LGTM now, but pls hold on for some days in case others have more comments.

LGTM too. Not a bad idea, but maybe give a days grace instead of a few days.

This revision was automatically updated to reflect the committed changes.