This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add the vector integer min/max instructions to isAssociativeAndCommutative.
ClosedPublic

Authored by craig.topper on Jun 1 2019, 6:38 PM.

Details

Summary

As far as I know these should be freely reassociatable just like
the floating point MAXC/MINC instructions.

The test changes demonstrate that something happened from this
patch, but I'm not sure we always made the right choice. I
think the fact that llc defaults to "generic" CPU is causing
us to not have a scheduler model so MachineCombiner doesn't
make the best decisions. But this isn't unique to these
instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Jun 1 2019, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2019, 6:38 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

I didn't review all of the test diffs, but all of the ones that I did look at are regressions. I agree that this is MachineCombiner's fault when a CPU model is absent and independent of this particular code change, but I think we should adjust the test files to either include a CPU model across the board or include an extra RUN line with and without a CPU model (could split off some subset of these tests into separate files so we're not wasting test time on every variation of these patterns). That way, we'll have some dedicated MachineCombiner test coverage and know more easily if something breaks/changes in there.

This might be a sign that we need to review the generic CPU - for instance should we enable PostRA?

Incidently, what is the reasoning that we don't enable PostRA on big Intel core models?

This might be a sign that we need to review the generic CPU - for instance should we enable PostRA?

We need to differentiate here: the generic CPU ("target-cpu"="x86-64") is set by clang and uses the SandyBridge scheduler model, but these regression tests have *no* CPU model rather than the generic model. Maybe we should have llc set the generic model as the default?

This might be a sign that we need to review the generic CPU - for instance should we enable PostRA?

We need to differentiate here: the generic CPU ("target-cpu"="x86-64") is set by clang and uses the SandyBridge scheduler model, but these regression tests have *no* CPU model rather than the generic model. Maybe we should have llc set the generic model as the default?

That will probably result in every single test being regenerated?

-Add the SQ and UQ instructions.
-Fix the MIN list which I thought I just copied from the MAX list and search and replaced.
-Show test diffs in machine-combiner-int-vec.ll which now tests min/max

Rebase after adding avx512bw to the avx512 command line to get proper coverage of v64i8/v32i16

spatel accepted this revision.Jun 5 2019, 6:17 AM

This might be a sign that we need to review the generic CPU - for instance should we enable PostRA?

We need to differentiate here: the generic CPU ("target-cpu"="x86-64") is set by clang and uses the SandyBridge scheduler model, but these regression tests have *no* CPU model rather than the generic model. Maybe we should have llc set the generic model as the default?

That will probably result in every single test being regenerated?

I haven't tried the experiment, but yes, I'd guess there would be substantial churn. But it's not good that we use/test a default configuration that does not match what most users see.

This patch has dedicated tests now, so LGTM.

This revision is now accepted and ready to land.Jun 5 2019, 6:17 AM
This revision was automatically updated to reflect the committed changes.