Page MenuHomePhabricator

[MIPS GlobalISel] ClampScalar G_SHL, G_ASHR and G_LSHR
ClosedPublic

Authored by Petar.Avramovic on Aug 21 2019, 7:02 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 7:02 AM

@aemerson @arsenm
test-suite/SingleSource/UnitTests/2003-05-31-LongShifts.c used to give bad result in high bits for some values,
changes to narrow scalar for G_SHL, G_ASHR and G_LSHR fixes that on MIPS32. Does it work for you?

arsenm added inline comments.Aug 21 2019, 8:24 AM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2993 ↗(On Diff #216392)

This looks like it was copied incorrectly from the DAG version. Can you commit this part separately?

3001 ↗(On Diff #216392)

This now differs from the DAG version?

3014–3016 ↗(On Diff #216392)

As far as I can tell this is ultimately still the same as the DAG version?

3020 ↗(On Diff #216392)

Shouldn't need a new MachineInstrBuilder

arsenm added inline comments.Aug 21 2019, 8:42 AM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
3020 ↗(On Diff #216392)

Oh, I thought this was a MachineIRBuilder. This can still be simplified to just a Register

Petar.Avramovic marked an inline comment as done.Aug 21 2019, 8:58 AM
Petar.Avramovic added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
3001 ↗(On Diff #216392)

Oh, now I see it, didn't look into DAG version, I was guided by the test.
Will have to double check this.

Petar.Avramovic marked 4 inline comments as done.Aug 21 2019, 10:00 AM
Petar.Avramovic added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2993 ↗(On Diff #216392)

The whole change in LegalizerHelper.cpp and tests including merge of G_ASHR and G_LSHR into same case? OrLHS for G_ASHR also had typo, it used buildAShr instead of buildShl.

3001 ↗(On Diff #216392)

MIPS has custom legalize for shifts in SDAG so that is target optimization since shift for Amt and AmtExcess is effectively the same thing since only low 5 bits are considered for shift amount. Will return AmtExcess here.

3014–3016 ↗(On Diff #216392)

yes

arsenm added inline comments.Aug 21 2019, 10:03 AM
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2993 ↗(On Diff #216392)

At a minimum the correctness fix should be split out, but it seems like there are 3 separate changes here

3001 ↗(On Diff #216392)

If it doesn't matter for other targets and it's effectively the same, I don't care

Petar.Avramovic marked an inline comment as done.
Petar.Avramovic edited the summary of this revision. (Show Details)

Split changes to three smaller patches.

This revision is now accepted and ready to land.Tue, Aug 27, 6:21 AM
This revision was automatically updated to reflect the committed changes.