Page MenuHomePhabricator

[NFC][LLVM] Merge shouldExpandAtomic*InIR into shouldExpandAtomicInstInIR
AbandonedPublic

Authored by tianshilei1992 on May 20 2022, 11:02 AM.

Details

Summary

This patch merges the four virtual functions, shouldExpandAtomic{Load,Store,CmpX,RMW}InIR
into one function shouldExpandAtomicInstInIR such that we don't have to encode
the instruction information in the function name.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 11:02 AM
tianshilei1992 requested review of this revision.May 20 2022, 11:02 AM
jyknight requested changes to this revision.May 20 2022, 11:27 AM

IMO, this change hurts the readability of the code, not improves it.

This revision now requires changes to proceed.May 20 2022, 11:27 AM

IMO, this change hurts the readability of the code, not improves it.

We already have the instruction information in the argument, and it is the only argument, is it necessary to encode it in the function name as well? One of the advantages having overloaded function is that we don't need to write a series of functions with different names whose only difference is just the argument type.

jdoerfert retitled this revision from [NFC][LLVM] Renmae all shouldExpandAtomic*InIR to shouldExpandAtomicInstInIR to [NFC][LLVM] Rename all shouldExpandAtomic*InIR to shouldExpandAtomicInstInIR.May 20 2022, 11:40 AM

Overloaded virtual functions seem more difficult to read and are more error-prone. Having distinct function names can be useful as people searching code can find the relevant one easily.

tra added a comment.May 20 2022, 11:52 AM

My original suggestion which resulted in this patch was to generalize the API to just one function accepting Instruction* and let the target deal with the details.
For many targets there's a fair amount of duplicated code across the per-instruction variant implementation of shouldExpandX and using single call would allow to consolidate the atomic expansion logic in fewer places.

Using overloads, indeed, only makes things more opaque without reducing the API surface.

My original suggestion which resulted in this patch was to generalize the API to just one function accepting Instruction* and let the target deal with the details.
For many targets there's a fair amount of duplicated code across the per-instruction variant implementation of shouldExpandX and using single call would allow to consolidate the atomic expansion logic in fewer places.

Using overloads, indeed, only makes things more opaque without reducing the API surface.

I did have one patch locally that only keeps the function shouldExpandAtomicInstInIR(Instruction *) and in the body it has something like if (auto *SI = dyn_cast<StoreInst>(I)). In terms of readability, IMO it is same as overload. However, of course it can reduce number of APIs. I can use that method, but it looks like the controversy here is readability.

I did have one patch locally that only keeps the function shouldExpandAtomicInstInIR(Instruction *) and in the body it has something like if (auto *SI = dyn_cast<StoreInst>(I)). In terms of readability, IMO it is same as overload. However, of course it can reduce number of APIs. I can use that method, but it looks like the controversy here is readability.

Without seeing the result of that refactoring I can't say for sure, but I can imagine that could be an improvement over the status quo.

asb added a comment.May 21 2022, 5:57 AM

I did have one patch locally that only keeps the function shouldExpandAtomicInstInIR(Instruction *) and in the body it has something like if (auto *SI = dyn_cast<StoreInst>(I)). In terms of readability, IMO it is same as overload. However, of course it can reduce number of APIs. I can use that method, but it looks like the controversy here is readability.

Without seeing the result of that refactoring I can't say for sure, but I can imagine that could be an improvement over the status quo.

FWIW, I think that would be better too.

use one function

tianshilei1992 edited the summary of this revision. (Show Details)May 22 2022, 10:33 AM
tianshilei1992 retitled this revision from [NFC][LLVM] Rename all shouldExpandAtomic*InIR to shouldExpandAtomicInstInIR to [NFC][LLVM] Merge shouldExpandAtomic*InIR into shouldExpandAtomicInstInIR.
tra added a comment.May 23 2022, 11:21 AM

Looks like it's a wash -- public API is smaller, but the implementations didn't really benefit much. If anything, they got a bit less readable.

I'm fine with dropping this change.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19702

For targets that do have distinct per-instruction logic it may make sense to keep it in separate functions and just add a top-level dispatch function calling the right variant.

tianshilei1992 abandoned this revision.May 23 2022, 2:40 PM