Page MenuHomePhabricator

[SVE] Fix crashes with inline assembly
ClosedPublic

Authored by david-arm on Dec 3 2020, 1:27 AM.

Details

Summary

All the crashes found compiling inline assembly are fixed in this
patch by changing AArch64TargetLowering::getRegForInlineAsmConstraint
to be more resilient to mismatched value and register types. For
example, it makes no sense to request a predicate register for
a nxv2i64 type and so on.

Tests have been added here:

test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll

Diff Detail

Event Timeline

david-arm created this revision.Dec 3 2020, 1:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
david-arm requested review of this revision.Dec 3 2020, 1:27 AM
sdesmalen added inline comments.Dec 3 2020, 2:49 AM
llvm/test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll
33

How is foo4 different from foo1?

david-arm added inline comments.Dec 3 2020, 2:52 AM
llvm/test/CodeGen/AArch64/inline-asm-constraints-bad-sve.ll
33

Fair point. The original C tests were different, but I hadn't spotted they'd boiled down to the same IR. I'll remove it.

david-arm updated this revision to Diff 309218.Dec 3 2020, 3:48 AM
sdesmalen accepted this revision.Dec 7 2020, 3:32 AM

LGTM

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7528

nit: is it worth rewriting this to:

TypeSize VTSize = VT.getSizeInBits();
if (VTSize == TypeSize::getScalable(1))
  return ...
if (VTSize == TypeSize::getFixed(16))
  return ...
if (VTSize == TypeSize::getFixed(32))
  return ...

if (VT.isScalableVector())
  return std::make_pair(0U, nullptr);
break;
7561

nit:

if (!VT.isScalableVector() || VT.getVectorElementType() != MVT::i1)
  return std::make_pair(0U, nullptr);
bool restricted = (PC == PredicateConstraint::Upl);
return restricted ? std::make_pair(0U, &AArch64::PPR_3bRegClass)
                  : std::make_pair(0U, &AArch64::PPRRegClass);

(that saves having to indent the block)

This revision is now accepted and ready to land.Dec 7 2020, 3:32 AM
This revision was automatically updated to reflect the committed changes.
david-arm added inline comments.Dec 8 2020, 5:51 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7528

Hi Sander, I tried rewriting this and then I realised we can't check "VTSize == TypeSize::getScalable(1)" because this is looking for a size of "vscale x 1 byte", which isn't the same as a predicate. I felt it was more obvious to check the element type explicitly. Hope that's ok!