Page MenuHomePhabricator

[Attributor] Improve the alignment of the loads
Needs ReviewPublic

Authored by omarahmed on Sat, Mar 21, 12:14 PM.

Details

Summary

This patch introduces an improvement in the Alignment of the loads generated in createReplacementValues() by querying AAAlign attribute for the best Alignment for the base.
Note : No alignment implies natural alignment.

Diff Detail

Event Timeline

omarahmed created this revision.Sat, Mar 21, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Mar 21, 12:14 PM
omarahmed retitled this revision from Improve the alignment of the loads to [Attributor] Improve the alignment of the loads.Sat, Mar 21, 12:15 PM
lebedev.ri added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
5557–5558

This should just take MaybeAlign.

5651–5654

Instead, do

createReplacementValues(MaybeAlign(AlignAA.getKnownAlign()), PrivatizableType.getValue(), ACS, Base, NewArgOperands);

You should query the base alignment AA in the initialize once to make sure it exists and is part of the fixpoint iteration. For now that is trivially the case but I can see that this might not be this way in the future.

llvm/lib/Transforms/IPO/Attributor.cpp
5652

I think you can take the assumed alignment.

You should query the base alignment AA in the initialize once to make sure it exists and is part of the fixpoint iteration. For now that is trivially the case but I can see that this might not be this way in the future.

Sry but you mean we should query the base in createInitialization right or you mean another initialization ?

jdoerfert added a comment.EditedSat, Mar 21, 5:08 PM

in AAPrivatize...::initialize or in updateImpl

omarahmed updated this revision to Diff 251887.Sun, Mar 22, 7:38 AM

Address comments

Basictest.ll always make a strange problem with me sometimes the align of the load be 0 and from the condition I added it transform to 1, and in other times it gives align to the load with 4 value so why this could happen ?

another question isn't the default alignment is 1 which means there is no alignment why AAAlign tends sometimes to return 0, shouldn't it return 1 when no alignment available ?
My guess of the default alignment is because I saw MayBeAlign say that 1 means no alignment so is the case different ?

lebedev.ri added inline comments.Sun, Mar 22, 8:06 AM
llvm/lib/Transforms/IPO/Attributor.cpp
5651–5654

What happens if you just do MaybeAlign(AlignAA.getAssumedAlign())?

omarahmed added inline comments.Sun, Mar 22, 9:00 AM
llvm/lib/Transforms/IPO/Attributor.cpp
5651–5654

some cases like argument promotion/attrs.ll returns 0 when query AAAlign attribute so that make it doesn't put any alignment to the load instr

lebedev.ri added inline comments.Sun, Mar 22, 9:21 AM
llvm/lib/Transforms/IPO/Attributor.cpp
5651–5654

Oh, i see what you mean, no alignment implies natural alignment, not alignment of 1..
Please explain all those implications in the source comment.

Minor cleanups

omarahmed edited the summary of this revision. (Show Details)Sun, Mar 22, 11:26 AM
lebedev.ri added a subscriber: courbet.

@courbet To be honest

explicit MaybeAlign(uint64_t Value) {
  assert((Value == 0 || llvm::isPowerOf2_64(Value)) &&
         "Alignment is neither 0 nor a power of 2");
  if (Value)
    emplace(Value);
}

that if-check looks like a major rake.

llvm/lib/Transforms/IPO/Attributor.cpp
5651–5655

I'd suggest rephrasing this to explain what is going on, something like

// Alignment of 0 is treated by MaybeAlign as no alignment,
// but when no alignment is specified for the load instruction,
// natural alignment is assumed. So we have to manually bump it to 1.
createReplacementValues(MaybeAlign(std::max(1, AlignAA.getAssumedAlign()),
omarahmed marked an inline comment as done.Sun, Mar 22, 12:23 PM
llvm/lib/Transforms/IPO/Attributor.cpp
5651–5655

yes you are right we have to be more specific I will modify it sry :) also I forgot about the max function it is much cleaner :)

omarahmed updated this revision to Diff 251914.Sun, Mar 22, 1:39 PM

Minor cleanups

lebedev.ri added inline comments.Sun, Mar 22, 2:06 PM
llvm/lib/Transforms/IPO/Attributor.cpp
5665

