In the commentary for D11660, I noted that I wasn't sure if it was alright to create new integer machine instructions without also creating the implicit EFLAGS operand. From what I can see, the implicit operand is always created by the MachineInstrBuilder based on the instruction type, so we don't have to do that explicitly. However, in reviewing the debug output, I noticed that the operand was not marked as 'dead'. I think the machine combiner should do that to preserve future optimization opportunities that may be checking for that dead EFLAGS operand themselves.
Details
Diff Detail
Event Timeline
Why would reassociation result in dead eflags again? Because it can kick-in only when eflags are dead before reassoc, so they must be dead after? The comment should be clear about why setting the attribute is correct.
-Gerolf
Patch updated:
- Added comment to better explain why the new operands must be dead.
- Added more asserts to verify that assumptions are intact.
- Split the EFLAGS check into its own function because eventually we'll want to hoist the non-architecture-specific logic into the MachineCombiner itself (there's already a TODO comment about this).
test/CodeGen/X86/machine-combiner-int.ll | ||
---|---|---|
2 | Nope - just wasn't aware of that option. I'll update the patch. Thanks for the suggestion! |
Patch updated: test case now uses '-stop-after machine-combiner' to be more efficient.
Thanks for your follow-ups. I leave it up to you if you want to address my remaining comments.
LGTM.
lib/Target/X86/X86InstrInfo.cpp | ||
---|---|---|
6441 | How about: This is an ..... Set special operand attributes post re-association? | |
6443 | Would setSpecialOperandAttributes() be a better name? | |
6450 | Looks a bit of an overkill here. |
lib/Target/X86/X86InstrInfo.cpp | ||
---|---|---|
6443 | Yes, that's a better name. Thanks! | |
6450 | Agree - it's assert-heavy. But I wanted to be extra safe for when some of the code gets hoisted up to the machine combiner. It may not be so obvious what the architecture-specific assumptions are at that point. I'll leave this in for now, and if it feels unnecessary later, we can get rid of it. |
How about: This is an ..... Set special operand attributes post re-association?