[PATCH] D125485: [ArgPromotion] Unify byval promotion with non-byval

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:635
+              HandleEndUser(SI, SI->getValueOperand()->getType(),
+                            /* GuaranteedToExecute */ true)) {
+        if (!*Res)
----------------
nikic wrote:
> psamolysov wrote:
> > nikic wrote:
> > > psamolysov wrote:
> > > > nikic wrote:
> > > > > psamolysov wrote:
> > > > > > I'm not aware about speculative stores. Should we handle the stores in the exactly same way as loads here, so should the parameter be set to `false`?
> > > > > I think the parameter should be false here, because the store is not guaranteed to executed, and it would be confusing otherwise.
> > > > > 
> > > > > The relevant part here is that we're not going to speculate any stores, so we don't care about the alignment at all -- we can explicitly skip the alignment code in HandleEndUser for stores to make this clear.
> > > >  Thank you for the suggestion. I've changed the argument's value to `true` and fix the alignment check in the `HandleEndUser` in order to let it work for loads only. Also, I've edited the comment before the check to make it clear that only loads are checked.
> > > > 
> > > > Two questions, please. Currently `Part` can be either a load or a store previously saved in the `ArgParts` map by the same offset. Should the condition that the `MustExecInstr` in the seen before `ArgPart` is a load (or at least no a store) also be added? Also, when the `Part.Alignment` field is recalculated - `Part.Alignment = std::max(Part.Alignment, I->getAlign());`, should we do this whenever `I` is a load only or in any case?
> > > > 
> > > > Thank you.
> > > Hm, so I guess it's not quite true that we don't speculate stores, in the sense that we do insert a store to store the argument into the alloca. Of course, as this store gets promoted later, the actual alignment doesn't really matter. It's probably still best to produce correct alignment for it.
> > > 
> > > So I think the safest thing to do here is just not special case the store case at all -- or would that cause any regressions in tests?
> > No, it adds no regressions, so I've just remove the check that the instruction is a `load` and use the same check for `load` as well as `store` instructions and reformulated the comment a bit to cover both cases.
> Though we could also explicitly set the align of the alloca to the same value, in which case those would always match. Probably a good idea to do that anyway.
>> set the align of the alloca to the same value...

Should the value be `Pair.second.Alignment` as for the `store` into the alloca instruction on line 365?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125485/new/

https://reviews.llvm.org/D125485