Diff Detail
Event Timeline
This can land before D98050 (or any other fma combine) but for a review it might be easier to see src modifiers in .ll tests than .mir.
If this is accepted before, we can commit it without .ll tests.
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h | ||
---|---|---|
595–609 | Does this need to be a separate patch with a unit test? |
Looks like the logic is mostly copied from selectiondag, so I guess it's OK.
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h | ||
---|---|---|
595 | Does this match exactly one copy? Would it be more useful to skip zero or more copies? | |
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
2321–2323 | I think these three lines can be replaced with IsFMA ? !Subtarget->hasFmaMixInsts() : !Subtarget->hasMadMixInsts() |
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h | ||
---|---|---|
595 | m_Copy matches exactly one copy. This matches zero or one copy. Renamed to m_IgnoreCopy, StripCopy was a little misleading. It could be easily changed to zero or more, but I did not run into a case where that was necessary. |
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h | ||
---|---|---|
595–609 | I feel like it would be better to match multiple copies, kind of like getDefIgnoringCopies. You'll probably run into multiple copies one day, and chances are you'll want to walk past them too. |
Updated IgnoreCopy matcher to skip through multiple copies.
IR has matchers like m_TruncOrSelf and m_ZExtOrSelf which match one or zero of those nodes. So I guess it's not unprecedented to have a matcher like this.
Maybe rename to m_CopyOrSelf?
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h | ||
---|---|---|
595 | The practical case where you would want to use this is when you know you can ignore cross bank copies, which I think should only ever see 1 copy | |
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
3512 | Really this copy shouldn't be here, but regbankselect and the post-regbank combiner don't optimally handle constants. For an inline constant we should always have the constant appear directly with the result regbank. Can you add a fix to remove this? |
Does this match exactly one copy? Would it be more useful to skip zero or more copies?