[PATCH] D93927: [ArgPromotion] Copy !range metadata for loads.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:318-325
+            bool isOrigLoadGuaranteedToExecute = true;
+            for (const Instruction &II : *BB) {
+              if (!isOrigLoadGuaranteedToExecute || &II == OrigLoad) {
+                break;
+              }
+              isOrigLoadGuaranteedToExecute =
+                  llvm::isGuaranteedToTransferExecutionToSuccessor(&II);
----------------
wecing wrote:
> MatzeB wrote:
> > avoid check for isOrigLoadGuaranteedToExecute in every loop iteration.
> We need to check the result of llvm::isGuaranteedToTransferExecutionToSuccessor() in every iteration anyways. The difference between my code and yours is minimal. :)
Yep the difference is minuscule. Though as long as the code doesn't get more complicated, why not... Anyway no strong opinion if you want to keep as-is.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:333-337
+              newLoad->copyMetadata(*OrigLoad, {LLVMContext::MD_nontemporal,
+                                                LLVMContext::MD_nonnull,
+                                                LLVMContext::MD_dereferenceable,
+                                                LLVMContext::MD_noundef,
+                                                LLVMContext::MD_range});
----------------
wecing wrote:
> MatzeB wrote:
> > How about using `llvm::copyMetadataForLoad` from `Transforms/Utils/Local.h` at a first glance it  seems to work fine even for loads in different functions.
> Quoting @fhahn's comment above:
> 
> > I am not sure if it is safe to copy all metadata, it would probably be better to add them one-by-one. I was mostly asking to check which other kinds would be safe. For example, debug metadata is function specific, so it can't simply be copied I think.
> 
> `llvm::copyMetadataForLoad` also copies debug metadata, so I think it's not safe to use here. It might be better to just hand pick a few ones that are safe to copy.
Maintaining a custom list of metadata to copy is not great for future extension. Whenever someone adds a new kind of metadata he has to be aware of this list, understand the `ArgumentPromotion` pass and then add the new metadata to the list or the new metadata will get lost by the optimization.

I wonder what makes debug info function-dependent. At a first glance it doesn't look like it is (given that the DILocations has a scope pointing to a DISubprogram which specifies the relevant function). Anyway I'm not too experienced here, so I may miss something.

Well I'm gonna accept the changes as-is, but I think it's not ideal to have a custom list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93927