This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Support load/store of dso_local PIC global values
ClosedPublic

Authored by MaskRay on Jul 17 2022, 5:17 PM.

Details

Summary

lowerGlobalAddress added by D128427 can be used for PIC. The actual condition is
that the global value needs to be dso_local (a dso_preemptable one needs GOT
indirection).

load-store.ll has UB due to out-of-bounds load/store. Fix the UB in the variable
test and add an array test. Note: NOPIC array index is currently wrong.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 17 2022, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2022, 5:17 PM
MaskRay requested review of this revision.Jul 17 2022, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2022, 5:17 PM

Thanks!

load-store.ll has UB due to out-of-bounds load/store. Fix the UB in the variable
test and add an array test. Note: PIC array index is currently wrong.

Spelling mistakes? Is NONPIC?

The wrong array index is due to constant folding with GlobalAddress when isDSOLocal().
The interface function of the common layer is TargetLowering::isOffsetFoldingLegal.

Thanks!

load-store.ll has UB due to out-of-bounds load/store. Fix the UB in the variable
test and add an array test. Note: PIC array index is currently wrong.

Spelling mistakes? Is NONPIC?

I pick NOPIC because -fno-pic is usually used to refer the position-dependent codegen. NOPIC should be fine.

The wrong array index is due to constant folding with GlobalAddress when isDSOLocal().
The interface function of the common layer is TargetLowering::isOffsetFoldingLegal.

Thanks!

Thanks!

load-store.ll has UB due to out-of-bounds load/store. Fix the UB in the variable
test and add an array test. Note: PIC array index is currently wrong.

Spelling mistakes? Is NONPIC?

I pick NOPIC because -fno-pic is usually used to refer the position-dependent codegen. NOPIC should be fine.

Sorry, my mistake(Comments without context).

I mean, in Summary, the description of this sentence:

Note: PIC array index is currently wrong.

Should be replaced by:

Note: NOPIC array index is currently wrong.
MaskRay edited the summary of this revision. (Show Details)Jul 17 2022, 11:20 PM
wangleiat accepted this revision.Jul 21 2022, 7:30 PM

Thanks!

load-store.ll has UB due to out-of-bounds load/store. Fix the UB in the variable
test and add an array test. Note: PIC array index is currently wrong.

Spelling mistakes? Is NONPIC?

I pick NOPIC because -fno-pic is usually used to refer the position-dependent codegen. NOPIC should be fine.

The wrong array index is due to constant folding with GlobalAddress when isDSOLocal().
The interface function of the common layer is TargetLowering::isOffsetFoldingLegal.

Thanks!

LGTM

Will you fix wrong array index in this patch or in separate one?

This revision is now accepted and ready to land.Jul 21 2022, 7:30 PM

Thanks!

load-store.ll has UB due to out-of-bounds load/store. Fix the UB in the variable
test and add an array test. Note: PIC array index is currently wrong.

Spelling mistakes? Is NONPIC?

I pick NOPIC because -fno-pic is usually used to refer the position-dependent codegen. NOPIC should be fine.

The wrong array index is due to constant folding with GlobalAddress when isDSOLocal().
The interface function of the common layer is TargetLowering::isOffsetFoldingLegal.

Thanks!

LGTM

Will you fix wrong array index in this patch or in separate one?

If you have a patch, please just fix it :) otherwise I may be interested in fixing it in my spare time... Thanks!

This revision was landed with ongoing or failed builds.Jul 21 2022, 7:38 PM
This revision was automatically updated to reflect the committed changes.