This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Simplify out of range rotate amount.
ClosedPublic

Authored by aemerson on Apr 21 2021, 5:33 PM.

Diff Detail

Event Timeline

aemerson created this revision.Apr 21 2021, 5:33 PM
aemerson requested review of this revision.Apr 21 2021, 5:33 PM
arsenm added inline comments.Apr 21 2021, 5:46 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

I think one off MIRBuilders should never be used

aemerson added inline comments.Apr 21 2021, 5:50 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

What do you suggest here instead?

arsenm added inline comments.Apr 21 2021, 6:00 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

The combiner helper should have a universal builder

paquette added inline comments.Apr 21 2021, 6:05 PM
llvm/test/CodeGen/AArch64/GlobalISel/postlegalizercombiner-rotate.mir
5

Maybe a couple boundary tests would be nice here?

G_CONSTANT == bit size, G_CONSTANT == bit size - 1?

aemerson added inline comments.Apr 21 2021, 6:06 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

The universal builder is a CSE one, and I need a constant folding one here. Maybe CSE should do it too?

arsenm added inline comments.Apr 21 2021, 6:39 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

I honestly don't see the point of having all of these MIRBuilder flavors. Why don't we have just one that does everything?

arsenm added inline comments.Apr 21 2021, 6:41 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

I thought the CSE builder did do constant folding. Also can't you just directly call the constant folding utility function?

foad added a comment.Apr 22 2021, 1:50 AM

What's the point of this? Does it somehow end up generating better code?

If it is useful, you could also do the same for FSHL and FSHR.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

Yes the CSE builder does constant folding. I think ConstantFoldingMIRBuilder should be deleted. It's unused and I was told it was just an experiment.

What's the point of this? Does it somehow end up generating better code?

If it is useful, you could also do the same for FSHL and FSHR.

It allows us to select immediate forms of rotate instructions for AArch64.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3924

Thanks, I hadn't realized since we didn't enable CSEInfo for this combiner pass so it wasn't working.

aemerson updated this revision to Diff 339781.Apr 22 2021, 2:16 PM
  • Add more tests.
  • Factor out the APInt checking part of the constant folder to be used as a query.
  • Enable CSEInfo in the postlegalizer combiner so that we don't have to instantiate a constant-folding MIRBuilder.
foad added a comment.Apr 23 2021, 1:19 AM

What's the point of this? Does it somehow end up generating better code?

It allows us to select immediate forms of rotate instructions for AArch64.

I would expect most targets to handle that during instruction selection by ignoring the high bits of the rotate amount, but perhaps there's some reason why it's awkward to do that for AArch64. Would it make sense for your new combine to be completely AArch64-specific? Currently you have added it to all_combines so it will be run on other targets too.

llvm/include/llvm/Target/GlobalISel/Combine.td
622

Seems a bit odd to include this in "funnel_shift_combines" since it doesn't actually touch funnel shifts.

What's the point of this? Does it somehow end up generating better code?

It allows us to select immediate forms of rotate instructions for AArch64.

I would expect most targets to handle that during instruction selection by ignoring the high bits of the rotate amount, but perhaps there's some reason why it's awkward to do that for AArch64. Would it make sense for your new combine to be completely AArch64-specific? Currently you have added it to all_combines so it will be run on other targets too.

That's possible, but I assumed that generic code wanted this since this is already done in DAGCombine for rotates.

aemerson updated this revision to Diff 340122.Apr 23 2021, 11:34 AM

Only run this for AArch64.

foad added inline comments.Apr 26 2021, 2:23 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3918–3919

Why not just return true? Why would this ever fail?

aemerson updated this revision to Diff 340657.Apr 26 2021, 2:47 PM

Remove constant folding check.

foad accepted this revision.Apr 29 2021, 2:47 AM

Looks fine to me. Could also do it for funnel shifts if you want.

This revision is now accepted and ready to land.Apr 29 2021, 2:47 AM

Looks fine to me. Could also do it for funnel shifts if you want.

Thanks. We lower funnel shifts though so it's not something we can test for AArch64.

This revision was landed with ongoing or failed builds.Apr 29 2021, 2:06 PM
This revision was automatically updated to reflect the committed changes.