This is an archive of the discontinued LLVM Phabricator instance.

Update InstCombine to transform aggregate loads into scalar loads.
ClosedPublic

Authored by deadalnix on Mar 13 2015, 7:12 PM.

Details

Summary

One step further getting aggregate loads and store being optimized properly. This will only handle struct with one element at this point.

Depends on D7780

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 21978.Mar 13 2015, 7:12 PM
deadalnix retitled this revision from to Update InstCombine to transform aggregate loads into scalar loads..
deadalnix updated this object.
deadalnix edited the test plan for this revision. (Show Details)
deadalnix added a subscriber: Unknown Object (MLST).
deadalnix updated this revision to Diff 22040.Mar 16 2015, 12:42 PM

Update as per style
rebase

deadalnix updated this revision to Diff 22538.Mar 23 2015, 9:26 PM

Better naming for unpacked loads.

deadalnix updated this revision to Diff 22920.Mar 30 2015, 5:14 PM

Rebase and ping

It's been 3 weeks folk. Can we make something happen here, before we all die from old age ?

majnemer added inline comments.Apr 4 2015, 9:47 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
524

We know the type from the right hand side, please change this to auto *ST.

526

Is there any reason why you only go one level deep? What if we peel the struct type away just to find another single element struct type? Perhaps the InstCombine worklist will naturally get this... In any case, can you please have a test for this case?

test/Transforms/InstCombine/unpack-fca.ll
33

Please use CHECK-LABEL with the IR function's name for each test.

mehdi_amini edited edge metadata.Apr 4 2015, 9:58 PM

Can you provide a more complete description of what this is achieving in the commit message?

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
317

Is there an invariant between V and NewTy here?
Should we add:

assert((V->getType() == NewTy->getPointerTo(LI.getPointerAddressSpace())) && "V type mismatches NewTy");
512

Missing documentation.

522

Is this check useful? It seems redundant with the next one.
What about doing an early exit instead of adding nesting level:

StructType *ST = dyn_cast<StructType>(T);
// Only unpack structure with one element.
if(!ST || ST->getNumElements() != 1) return nullptr;

Ptr = IC.Builder->CreateStructGEP(Ptr, 0);
LoadInst *NewLoad = combineLoadToNewValue(IC, LI, Ptr, T, ".unpack");

Instruction *V = InsertValueInst::Create(UndefValue::get(T), NewLoad, 0, LI.getName());

LI.replaceAllUsesWith(V);
return V;
test/Transforms/InstCombine/unpack-fca.ll
15

When there are multiple tests in one file it is a good practice to use CHECK-LABEL for each function.

61

Use CHECK-NEXT to be sure that no load remains between the store and the ret (and makes explicit that you don't expect any other instruction here).
Alternatively you can also use

CHECK-NOT: load

after the check for the store.

deadalnix added inline comments.Apr 5 2015, 4:14 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
522

The code has it current structure because I intend to add support for arrays and multiple element structs. In this case, the early return do not make sense. The isAggregateType however, is indeed an early return in that case. I can update this if you feel strongly about it, but that would have to be undone is a subsequent diff.

526

Yes, InstCombine use a specific inserter in its builder. Also, this is what the structOfA test test for :)

test/Transforms/InstCombine/unpack-fca.ll
61

I never really intended to check this as this isn't really the goal of the diff, but I can add that.

deadalnix added inline comments.Apr 5 2015, 4:21 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
317

No, but they should be the same size. Adding this invariant.

deadalnix updated this revision to Diff 23261.Apr 5 2015, 4:38 PM
deadalnix edited edge metadata.

Rebase and address various comments.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
522

OK, makes sense.

reames edited edge metadata.Apr 9 2015, 9:57 AM

Looks functionally correct to me. I do have some style comments that need addressed though.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
316

The naming here is confusing to the point where I'm having a hard time telling what you intend. If I'm reading this right, by Value you mean address loaded from? You're changing both the type loaded and the pointer operand in one go?

