Related to: D78659
Details
Diff Detail
Event Timeline
Handle nomerge flag in LowerCall at targets AArch64, ARM, PowerPC, RISCV, and add test cases for each target.
Overall, this looks good to me, but I'd like to ask @craig.topper for an opinion. His area of expertise is the x86 backend.
llvm/include/llvm/CodeGen/SelectionDAG.h | ||
---|---|---|
281 | Please zero initialize this, otherwise the default constructor will not initialize it, and if entries in this hash table are added for other reasons, it will be uninitialized. You could observe this by combining the nomerge attribute with __declspec(allocator), but IMO a test is not necessary. | |
1916 | If NoMerge is false, you can skip this hash table insertion. Otherwise, we will do this hash insertion for each and ever call instruction, but we only need to do it for this feature. |
Zero Initialize NoMerge in CallSiteDbgInfo.
Put Check in addNoMergeSiteInfo to avoid unnecessary hash table insertion.
llvm/include/llvm/CodeGen/SelectionDAG.h | ||
---|---|---|
1916 | Previously, I put the check before each call site of this function. |
After this lands, can you follow this up by migrating the sanitizer passes to using the attribute instead of using empty inline asm blobs? Send the patch to @vitalybuka. This is the code I'm thinking of:
https://github.com/llvm/llvm-project/blob/028bfdd8913616f7a3e57e8ef5c2a9990e528ff0/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L1626
https://github.com/llvm/llvm-project/blob/d6695e18763a05b30cb336c18157175277da8f4b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L864
... and any similar uses.
Run check-asan, check-msan, etc to run the sanitizer integration tests, and confirm that they pass. You can run check-all to run everything and be sure as well.
llvm/include/llvm/CodeGen/MachineInstr.h | ||
---|---|---|
109 | It's not obvious to me what this means. These are also supposed to be droppable optimization flags, but it seems like you're using this for something semantically required? |
llvm/include/llvm/CodeGen/MachineInstr.h | ||
---|---|---|
109 | I don't think BundledPred or FrameSetup are droppable, IMO they are semantically significant. The only alternative I see to this would be to encode this information in the MCInstrDesc, which would require mirroring the entire family of X86 CALL instructions, of which there are many. |
Sure, I will do after this lands.
llvm/include/llvm/CodeGen/MachineInstr.h | ||
---|---|---|
109 | This NoMerge flag is used to disable branch-fold optimization when present. What do you mean something semantically required? |
llvm/include/llvm/CodeGen/MachineInstr.h | ||
---|---|---|
109 | I forgot those were also here. Maybe this should be sorted up to the other mandatory flags. I'm pretty sure FrameSetup only changes debug info, so I guess that's not exactly mandatory? I would still like a better comment here. This one just restates the name of the flag and doesn't tell me what it means by merge |
I think this looks good, but I want to find another reviewer for CodeGen. @craig.topper or @arsenm, are you OK reviewing this, or can you recommend another reviewer?
llvm/include/llvm/CodeGen/MachineInstr.h | ||
---|---|---|
109 | I second Matt's comments on reordering the flags to group this flag away from the math flags, which are generally ignorable, and you can expand on this comment here. The notion of "merging" instructions isn't self-explanatory. Perhaps "Passes that drop source location (e.g. tail merging) info should skip this instruction." could work. |
llvm/test/CodeGen/PowerPC/nomerge.ll | ||
---|---|---|
32 | Change the three CHECK: bl bar to CHECK-NEXT: bl bar It makes the reader clear they are first instructions in their basic blocks and there will be a friendly error message if things go wrong. ; CHECK: # %if.then2 ; CHECK-NEXT: bl bar | |
llvm/test/CodeGen/RISCV/nomerge.ll | ||
32 | ditto |
Change some CHECK to CHECK-NEXT where the call to bar is first instruction in blocks.
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
4272 | Does this need to be in this else or can it apply to both paths? |
rebase and address comment.
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
4272 | Yes, it can be applied to both paths. |
It's not obvious to me what this means. These are also supposed to be droppable optimization flags, but it seems like you're using this for something semantically required?