Page MenuHomePhabricator

[Codegen] If reasonable, materialize clang's `AssumeAlignedAttr` as llvm's Alignment Attribute on call-site function return value
ClosedPublic

Authored by lebedev.ri on Jan 19 2020, 9:48 AM.

Details

Summary

This should be mostly NFC - we still lower the same alignment
knowledge to the IR. The main reasoning here is that
this somewhat improves readability of IR like this,
and will improve test coverage in upcoming patch.

Even though the alignment is guaranteed to always be an I-C-E,
we don't always materialize it as llvm's Alignment Attribute because:

  1. There may be a non-zero offset
  2. We may be sanitizing for alignment

Note that if there already was an IR alignment attribute
on return value, we union them, and thus the alignment
only ever rises.

Also, there is a second relevant clang attribute AllocAlignAttr,
so that is why AbstractAssumeAlignedAttrEmitter is templated.

Diff Detail

Event Timeline

jdoerfert added inline comments.Jan 21 2020, 2:54 PM
clang/test/CodeGen/assume-aligned-and-alloc-align-attributes.c
12–13

Why would we still emit the llvm.assume here?

lebedev.ri marked 2 inline comments as done.Jan 21 2020, 3:04 PM
lebedev.ri added inline comments.
clang/test/CodeGen/assume-aligned-and-alloc-align-attributes.c
12–13

Because it is for the second/other attribute - __attribute__((alloc_align(2))).
This is potentially fixed by the next patch - D73006.

erichkeane added inline comments.Jan 23 2020, 9:23 AM
clang/lib/CodeGen/CGCall.cpp
3835

Does this need an initial value?

3840

Does it make sense to call this on not a functiondecl? Should this just be an assert?

3851

dyn_cast_or_null?

lebedev.ri marked 5 inline comments as done.Jan 23 2020, 9:38 AM

Thank you for taking a look!

clang/lib/CodeGen/CGCall.cpp
3835

I'd say no, we should always set it in constructor.
Although sure, let's add one.

3840

This is modelling what's happening in CodeGenFunction::EmitCall().
There, we clearly check that TargetDecl is not null,
which means i'd say we shouldn't assert that here, but also check.

3851

We shouldn't ever get null pointer there.
If that somehow happens (i'm not sure how), failed assert would be correct IMO.

erichkeane accepted this revision.Jan 23 2020, 9:41 AM
This revision is now accepted and ready to land.Jan 23 2020, 9:41 AM
lebedev.ri marked 4 inline comments as done.

Init Alignment to nullptr by default.
While there, tune maybeRaiseRetAlignmentAttribute() to do nothing
if new alignment is not greater than the existing alignment.

Thank you for the review!

Rebased, NFC.