If so, I might suggest the following:

  • pass both the pointer operand and new type explicitly. This will be necessary for the new unified pointer type work and helps readability now.
  • Don't use the name V. Call it NewPointerOperand or something similiar. NewAddr would also be fine.
  • Clarify the comment about what the function does.

You might also consider changing the type loaded and the pointer operand in two distinct steps.
NewLI = combineLoadToType(*this, OldLI, ElementType);
NewLI->setPointerOperand(ElementPtr);

The intermediate state (load type X from a pointer of type Y using a bitcast) is entirely legal.

745

I suspect your new code should be below these checks.

deadalnix added inline comments.Apr 10 2015, 5:45 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
316

setPointerOperand does not exists, but that is no big deal, a bitcast is just as valid here, so let's do that, it can make the diff much simpler.

deadalnix updated this revision to Diff 23637.EditedApr 10 2015, 7:05 PM
deadalnix edited edge metadata.

Simplify the whole thing as suggested by @reames

deadalnix updated this revision to Diff 23956.Apr 17 2015, 12:07 PM

Friday ping !

pete added a subscriber: pete.Apr 17 2015, 12:28 PM

You probably need to wait for some of the other commenters to give the final ok on this, but FWIW it LGTM with the changes I pointed out.

Cheers,
Pete

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
515

I think this should now be an assert as you've moved the call to unpackLoadToAggregate below an identical check.

519

This can just be LI.getType() instead of going through the pointer type.

521

If you're planning on handling arrays then please ignore me, but otherwise you don't need this check as you already explicitly check for struct types below.

With Pete's comments addressed and the missing setPointerOperand(gep struct, 0) added, LGTM.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
530

You're missing the setPointerOperand step. Technically, the first element doesn't have to start at the beginning of the object. I think this is fine in practice, but there's a theoretical issue here.

deadalnix added inline comments.Apr 20 2015, 6:11 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
515

Other check also have this as a check rather than an invariant. I think this is ok as the applicability of these optimization will vary, and at the end we want the check there (considering optimizing non simple load/stores is making progress).

521

Yes, that is the plan. I had a complete solution, but I am breaking it up for easier review.

530

I'm not sure how this is possible at all.

deadalnix updated this revision to Diff 24088.Apr 20 2015, 6:12 PM

Use LI.getType()

reames added inline comments.Apr 20 2015, 6:50 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
530

Unless the struct is packed, the data layout alignment could require that the internal field be aligned to a higher alignment than the struct. This would be a weird arrangement, but I believe it's possible.

deadalnix added inline comments.Apr 20 2015, 7:45 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
530

That would require the field to be at a variable position in the struct, which I don't think is supported.

Note that this piece of code is plugged AFTER the alignment is computed from the type for that very reason. Maybe I should add an assert to ensure alignment is set so it is won't break later.

deadalnix updated this revision to Diff 24870.May 3 2015, 8:20 PM

rebase, add an assertion to ensure alignement is se is sett.

And let's get that in !

deadalnix updated this revision to Diff 24937.May 5 2015, 12:39 AM

Rebase and ping.

majnemer added inline comments.May 5 2015, 1:20 AM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
316

Ampersand goes on the right hand side.

527–528

Did you intend to use i for any particular purpose? If not, please fold it into your call to getTypeAtIndex.

530–534

Can this be:

return ReplaceInstUsesWith(LI, Builder->CreateInsertValue(UndefValue::get(T), NewLoad, 0, LI.getName());
deadalnix added inline comments.May 5 2015, 2:51 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
527–528

Does not work. getTypeAtIndex has 2 overload, and passing 0 directly is ambiguous (in C++ 0 can also be a null pointer).

majnemer added inline comments.May 5 2015, 3:04 PM
lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
527–528

getTypeAtIndex(0U) should work.

reames removed a reviewer: reames.May 5 2015, 3:39 PM
majnemer accepted this revision.May 5 2015, 8:36 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 5 2015, 8:36 PM
mehdi_amini closed this revision.May 6 2015, 10:56 PM

Committed: r236695