This is an archive of the discontinued LLVM Phabricator instance.

Remove CRC32 instructions from AArch64InstrInfo::hasShiftedReg
ClosedPublic

Authored by azharudd on Feb 22 2017, 2:34 PM.

Details

Summary

A53 scheduler causes an assertion failure on all CRC instructions:
include/llvm/CodeGen/MachineInstr.h:280: const llvm::MachineOperand
&llvm::MachineInstr::getOperand(unsigned int) const: Assertion `i <
getNumOperands() && "getOperand() out of range!"' failed.

The case statements corresponding to CRC instructions are incorrect and should
be removed.

Also adding a testcase while on this.

Diff Detail

Repository
rL LLVM

Event Timeline

azharudd created this revision.Feb 22 2017, 2:34 PM
rengolin added inline comments.Feb 23 2017, 1:33 AM
test/CodeGen/AArch64/arm64-crc32.ll
2 ↗(On Diff #89422)

Why is this new run line related? Maybe add this to the commit message would help.

azharudd edited the summary of this revision. (Show Details)Mar 2 2017, 3:41 PM
azharudd edited the summary of this revision. (Show Details)
azharudd edited the summary of this revision. (Show Details)
azharudd marked an inline comment as done.
rengolin edited edge metadata.Mar 4 2017, 3:43 PM

Can you update the context around using git diff -U999?

rengolin accepted this revision.Mar 4 2017, 3:46 PM

Hi, right, so Tim has approved it on the mailing list, which didn't get transferred here.

But he also said an additional interesting point why you have to use mcpu and not another mattr: this is a code path that only triggers with A53 because of its scheduler.

LGTM, too. Can you just add the scheduler info (instead of just saying it hits with A53) on the commit line?

thanks!

This revision is now accepted and ready to land.Mar 4 2017, 3:46 PM
azharudd updated this revision to Diff 90880.Mar 7 2017, 10:26 AM
azharudd edited the summary of this revision. (Show Details)

Updated the context around the change.

Can you update the context around using git diff -U999?

Thanks Renato. I have updated it.

Hi, right, so Tim has approved it on the mailing list, which didn't get transferred here.

But he also said an additional interesting point why you have to use mcpu and not another mattr: this is a code path that only triggers with A53 because of its scheduler.

LGTM, too. Can you just add the scheduler info (instead of just saying it hits with A53) on the commit line?

thanks!

Updated the commit message to mention the A53 scheduler.

evandro added a subscriber: evandro.Mar 8 2017, 8:30 AM
This revision was automatically updated to reflect the committed changes.