This is an archive of the discontinued LLVM Phabricator instance.

Enable `align <n>` to be used in intrinsic definitions.
ClosedPublic

Authored by hliao on May 21 2020, 9:14 PM.

Diff Detail

Event Timeline

hliao created this revision.May 21 2020, 9:14 PM
hliao added a comment.May 21 2020, 9:20 PM

To prepare the refactoring on D80364, intrinsics interested should be specified with the alignment on the return pointer. With this patch, amdgcn.dispatch.ptr is defined as follows

def int_amdgcn_dispatch_ptr :
  Intrinsic<[LLVMQualPointerType<llvm_i8_ty, 4>], [],
  [Align<-1, 4>, IntrNoMem, IntrSpeculatable]>;

Should also add this to at least one of the amdgpu intrinsics, and add some tests for it. In particular I would like to see a test that shows an explicit alignment on a call site will override the intrinsic's alignment

To prepare the refactoring on D80364, intrinsics interested should be specified with the alignment on the return pointer. With this patch, amdgcn.dispatch.ptr is defined as follows

def int_amdgcn_dispatch_ptr :
  Intrinsic<[LLVMQualPointerType<llvm_i8_ty, 4>], [],
  [Align<-1, 4>, IntrNoMem, IntrSpeculatable]>;

Referring to the return index by a negative parameter index is a bit weird. How does this work for multiple return values? Is it [-2, -1]? This needs documenting somewhere

hliao added a comment.EditedMay 22 2020, 7:00 AM

To prepare the refactoring on D80364, intrinsics interested should be specified with the alignment on the return pointer. With this patch, amdgcn.dispatch.ptr is defined as follows

def int_amdgcn_dispatch_ptr :
  Intrinsic<[LLVMQualPointerType<llvm_i8_ty, 4>], [],
  [Align<-1, 4>, IntrNoMem, IntrSpeculatable]>;

Referring to the return index by a negative parameter index is a bit weird. How does this work for multiple return values? Is it [-2, -1]? This needs documenting somewhere

There's no such thing. Multiple return values are always aggregated into a single struct and there's only one return value. Different from parameter indices, the return index for the attribute list is just 0 only.

351 class AttributeList {                                                                                                                                                                                                                                     
352 public:                                                                                                                                                                                                                                                   
353   enum AttrIndex : unsigned {                                                                                                                                                                                                                             
354     ReturnIndex = 0U,                                                                                                                                                                                                                                     
355     FunctionIndex = ~0U,                                                                                                                                                                                                                                  
356     FirstArgIndex = 1,                                                                                                                                                                                                                                    
357   };
hliao updated this revision to Diff 265736.May 22 2020, 7:18 AM

Add tests & example changes in relevant AMDGPU intrinsics.

hliao updated this revision to Diff 265737.May 22 2020, 7:28 AM

Revise the comment in the intrinsic emitter.

hliao updated this revision to Diff 265760.May 22 2020, 9:39 AM

Follow part of clang-tidy the suggestion on coding style.

Usually the attributes of a call are the union of (call site attributes, declaration attribute), but in this case it's a bit different since the call site will override it since it happens to be checked first. This is probably fine, but I'm also wondering if defining this as a minimum alignment might be better

Usually the attributes of a call are the union of (call site attributes, declaration attribute), but in this case it's a bit different since the call site will override it since it happens to be checked first. This is probably fine, but I'm also wondering if defining this as a minimum alignment might be better

Not sure I understand. If you query attributes you'll get the first that is found (I think), so if the call site provides one you get that, otherwise the function one, if present. I haven't seen anything here that would be "weird". Wrt. minimum alignment, I guess that is by definition the case. align(X) anywhere means minimum alignment and the Attributor would for example bump call site alignment to the function alignment if that is known to be higher.

I added some minor comments below. From my perspective this is fine, assuming I did understand @arsenm properly. If so, feel free to accept :)

llvm/include/llvm/IR/Intrinsics.td
78

Nit: maybe uint64_t for the align to match other places, though it will work fine as long as int is 32 bit (or more).

llvm/include/llvm/IR/IntrinsicsAMDGPU.td
173

I think -1 is in principle the right thing here *but* we should try to make it a variable with a proper name, e.g. ReturnIndex.

llvm/lib/IR/Attributes.cpp
1186

Nit: Message for the assert please.

llvm/utils/TableGen/CodeGenIntrinsics.h
157

Every time I see a use of tuple I ask myself the same question: Wouldn't a POD-like struct with some properly named members be better? I feel std::get<1>(T) is somewhat less helpful than T->Index. I don't need you to change it here but it's something to think about. In particular it might help to remove magic numbers below, e.g., 0 for the value.

xbolva00 added inline comments.
llvm/utils/TableGen/CodeGenIntrinsics.h
157

+1

hliao updated this revision to Diff 266556.May 27 2020, 8:38 AM
  • Introduce RetIndex and ArgIndex<n> to specify attribute indices.
  • Minor refinement following other review comments.
hliao marked 5 inline comments as done.May 27 2020, 8:41 AM
hliao added inline comments.
llvm/include/llvm/IR/Intrinsics.td
78

int in td is represented as int64_t internally.

llvm/include/llvm/IR/IntrinsicsAMDGPU.td
173

introduce RetIndex and ArgIndex<n> to specifiy that attribute indices.

hliao updated this revision to Diff 266559.May 27 2020, 8:44 AM

remove <tuple> header.

arsenm accepted this revision.May 27 2020, 10:56 AM

LGTM. I would split the ArgIndex refactor into a precommit but up to you

This revision is now accepted and ready to land.May 27 2020, 10:56 AM
This revision was automatically updated to reflect the committed changes.