This is an archive of the discontinued LLVM Phabricator instance.

Verify parameter alignment attribute
ClosedPublic

Authored by LuoYuanke on Mar 21 2022, 5:29 AM.

Details

Summary

In DAGISel, the parameter alignment only have 4 bits to hold the value.
The encode(alignment) would plus the value by 1, so the max aligment that
ISel can support is 2^14. This patch verify align attribute for parameter.

Diff Detail

Event Timeline

LuoYuanke created this revision.Mar 21 2022, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 5:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
LuoYuanke requested review of this revision.Mar 21 2022, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 5:29 AM
LuoYuanke updated this revision to Diff 416909.Mar 21 2022, 5:55 AM

Fix test case failure.

pengfei added inline comments.Mar 21 2022, 6:08 AM
llvm/lib/IR/Verifier.cpp
1828

I think we can put the check inside here:

if (Attrs.hasAttribute(Attribute::ByVal)) {
  if (Attrs.hasAttribute(Attribute::Alignment)) {
    ...
  }
  SmallPtrSet<Type *, 4> Visited;
1832

Do we need check for ByRef?

LuoYuanke added inline comments.Mar 21 2022, 6:33 AM
llvm/lib/IR/Verifier.cpp
1832

It seems align attribute doesn't affect ByRef. Run below code with "llc t.ll" doesn't crash.

define dso_local void @foo(i8* %p) {
entry:
  %p1 = bitcast i8* %p to <8 x float>*
  call void @bar(<8 x float>* noundef byref(<8 x float>) align 32768 %p1)
  ret void
}

declare dso_local void @bar(<8 x float>* %p)

In https://llvm.org/docs/LangRef.html, it says Note that this attribute has additional semantics when combined with the byval or preallocated attribute, which are documented there.

LuoYuanke updated this revision to Diff 416916.Mar 21 2022, 6:44 AM

Address Phoebe's comments.

craig.topper added inline comments.Mar 21 2022, 10:21 AM
llvm/lib/IR/Verifier.cpp
1824

ArgFlagsTy -> ISD::ArgFlagsTy

Address Craig's comments.

pengfei accepted this revision.Mar 21 2022, 10:46 PM

LGTM.

llvm/lib/IR/Verifier.cpp
1827

Missing the period .

This revision is now accepted and ready to land.Mar 21 2022, 10:46 PM

Address Phoebe's comments.

skan added inline comments.Mar 21 2022, 10:58 PM
llvm/lib/IR/Verifier.cpp
1825

The Align(1<<15) and comments here are duplicated in D121898, I suggest that you add Align(1<<15) as a const static member of Verifier, and move the comments to the position of the definition. The we can avoid duplicated code.

LuoYuanke added inline comments.Mar 21 2022, 11:20 PM
llvm/lib/IR/Verifier.cpp
1825

Good suggestion. I apply your suggestion in https://reviews.llvm.org/D121898. Will update this patch after D121898 land.

This revision was landed with ongoing or failed builds.Mar 26 2022, 6:13 PM
This revision was automatically updated to reflect the committed changes.