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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
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.
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.
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. |
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.