s/so when/but when/

omarahmed updated this revision to Diff 251920.Sun, Mar 22, 2:56 PM
omarahmed marked an inline comment as done.

Add ArgumentPromotion/Basictest.ll to XFAIL,
Minor cleanup.

jdoerfert requested changes to this revision.Sun, Mar 22, 11:38 PM
jdoerfert added a subscriber: efriedma.
jdoerfert added inline comments.
llvm/test/Transforms/Attributor/ArgumentPromotion/basictest.ll
5

Please update the test accordingly (or wait for D76588 to pass so you can probably just run the test update script).

This revision now requires changes to proceed.Sun, Mar 22, 11:38 PM

If you always deal with defined alignment there is no need to use MaybeAlign.
Use Align in createReplacementValues (it will get rid of the extra if check).

Now, LoadInst::setAlignment takes a MaybeAlign but Align implicitly casts to MaybeAlign so you can safely pass in an Align (I'll optimize this case by providing setAlignment overloads later).

Then if you get raw alignment values that can be 0 but you assume that 0 means 1 use assumeAligned.
Unfortunately the naming in this context is awkward (but that's how the API is supposed to be used) assumeAligned(AlignAA.getAssumedAlign()).

It will become better over time when AlignAA deals with Align/MaybeAlign directly.

@courbet To be honest

explicit MaybeAlign(uint64_t Value) {
  assert((Value == 0 || llvm::isPowerOf2_64(Value)) &&
         "Alignment is neither 0 nor a power of 2");
  if (Value)
    emplace(Value);
}

that if-check looks like a major rake.

I'm not sure what you mean here exactly. What do you think would be an appropriate check ?

omarahmed marked an inline comment as done.Mon, Mar 23, 9:15 AM

If you always deal with defined alignment there is no need to use MaybeAlign.
Use Align in createReplacementValues (it will get rid of the extra if check).

Now, LoadInst::setAlignment takes a MaybeAlign but Align implicitly casts to MaybeAlign so you can safely pass in an Align (I'll optimize this case by providing setAlignment overloads later).

Then if you get raw alignment values that can be 0 but you assume that 0 means 1 use assumeAligned.
Unfortunately the naming in this context is awkward (but that's how the API is supposed to be used) assumeAligned(AlignAA.getAssumedAlign()).

It will become better over time when AlignAA deals with Align/MaybeAlign directly.

I will try that thank you :)

llvm/test/Transforms/Attributor/ArgumentPromotion/basictest.ll
5

Okay sry :) the problem I have in this test that it is not consistent to a certain value in alignment it sometimes gives alignment of 4 to the loads and sometimes it gives no alignment which I change to normal alignment (value of 1 ) , I will try to look further in this behaviour :) , but what could possibly result in that

omarahmed updated this revision to Diff 254294.Wed, Apr 1, 1:29 PM

Usage of AssumeAligned to force natural alignment

omarahmed added a comment.EditedWed, Apr 1, 1:46 PM

sry for late update, Hard times
Anyway I wanted to ask about a thing i noticed in AAPrivatizable attribute :

In this test :

define internal i32 @test(i32* %X, i32* %Y) {
  %A = load i32, i32* %X
  %B = load i32, i32* %Y
  %C = add i32 %A, %B
  ret i32 %C
}
define internal i32 @caller(i32* %B) {
  %A = alloca i32
  store i32 1, i32* %A
  %C = call i32 @test(i32* %A, i32* %B)
  ret i32 %C
}
define i32 @callercaller() {
  %B = alloca i32
  store i32 2, i32* %B
  %X = call i32 @caller(i32* %B)
  ret i32 %X
}

and when i print the base used in loads in createReplacementValues function it prints :

%A = alloca i32
i32* B
%B = alloca i32

and sometimes i run again and it prints :

%B = alloca i32
%A = alloca i32
%B.priv = alloca i32

and i think the base (i32* B) is wrong and it is a base that's because sometimes ACSRepairCB is invoked before FnRepairCB so the alloca instructions are not yet added so it gets the value of the base from the argument itself and not the alloca so isn't we should make sure that FnRepairCB is invoked before ACSRepairCB to make sure that not happens ?