Page MenuHomePhabricator

AMDGPU: Implement computeKnownAlignForTargetInstr
ClosedPublic

Authored by arsenm on Jun 5 2020, 12:41 PM.

Details

Summary

We probably need to move where intrinsics are lowered to copies to
make this useful.

Diff Detail

Event Timeline

arsenm created this revision.Jun 5 2020, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 12:41 PM
hliao added inline comments.Jun 8 2020, 7:40 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11233

Try Intrinsic::getAttributes

arsenm updated this revision to Diff 269844.Jun 10 2020, 7:33 AM

Get alignmnent from intrinsic declaration. This can probably go in generic code, but I'm unclear what the semantics of a call site with a lower alignment is

foad added a comment.Jun 11 2020, 5:40 AM

This can probably go in generic code, but I'm unclear what the semantics of a call site with a lower alignment is

IR has the same issue and the solution adopted in D65281 is that any attribute on the call site takes priority.

foad added a comment.Jun 17 2020, 1:34 AM

Why not put it in generic code to start with? I don't understand why the concern about conflicting alignment would prevent that. I mean, are you more confident about how to handle conflicting alignments for AMDGPU than you are for all other targets?

Why not put it in generic code to start with? I don't understand why the concern about conflicting alignment would prevent that. I mean, are you more confident about how to handle conflicting alignments for AMDGPU than you are for all other targets?

I know in this case, even if the call site specified a lower alignment, you're always getting at least 4. For other targets, I don't know if a lower call site alignment would need to be respected. Ultimately we need an assert_align pseudo to track the call site attribute

foad accepted this revision.Jun 17 2020, 6:13 AM

I'd still be inclined to either follow the precedent of IR, or take the max of the two alignments. But I guess this is OK if you're reluctant to do that.

This revision is now accepted and ready to land.Jun 17 2020, 6:13 AM

I'd still be inclined to either follow the precedent of IR, or take the max of the two alignments. But I guess this is OK if you're reluctant to do that.

From the context of just the intrinsic itself, we don't have the option