This is an archive of the discontinued LLVM Phabricator instance.

Add/Implement AddresSpace to PseudoSourceValue.
ClosedPublic

Authored by jsjodin on Jul 6 2017, 2:30 PM.

Details

Summary

This is built on top of https://reviews.llvm.org/D21688 (not yet committed). Instead of taking the address space as an argument in the constructor we use a the target and callback inside the constructor since the source kind should have enough information for the target to decide which address space to use. This also contains the changes in the rest of the compiler to use the new interface. The TII argument does not have a default value (nullptr) because it should be explicit that the target is not needed and the generic address space will be used in all cases, otherwise it is easy to forget and could cause bugs that would be hard to find.

Diff Detail

Event Timeline

jsjodin created this revision.Jul 6 2017, 2:30 PM
arsenm added inline comments.Jul 6 2017, 2:55 PM
lib/CodeGen/PseudoSourceValue.cpp
30

If it's required, TII should be a reference or at least this should assert

lib/Target/AMDGPU/SIInstrInfo.cpp
1771–1775

These should all be CONSTANT_ADDRESS

1777

Prefereable to use the enum value rather than 0

lib/Target/Mips/MipsMachineFunction.cpp
96

I thought the PSVManager held a TII reference?

jsjodin added inline comments.Jul 7 2017, 3:53 AM
lib/CodeGen/PseudoSourceValue.cpp
30

Yes, I think it may be better to have an assert. That way we don't have to dereference any pointers at the call sites. My initial idea was that if the target only has a generic address space there would be no need for TII, so a nullptr would be okay, but it would at least be explicit in the call, but that may be more confusing. I will fix this.

lib/Target/Mips/MipsMachineFunction.cpp
96

I wasn't sure, but in that case we can use TII, and put an assert in the PSV constructor.

jsjodin updated this revision to Diff 105621.Jul 7 2017, 5:05 AM

Make TII mandatory (assert if given nullptr) in PSV constructor. Add TII as a member to the PSV manager so that it is not needed in the method calls.

jsjodin marked 2 inline comments as done.Jul 7 2017, 5:08 AM
jsjodin marked 2 inline comments as done.
yaxunl added a subscriber: yaxunl.Jul 17 2017, 11:11 AM
t-tye added a subscriber: t-tye.Aug 3 2017, 11:48 AM

Ping.

Any progress? D35361 relies on this patch and we need it to fix vload_private/vstore_private failure with new address space mapping.

hfinkel added a subscriber: hfinkel.Sep 2 2017, 1:09 PM

Can you please upload a full-context patch?

jsjodin updated this revision to Diff 114651.Sep 11 2017, 12:27 PM

Merged code with TOT and made full context patch.

hfinkel added inline comments.Sep 11 2017, 7:49 PM
lib/CodeGen/PseudoSourceValue.cpp
30

I don't understand this response. Can you make this a reference? It seems required here in the constructor.

lib/Target/AMDGPU/SIInstrInfo.h
261

Line is too long.

jsjodin added inline comments.Sep 12 2017, 10:16 AM
lib/CodeGen/PseudoSourceValue.cpp
30

Yes. it can be made into a reference. I will post a new patch with this.

jsjodin updated this revision to Diff 114868.Sep 12 2017, 11:17 AM

Made changes to address comments by Hal Finkel.

jsjodin marked an inline comment as done.Sep 12 2017, 11:18 AM
hfinkel accepted this revision.Sep 13 2017, 2:34 AM

LGTM

lib/Target/AMDGPU/SIInstrInfo.cpp
1848

Indentation off here.

This revision is now accepted and ready to land.Sep 13 2017, 2:34 AM
This revision was automatically updated to reflect the committed changes.