[PATCH] AArch64: Don't modify other modules in AArch64PromoteConstant

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Mar-18, at 15:27, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
>> 
>> On 2016-Mar-18, at 13:28, Justin Bogner <mail at justinbogner.com> wrote:
>> 
>> "Duncan P. N. Exon Smith via llvm-commits" <llvm-commits at lists.llvm.org>
>> writes:
>>> This patch stops modifying other modules in `AArch64PromoteConstant`
>>> when the constant is `ConstantData` (a horrible accident, I'm sure,
>>> caught by an experimental follow-up to r261464).
>>> 
>>> Previously, this walked through all the users of a constant, but that
>>> reaches into other modules when the constant doesn't depend transitively
>>> on a `GlobalValue`!  Since we're walking instructions anyway, just
>>> modify the instructions we actually see.
>>> 
>>> As a drive-by, instead of storing `Use` and getting the instructions
>>> again via `Use::getUser()` (which is not a constantant time lookup),
>>> store `std::pair<Instruction, unsigned>`.  Besides being cheaper, this
>>> makes it easier to drop use-lists form `ConstantData` in the future.
>>> (I threw this in because I was touching all the code anyway.)
>>> 
>>> Because the patch completely changes the traversal logic, it looks
>>> like a rewrite of the pass, but the core logic is all the same (or
>>> should be, minus the out-of-module changes).  In other words, there
>>> should be NFC as long as the LLVMContext only has a single Module.
>>> 
>>> I didn't think of a good way to test this, but I hope to submit a
>>> patch eventually that makes walking these use-lists illegal.
>> 
>> I guess this is difficult to test because it's hard to convince opt/llc
>> to load multiple modules?
> 
> Yes, that's the problem.
> 
>> The existing tests should cover the NFC parts
>> of this, so maybe this is okay.
>> 
>> This basically LGTM, but see my comment below.
>> 
>>> 
>>> -bool AArch64PromoteConstant::computeAndInsertDefinitions(Constant *Val) {
>>> -  InsertionPointsPerFunc InsertPtsPerFunc;
>>> -  computeInsertionPoints(Val, InsertPtsPerFunc);
>>> -  return insertDefinitions(Val, InsertPtsPerFunc);
>>> +  return true;
>> 
>> Looks like this function only ever returns true. If that's intentional
>> maybe we should change it to return void, but I suspect it was a
>> mistake.
> 
> It's correct to always return true.  The function it replaced
> (AArch64PromoteConstant::insertDefinitions) also always returned
> true, but I made it more obvious.  It looks like this is a half-
> measure; we should just remove the return value entirely.

Removing that one made it clear that a few other `bool`s were
unnecessary.  See the updated patch (also attached
remove-unnecessary-bools.patch, which just has the diff against
the last one).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AArch64-Don-t-modify-other-modules-in-AArch64Prom-v2.patch
Type: application/octet-stream
Size: 21143 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160318/5c9e120c/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-unnecessary-bools.patch
Type: application/octet-stream
Size: 3573 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160318/5c9e120c/attachment-0001.obj>