This is an archive of the discontinued LLVM Phabricator instance.

[ARM][CGP] Check trunc value size before replacing
ClosedPublic

Authored by samparker on Jan 22 2019, 1:36 AM.

Details

Summary

In the last stage of type promotion, we replace any zext that uses a new trunc with the operand of the trunc. This is okay when we only allowed one type to be optimised, but now its the case that the trunc maybe needed to produce a more narrow type than the one we were optimising for. So we need to check this before doing the replacement.

Diff Detail

Event Timeline

samparker created this revision.Jan 22 2019, 1:36 AM

Was just getting up to speed with this again; just a first round of nits before I start looking at the actual functional change.

lib/Target/ARM/ARMCodeGenPrepare.cpp
188

There's not enough context to show the class definition:

namespace {
class IRPromoter {
  SmallPtrSet<Value*, 8> NewInsts;
  SmallPtrSet<Instruction*, 4> InstsToRemove;
  DenseMap<Value*, SmallVector<Type*, 4>> TruncTysMap;
  SmallPtrSet<Value*, 8> Promoted;
  Module *M = nullptr;
  LLVMContext &Ctx;
  IntegerType *ExtTy = nullptr;
  IntegerType *OrigTy = nullptr;
  SmallPtrSetImpl<Value*> *Visited;
  SmallPtrSetImpl<Value*> *Sources;
  SmallPtrSetImpl<Instruction*> *Sinks;
  SmallPtrSetImpl<Instruction*> *SafeToPromote;
  ...

But I started looking at where ExtTy was defined again, because it was mentioned in the comments of this change. It is quite a crucial aspect of the IRPromoter, so perhaps you can spend a few comments on what it is and does.

329

nit: trailing whitespace

607

nit: trailing whitespace here

703

typo: "we can we can"

703

nit: they = zexts?

705

typo: remove "the"?

samparker updated this revision to Diff 182881.Jan 22 2019, 3:16 AM

Cheers, added some comments and removed whitespace.

SjoerdMeijer accepted this revision.Jan 23 2019, 1:07 AM

Thanks, LGTM

This revision is now accepted and ready to land.Jan 23 2019, 1:07 AM
This revision was automatically updated to reflect the committed changes.